[PATCH 1/3] futex: remove duplicated code

[PATCH 1/3] futex: remove duplicated code

[Heiko Carstens]

On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote:
> There is code duplicated over all architecture's headers for
> futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> and comparison of the result.
> 
> Remove this duplication and leave up to the arches only the needed
> assembly which is now in arch_futex_atomic_op_inuser.
> 
> Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> remove pointless access_ok() checks") as access_ok there returns true.
> We introduce it back to the helper for the sake of simplicity (it gets
> optimized away anyway).
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  arch/s390/include/asm/futex.h       | 23 ++++-------------
>  include/asm-generic/futex.h         | 50 +++++++------------------------------
>  kernel/futex.c                      | 36 ++++++++++++++++++++++++++

Looks good to me and still boots on s390. Therefore for the s390 bits:
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Thanks!

[PATCH 1/3] futex: remove duplicated code

[Chris Metcalf]

On 3/3/2017 7:27 AM, Jiri Slaby wrote:
> There is code duplicated over all architecture's headers for
> futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> and comparison of the result.
>
> Remove this duplication and leave up to the arches only the needed
> assembly which is now in arch_futex_atomic_op_inuser.
>
> Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> remove pointless access_ok() checks") as access_ok there returns true.
> We introduce it back to the helper for the sake of simplicity (it gets
> optimized away anyway).
>
> Signed-off-by: Jiri Slaby<jslaby@suse.cz>

Acked-by: Chris Metcalf <cmetcalf@mellanox.com> [for tile]

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com

[PATCH 1/3] futex: remove duplicated code

[Russell King - ARM Linux]

On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote:
> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> index 6795368ad023..cc414382dab4 100644
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -128,20 +128,10 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>  #endif /* !SMP */
>  
>  static inline int
> -futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
> +arch_futex_atomic_op_inuser(int op, int oparg, int *oval, 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;
>  	int oldval = 0, ret, tmp;
>  
> -	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
> -		oparg = 1 << oparg;
> -
> -	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
> -		return -EFAULT;
> -
>  #ifndef CONFIG_SMP
>  	preempt_disable();
>  #endif
> @@ -172,17 +162,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
>  	preempt_enable();
>  #endif
>  
> -	if (!ret) {
> -		switch (cmp) {
> -		case FUTEX_OP_CMP_EQ: ret = (oldval == cmparg); break;
> -		case FUTEX_OP_CMP_NE: ret = (oldval != cmparg); break;
> -		case FUTEX_OP_CMP_LT: ret = (oldval < cmparg); break;
> -		case FUTEX_OP_CMP_GE: ret = (oldval >= cmparg); break;
> -		case FUTEX_OP_CMP_LE: ret = (oldval <= cmparg); break;
> -		case FUTEX_OP_CMP_GT: ret = (oldval > cmparg); break;
> -		default: ret = -ENOSYS;
> -		}
> -	}
> +	if (!ret)
> +		*oval = oldval;
> +
>  	return ret;
>  }
>  
> 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".

> +	int oldval, ret;
> +
> +	if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
> +		oparg = 1 << oparg;

I guess it doesn't matter that oparg can be >= the bit size of oparg
(so large values produce an undefined result) as it's no different
from userspace trying to do the same with large shifts.

> +
> +	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
> +		return -EFAULT;
> +
> +	ret = arch_futex_atomic_op_inuser(op, oparg, &oldval, uaddr);
> +	if (ret)
> +		return ret;
> +
> +	switch (cmp) {
> +	case FUTEX_OP_CMP_EQ:
> +		return oldval == cmparg;
> +	case FUTEX_OP_CMP_NE:
> +		return oldval != cmparg;
> +	case FUTEX_OP_CMP_LT:
> +		return oldval < cmparg;
> +	case FUTEX_OP_CMP_GE:
> +		return oldval >= cmparg;
> +	case FUTEX_OP_CMP_LE:
> +		return oldval <= cmparg;
> +	case FUTEX_OP_CMP_GT:
> +		return oldval > cmparg;
> +	default:
> +		return -ENOSYS;
> +	}
> +}
> +
>  /*
>   * Wake up all waiters hashed on the physical page that is mapped
>   * to this virtual address:

As it's no worse than our existing code, for the above,

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/3] futex: remove duplicated code

[H. Peter Anvin]

On 03/04/17 05:05, Russell King - ARM Linux wrote:
>>  
>> +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".
> 

For readability, perhaps we should make sign- and zero-extension an
explicit facility?

/*
 * Truncate an integer x to n bits, using sign- or
 * zero-extension, respectively.
 */
