Re: [PATCH] io_uring: add overflow checks for poll refcounting

[Dylan Yudaken <dylany@fb.com>]

On Wed, 2022-03-23 at 19:48 +0000, Pavel Begunkov wrote:
> On 3/23/22 15:07, Dylan Yudaken wrote:
> > On Wed, 2022-03-23 at 11:14 +0000, Pavel Begunkov wrote:
> > > 
> > ...
> > >   
> > > -#define IO_POLL_CANCEL_FLAG    BIT(31)
> > > -#define IO_POLL_REF_MASK       GENMASK(30, 0)
> > > +/* keep the sign bit unused to improve overflow detection */
> > > +#define IO_POLL_CANCEL_FLAG    BIT(30)
> > > +#define IO_POLL_REF_MASK       GENMASK(29, 0)
> > > +
> > > +/* 2^16 is choosen arbitrary, would be funky to have more than
> > > that
> > > */
> > > +#define io_poll_ref_check_overflow(refs) ((unsigned int)refs >=
> > > 65536u)
> > > +#define io_poll_ref_check_underflow(refs) ((int)refs < 0)
> > >   
> > 
> > I believe if the cancel flag is set, then this will not catch an
> > underflow but the result will be the cancel flag unset. You could
> > fix
> > by also checking for overflow on the masked bits.
> 
> Good point. I'm thinking now about using bit(0) for
> IO_POLL_CANCEL_FLAG
> and 1-31 for refs. We'd need to do +2 in io_poll_get_ownership() but
> the sign logic should work w/o extra weirdness.
> 

I think that should work.

If you're checking all the time anyway, you could also use bit 32 for
cancel, bit 31 init as 0, and bit 30 init as 1. Overflow/underflow
happens when bit 30 changes but still doesnt do anything to the cancel
bit.
In this case the io_poll_put_ownership might want to check for too big
a decrement in `nr`.

I don't have a strong opinion, just that +2 is a weird behaviour for a
reference count.