Re: [PATCH v2 05/14] futex: Add sys_futex_wake()

[André Almeida]

Em 10/08/2023 09:13, Peter Zijlstra escreveu:
> On Wed, Aug 09, 2023 at 07:25:19PM -0300, André Almeida wrote:
> > Hi Peter,
> >
> > Em 07/08/2023 09:18, Peter Zijlstra escreveu:
> > > To complement sys_futex_waitv() add sys_futex_wake(). This syscall
> > > implements what was previously known as FUTEX_WAKE_BITSET except it
> > > uses 'unsigned long' for the bitmask and takes FUTEX2 flags.
> > >
> > > The 'unsigned long' allows FUTEX2_SIZE_U64 on 64bit platforms.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > Acked-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > ---
> >
> > [...]
> >
> > > +/*
> > > + * sys_futex_wake - Wake a number of futexes
> > > + * @uaddr:	Address of the futex(es) to wake
> > > + * @mask:	bitmask
> > > + * @nr:		Number of the futexes to wake
> > > + * @flags:	FUTEX2 flags
> > > + *
> > > + * Identical to the traditional FUTEX_WAKE_BITSET op, except it is part of the
> > > + * futex2 family of calls.
> > > + */
> > > +
> > > +SYSCALL_DEFINE4(futex_wake,
> > > +		void __user *, uaddr,
> > > +		unsigned long, mask,
> > > +		int, nr,
> > > +		unsigned int, flags)
> > > +{
> >
> > Do you think we could have a
> >
> > 	if (!nr)
> > 		return 0;
> >
> > here? Otherwise, calling futex_wake(&f, 0, flags) will wake 1 futex (if
> > available), which is a strange undocumented behavior in my opinion.
>
> Oh 'cute' that.. yeah, but how about I put it ...
>
> > > +	if (flags & ~FUTEX2_VALID_MASK)
> > > +		return -EINVAL;
> > > +
> > > +	flags = futex2_to_flags(flags);
> > > +	if (!futex_flags_valid(flags))
> > > +		return -EINVAL;
> > > +
> > > +	if (!futex_validate_input(flags, mask))
> > > +		return -EINVAL;
>
> here, because otherwise we get:
>
> 	sys_futex_wake(&f, 0xFFFF, 0, FUTEX2_SIZE_U8)
>
> to return 0, even though that is 'obviously' nonsensical and should
> return -EINVAL. Or even garbage flags would be 'accepted'.
>
> (because 0xFFFF is larger than U8 can accomodate)

That make sense to me, but we would also want to validate the value of 
f, if it's NULL or something strange to return -EINVAL... but this 
happens only inside get_futex_key()...

To make this right, I think we would need to move this verification to 
the syscall validation part:

	if (unlikely((address % sizeof(u32)) != 0))
		return -EINVAL;

	if (unlikely(!access_ok(uaddr, sizeof(u32))))
		return -EFAULT;

And have u32 replaced with the proper size being used.

> > > +
> > > +	return futex_wake(uaddr, flags, nr, mask);
> > > +}