static inline __const_func__ s32 sex32(s32 x, int n)
{
  return (x << (32-n)) >> (32-n);
}

static inline __const_func__ s64 sex64(s64 x, int n)
{
  return (x << (64-n)) >> (64-n);
}

#define sex(x,y)						\
	((__typeof__(x))					\
	 (((__builtin_constant_p(y) && ((y) <= 32)) ||		\
	   (sizeof(x) <= sizeof(s32)))				\
	  ? sex32((x),(y)) : sex64((x),(y))))

static inline __const_func__ u32 zex32(u32 x, int n)
{
  return (x << (32-n)) >> (32-n);
}

static inline __const_func__ u64 zex64(u64 x, int n)
{
  return (x << (64-n)) >> (64-n);
}

#define zex(x,y)						\
	((__typeof__(x))					\
	 (((__builtin_constant_p(y) && ((y) <= 32)) ||		\
	   (sizeof(x) <= sizeof(u32)))				\
	  ? zex32((x),(y)) : zex64((x),(y))))

[PATCH 1/3] futex: remove duplicated code

[Stafford Horne]

On Sat, Mar 04, 2017 at 11:15:17AM -0800, H. Peter Anvin wrote:
> On 03/04/17 05:05, Russell King - ARM Linux wrote:
> >>  
> >> +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".
> > 
> 
> For readability, perhaps we should make sign- and zero-extension an
> explicit facility?

There is some of this in already here, 32 and 64 bit versions:

  include/linux/bitops.h

Do we really need zero extension? It seems the same.

Example implementation from bitops.h

static inline __s32 sign_extend32(__u32 value, int index)
{
        __u8 shift = 31 - index;
        return (__s32)(value << shift) >> shift;
}

> /*
>  * Truncate an integer x to n bits, using sign- or
>  * zero-extension, respectively.
>  */
> static inline __const_func__ s32 sex32(s32 x, int n)
> {
>   return (x << (32-n)) >> (32-n);
> }
> 
> static inline __const_func__ s64 sex64(s64 x, int n)
> {
>   return (x << (64-n)) >> (64-n);
> }
> 
> #define sex(x,y)						\
> 	((__typeof__(x))					\
> 	 (((__builtin_constant_p(y) && ((y) <= 32)) ||		\
> 	   (sizeof(x) <= sizeof(s32)))				\
> 	  ? sex32((x),(y)) : sex64((x),(y))))
> 
> static inline __const_func__ u32 zex32(u32 x, int n)
> {
>   return (x << (32-n)) >> (32-n);
> }
> 
> static inline __const_func__ u64 zex64(u64 x, int n)
> {
>   return (x << (64-n)) >> (64-n);
> }
> 
> #define zex(x,y)						\
> 	((__typeof__(x))					\
> 	 (((__builtin_constant_p(y) && ((y) <= 32)) ||		\
> 	   (sizeof(x) <= sizeof(u32)))				\
> 	  ? zex32((x),(y)) : zex64((x),(y))))
> 

[PATCH 1/3] futex: remove duplicated code

[Rich Felker]

On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote:
> There is code duplicated over all architecture's headers for
> futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> and comparison of the result.
> 
> Remove this duplication and leave up to the arches only the needed
> assembly which is now in arch_futex_atomic_op_inuser.
> 
> Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> remove pointless access_ok() checks") as access_ok there returns true.
> We introduce it back to the helper for the sake of simplicity (it gets
> optimized away anyway).

Overall I'm in favor of this patch, and it's close to what I had in
mind in the commit message for
00b73d8d1b7131da03aec73011a7286f566fe87f. But I'd actually like to see
it go further. These ops are mainly (only?) used for the (almost never
used) FUTEX_WAKE_OP operation, and there's very little sense in trying
to optimize them with dedicated arch-specific forms like "lock xadd".
Instead the entire logic should be in an arch-generic file, and all
the arch should need to provide is a cmpxchg-on-user-memory primitive
for it to use. On most archs, the same cmpxchg used in kernelspace
should also work for user addresses, meaning a huge amount of
unmaintained, largely untested, junk code can be removed.

Rich