Re: [PATCH 1/3] futex: remove duplicated code

[Rich Felker]

On Wed, Mar 08, 2017 at 08:36:30PM -0800, H. Peter Anvin wrote:
> <cmetcalf@xxxxxxxxxxxx>,Thomas Gleixner <tglx@xxxxxxxxxxxxx>,Ingo Molnar <mingo@xxxxxxxxxx>,Chris Zankel <chris@xxxxxxxxxx>,Max Filippov <jcmvbkbc@xxxxxxxxx>,Arnd Bergmann <arnd@xxxxxxxx>,x86@xxxxxxxxxx,linux-alpha@xxxxxxxxxxxxxxx,linux-snps-arc@xxxxxxxxxxxxxxxxxxx,linux-arm-kernel@xxxxxxxxxxxxxxxxxxx,linux-hexagon@xxxxxxxxxxxxxxx,linux-ia64@xxxxxxxxxxxxxxx,linux-mips@xxxxxxxxxxxxxx,openrisc@xxxxxxxxxxxxxxxxxxxx,linux-parisc@xxxxxxxxxxxxxxx,linuxppc-dev@xxxxxxxxxxxxxxxx,linux-s390@xxxxxxxxxxxxxxx,linux-sh@xxxxxxxxxxxxxxx,sparclinux@xxxxxxxxxxxxxxx,linux-xtensa@xxxxxxxxxxxxxxxx,linux-arch@xxxxxxxxxxxxxxx
> From: hpa@xxxxxxxxx
> Message-ID: <83324528-AAA1-4BED-B0C7-48426ECBA261@xxxxxxxxx>
> 
> On March 8, 2017 8:16:49 PM PST, Rob Landley <rob@xxxxxxxxxxx> wrote:
> >On 03/04/2017 07:05 AM, Russell King - ARM Linux wrote:
> >> On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote:
> >>> diff --git a/kernel/futex.c b/kernel/futex.c
> >>> index b687cb22301c..c5ff9850952f 100644
> >>> --- a/kernel/futex.c
> >>> +++ b/kernel/futex.c
> >>> @@ -1457,6 +1457,42 @@ futex_wake(u32 __user *uaddr, unsigned int
> >flags, int nr_wake, u32 bitset)
> >>>  	return ret;
> >>>  }
> >>>  
> >>> +static int futex_atomic_op_inuser(int encoded_op, u32 __user
> >*uaddr)
> >>> +{
> >>> +	int op = (encoded_op >> 28) & 7;
> >>> +	int cmp = (encoded_op >> 24) & 15;
> >>> +	int oparg = (encoded_op << 8) >> 20;
> >>> +	int cmparg = (encoded_op << 20) >> 20;
> >> 
> >> Hmm.  oparg and cmparg look like they're doing these shifts to get
> >sign
> >> extension of the 12-bit values by assuming that "int" is 32-bit -
> >> probably worth a comment, or for safety, they should be "s32" so it's
> >> not dependent on the bit-width of "int".
> >
> >I thought Linux depended on the LP64 standard for all architectures?
> >
> >Standard: http://www.unix.org/whitepapers/64bit.html
> >Rationale: http://www.unix.org/version2/whatsnew/lp64_wp.html
> >
> >So int has a defined bit width (32) on linux?
> >
> >Rob
> 
> Linux is ILP32 on 32-bit architectures and LP64 on 64-bit
> architectures, but that doesn't inherently make this stuff clear.

32-bit int is part of the futex ABI anyway, but that doesn't justify
gratuitous assumption of it in other places where the assumption and
the intent may not be clear to the reader. It also doesn't justify all
the UB from invalid shifts in the above code.

Rich