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 <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Vineet Gupta <[email protected]>
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Richard Kuo <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Jonas Bonn <[email protected]>
Cc: Stefan Kristiansson <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Yoshinori Sato <[email protected]>
Cc: Rich Felker <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Chris Metcalf <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
arch/alpha/include/asm/futex.h | 26 ++++---------------
arch/arc/include/asm/futex.h | 40 ++++-------------------------
arch/arm/include/asm/futex.h | 26 +++----------------
arch/arm64/include/asm/futex.h | 26 +++----------------
arch/frv/include/asm/futex.h | 3 ++-
arch/frv/kernel/futex.c | 27 +++-----------------
arch/hexagon/include/asm/futex.h | 38 +++-------------------------
arch/ia64/include/asm/futex.h | 25 +++----------------
arch/microblaze/include/asm/futex.h | 38 +++-------------------------
arch/mips/include/asm/futex.h | 25 +++----------------
arch/openrisc/include/asm/futex.h | 39 +++--------------------------
arch/parisc/include/asm/futex.h | 26 +++----------------
arch/powerpc/include/asm/futex.h | 26 ++++---------------
arch/s390/include/asm/futex.h | 23 ++++-------------
arch/sh/include/asm/futex.h | 26 +++----------------
arch/sparc/include/asm/futex_64.h | 26 ++++---------------
arch/tile/include/asm/futex.h | 40 ++++-------------------------
arch/x86/include/asm/futex.h | 40 ++++-------------------------
arch/xtensa/include/asm/futex.h | 27 ++++----------------
include/asm-generic/futex.h | 50 +++++++------------------------------
kernel/futex.c | 36 ++++++++++++++++++++++++++
21 files changed, 127 insertions(+), 506 deletions(-)
diff --git a/arch/alpha/include/asm/futex.h b/arch/alpha/include/asm/futex.h
index f939794363ac..56474690e685 100644
--- a/arch/alpha/include/asm/futex.h
+++ b/arch/alpha/include/asm/futex.h
@@ -29,18 +29,10 @@
: "r" (uaddr), "r"(oparg) \
: "memory")
-static inline int futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
+static inline int 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -66,17 +58,9 @@ static inline int futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 11e1b1f3acda..eb887dd13e74 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -73,20 +73,11 @@
#endif
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
- return -EFAULT;
-
#ifndef CONFIG_ARC_HAS_LLSC
preempt_disable(); /* to guarantee atomic r-m-w of futex op */
#endif
@@ -118,30 +109,9 @@ static inline int 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/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/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
index 85c4a8981d47..5bb2fd4674e7 100644
--- a/arch/arm64/include/asm/futex.h
+++ b/arch/arm64/include/asm/futex.h
@@ -48,20 +48,10 @@ do { \
} while (0)
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;
-
pagefault_disable();
switch (op) {
@@ -91,17 +81,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/frv/include/asm/futex.h b/arch/frv/include/asm/futex.h
index 2e1da71e27a4..ab346f5f8820 100644
--- a/arch/frv/include/asm/futex.h
+++ b/arch/frv/include/asm/futex.h
@@ -7,7 +7,8 @@
#include <asm/errno.h>
#include <linux/uaccess.h>
-extern int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr);
+extern int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
+ u32 __user *uaddr);
static inline int
futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
diff --git a/arch/frv/kernel/futex.c b/arch/frv/kernel/futex.c
index d155ca9e5098..37f7b2bf7f73 100644
--- a/arch/frv/kernel/futex.c
+++ b/arch/frv/kernel/futex.c
@@ -186,20 +186,10 @@ static inline int atomic_futex_op_xchg_xor(int oparg, u32 __user *uaddr, int *_o
/*
* do the futex operations
*/
-int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+int 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
-
pagefault_disable();
switch (op) {
@@ -225,18 +215,9 @@ int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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; break;
- }
- }
+ if (!ret)
+ *oval = oldval;
return ret;
-} /* end futex_atomic_op_inuser() */
+} /* end arch_futex_atomic_op_inuser() */
diff --git a/arch/hexagon/include/asm/futex.h b/arch/hexagon/include/asm/futex.h
index 7e597f8434da..c607b77c8215 100644
--- a/arch/hexagon/include/asm/futex.h
+++ b/arch/hexagon/include/asm/futex.h
@@ -31,18 +31,9 @@
static inline int
-futex_atomic_op_inuser(int encoded_op, int __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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
- return -EFAULT;
pagefault_disable();
@@ -72,30 +63,9 @@ futex_atomic_op_inuser(int encoded_op, int __user *uaddr)
pagefault_enable();
- 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/arch/ia64/include/asm/futex.h b/arch/ia64/include/asm/futex.h
index 76acbcd5c060..6d67dc1eaf2b 100644
--- a/arch/ia64/include/asm/futex.h
+++ b/arch/ia64/include/asm/futex.h
@@ -45,18 +45,9 @@ do { \
} while (0)
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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (! access_ok (VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -84,17 +75,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/microblaze/include/asm/futex.h b/arch/microblaze/include/asm/futex.h
index 01848f056f43..a9dad9e5e132 100644
--- a/arch/microblaze/include/asm/futex.h
+++ b/arch/microblaze/include/asm/futex.h
@@ -29,18 +29,9 @@
})
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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -66,30 +57,9 @@ futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/mips/include/asm/futex.h b/arch/mips/include/asm/futex.h
index 1de190bdfb9c..a9e61ea54ca9 100644
--- a/arch/mips/include/asm/futex.h
+++ b/arch/mips/include/asm/futex.h
@@ -83,18 +83,9 @@
}
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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (! access_ok (VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -125,17 +116,9 @@ futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/openrisc/include/asm/futex.h b/arch/openrisc/include/asm/futex.h
index 778087341977..8fed278a24b8 100644
--- a/arch/openrisc/include/asm/futex.h
+++ b/arch/openrisc/include/asm/futex.h
@@ -30,20 +30,10 @@
})
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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
-
pagefault_disable();
switch (op) {
@@ -68,30 +58,9 @@ futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
index ac8bd586ace8..06a1a883c72f 100644
--- a/arch/parisc/include/asm/futex.h
+++ b/arch/parisc/include/asm/futex.h
@@ -32,22 +32,12 @@ _futex_spin_unlock_irqrestore(u32 __user *uaddr, unsigned long int *flags)
}
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)
{
unsigned long int flags;
- 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, ret;
u32 tmp;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(*uaddr)))
- return -EFAULT;
-
_futex_spin_lock_irqsave(uaddr, &flags);
pagefault_disable();
@@ -85,17 +75,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
_futex_spin_unlock_irqrestore(uaddr, &flags);
- if (ret == 0) {
- 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/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
index eaada6c92344..719ed9b61ea7 100644
--- a/arch/powerpc/include/asm/futex.h
+++ b/arch/powerpc/include/asm/futex.h
@@ -29,18 +29,10 @@
: "b" (uaddr), "i" (-EFAULT), "r" (oparg) \
: "cr0", "memory")
-static inline int futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
+static inline int 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (! access_ok (VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -66,17 +58,9 @@ static inline int futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/s390/include/asm/futex.h b/arch/s390/include/asm/futex.h
index a4811aa0304d..8f8eec9e1198 100644
--- a/arch/s390/include/asm/futex.h
+++ b/arch/s390/include/asm/futex.h
@@ -21,17 +21,12 @@
: "0" (-EFAULT), "d" (oparg), "a" (uaddr), \
"m" (*uaddr) : "cc");
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int 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, newval, ret;
load_kernel_asce();
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
pagefault_disable();
switch (op) {
@@ -60,17 +55,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
}
pagefault_enable();
- 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/arch/sh/include/asm/futex.h b/arch/sh/include/asm/futex.h
index d0078747d308..8f8cf941a8cd 100644
--- a/arch/sh/include/asm/futex.h
+++ b/arch/sh/include/asm/futex.h
@@ -27,21 +27,12 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
return atomic_futex_op_cmpxchg_inatomic(uval, uaddr, oldval, newval);
}
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval,
+ u32 __user *uaddr)
{
- int op = (encoded_op >> 28) & 7;
- int cmp = (encoded_op >> 24) & 15;
- u32 oparg = (encoded_op << 8) >> 20;
- u32 cmparg = (encoded_op << 20) >> 20;
u32 oldval, newval, prev;
int ret;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
-
pagefault_disable();
do {
@@ -80,17 +71,8 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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 = ((int)oldval < (int)cmparg); break;
- case FUTEX_OP_CMP_GE: ret = ((int)oldval >= (int)cmparg); break;
- case FUTEX_OP_CMP_LE: ret = ((int)oldval <= (int)cmparg); break;
- case FUTEX_OP_CMP_GT: ret = ((int)oldval > (int)cmparg); break;
- default: ret = -ENOSYS;
- }
- }
+ if (!ret)
+ *oval = oldval;
return ret;
}
diff --git a/arch/sparc/include/asm/futex_64.h b/arch/sparc/include/asm/futex_64.h
index 4e899b0dabf7..1cfd89d92208 100644
--- a/arch/sparc/include/asm/futex_64.h
+++ b/arch/sparc/include/asm/futex_64.h
@@ -29,22 +29,14 @@
: "r" (uaddr), "r" (oparg), "i" (-EFAULT) \
: "memory")
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int 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, tem;
- if (unlikely(!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))))
- return -EFAULT;
if (unlikely((((unsigned long) uaddr) & 0x3UL)))
return -EINVAL;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
pagefault_disable();
switch (op) {
@@ -69,17 +61,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/tile/include/asm/futex.h b/arch/tile/include/asm/futex.h
index e64a1b75fc38..83c1e639b411 100644
--- a/arch/tile/include/asm/futex.h
+++ b/arch/tile/include/asm/futex.h
@@ -106,12 +106,9 @@
lock = __atomic_hashed_lock((int __force *)uaddr)
#endif
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int arch_futex_atomic_op_inuser(int op, u32 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 uninitialized_var(val), ret;
__futex_prolog();
@@ -119,12 +116,6 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
/* The 32-bit futex code makes this assumption, so validate it here. */
BUILD_BUG_ON(sizeof(atomic_t) != sizeof(int));
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
-
pagefault_disable();
switch (op) {
case FUTEX_OP_SET:
@@ -148,30 +139,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
}
pagefault_enable();
- if (!ret) {
- switch (cmp) {
- case FUTEX_OP_CMP_EQ:
- ret = (val == cmparg);
- break;
- case FUTEX_OP_CMP_NE:
- ret = (val != cmparg);
- break;
- case FUTEX_OP_CMP_LT:
- ret = (val < cmparg);
- break;
- case FUTEX_OP_CMP_GE:
- ret = (val >= cmparg);
- break;
- case FUTEX_OP_CMP_LE:
- ret = (val <= cmparg);
- break;
- case FUTEX_OP_CMP_GT:
- ret = (val > cmparg);
- break;
- default:
- ret = -ENOSYS;
- }
- }
+ if (!ret)
+ *oval = val;
+
return ret;
}
diff --git a/arch/x86/include/asm/futex.h b/arch/x86/include/asm/futex.h
index b4c1f5453436..f4dc9b63bdda 100644
--- a/arch/x86/include/asm/futex.h
+++ b/arch/x86/include/asm/futex.h
@@ -41,20 +41,11 @@
"+m" (*uaddr), "=&r" (tem) \
: "r" (oparg), "i" (-EFAULT), "1" (0))
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int 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, tem;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
-
pagefault_disable();
switch (op) {
@@ -80,30 +71,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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/arch/xtensa/include/asm/futex.h b/arch/xtensa/include/asm/futex.h
index b39531babec0..eaaf1ebcc7a4 100644
--- a/arch/xtensa/include/asm/futex.h
+++ b/arch/xtensa/include/asm/futex.h
@@ -44,18 +44,10 @@
: "r" (uaddr), "I" (-EFAULT), "r" (oparg) \
: "memory")
-static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+static inline int 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
#if !XCHAL_HAVE_S32C1I
return -ENOSYS;
@@ -89,19 +81,10 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
- if (ret)
- return ret;
+ if (!ret)
+ *oval = oldval;
- 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);
- }
-
- return -ENOSYS;
+ return ret;
}
static inline int
diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
index bf2d34c9d804..f0d8b1c51343 100644
--- a/include/asm-generic/futex.h
+++ b/include/asm-generic/futex.h
@@ -13,7 +13,7 @@
*/
/**
- * futex_atomic_op_inuser() - Atomic arithmetic operation with constant
+ * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
* argument and comparison of the previous
* futex value with another constant.
*
@@ -25,18 +25,11 @@
* <0 - On error
*/
static inline int
-futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
+arch_futex_atomic_op_inuser(int op, u32 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, ret;
u32 tmp;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
preempt_disable();
pagefault_disable();
@@ -74,17 +67,9 @@ futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
pagefault_enable();
preempt_enable();
- if (ret == 0) {
- 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 == 0)
+ *oval = oldval;
+
return ret;
}
@@ -126,18 +111,9 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
#else
static inline int
-futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
+arch_futex_atomic_op_inuser(int op, u32 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;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
-
- if (! access_ok (VERIFY_WRITE, uaddr, sizeof(u32)))
- return -EFAULT;
pagefault_disable();
@@ -153,17 +129,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
pagefault_enable();
- 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;
+ int oldval, ret;
+
+ if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
+ oparg = 1 << oparg;
+
+ 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:
--
2.12.0
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<[email protected]>
Acked-by: Chris Metcalf <[email protected]> [for tile]
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com
encoded_op uses int as type which results in pretty weird behaviour.
E.g. if encoded_op contains oparg 0xfff, it currently results in oparg
being -1.
Switch encoded_op to 'unsigned int' which is correct given it is a bit
mask anyway. And perform upper bound checking on oparg to inform users
about the failure. Finally, avoid int overflows using unsigned shift on
oparg. Note that given we use -fno-strict-overflow, this is not a fix as
there is no problem to fix in the first place.
Signed-off-by: Jiri Slaby <[email protected]>
---
kernel/futex.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index c5ff9850952f..c09424406560 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1457,7 +1457,7 @@ 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)
+static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
{
int op = (encoded_op >> 28) & 7;
int cmp = (encoded_op >> 24) & 15;
@@ -1465,8 +1465,11 @@ static int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
int cmparg = (encoded_op << 20) >> 20;
int oldval, ret;
- if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
- oparg = 1 << oparg;
+ if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
+ if (oparg >= 32)
+ return -EINVAL;
+ oparg = 1U << oparg;
+ }
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
return -EFAULT;
--
2.12.0
Decoding of encoded_op is a bit unreadable. It contains shifts to the
left and to the right by some constants. Make it clearly visible what
part of the bit mask is taken and shift the values only to the right
appropriatelly.
Signed-off-by: Jiri Slaby <[email protected]>
---
kernel/futex.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index c09424406560..5834df248f09 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1459,10 +1459,10 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
static int futex_atomic_op_inuser(unsigned 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;
+ int op = (encoded_op & 0x70000000) >> 28;
+ int cmp = (encoded_op & 0x0f000000) >> 24;
+ int oparg = (encoded_op & 0x00fff000) >> 12;
+ int cmparg = encoded_op & 0x00000fff;
int oldval, ret;
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
--
2.12.0
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
Jiri Slaby <[email protected]> writes:
> 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.
Looks OK and boots on powerpc. But I don't think anything's actually
calling those futex ops. Is there a test suite I should run?
Acked-by: Michael Ellerman <[email protected]> (powerpc)
cheers
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 <[email protected]>
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.
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))))
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))))
>
<[email protected]>,Chris Metcalf <[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
From: [email protected]
Message-ID: <[email protected]>
On March 4, 2017 1:38:05 PM PST, Stafford Horne <[email protected]> wrote:
>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))))
>>
Signed versus unsigned...
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
<[email protected]>,Chris Metcalf <[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
From: [email protected]
Message-ID: <[email protected]>
On March 4, 2017 1:38:05 PM PST, Stafford Horne <[email protected]> wrote:
>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))))
>>
Also, i strongly believe that making it syntactically cumbersome encodes people to open-code it which is bad...
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sat, Mar 04, 2017 at 03:08:50PM -0800, H. Peter Anvin wrote:
> <[email protected]>,Chris Metcalf <[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
> From: [email protected]
> Message-ID: <[email protected]>
>
> On March 4, 2017 1:38:05 PM PST, Stafford Horne <[email protected]> wrote:
> >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))))
> >>
>
> Also, i strongly believe that making it syntactically cumbersome encodes people to open-code it which is bad...
Right, I missed the signed vs unsigned bit.
And it is cumbersome, this would be better
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 03/03/2017, 01:27 PM, Jiri Slaby wrote:
> Decoding of encoded_op is a bit unreadable. It contains shifts to the
> left and to the right by some constants. Make it clearly visible what
> part of the bit mask is taken and shift the values only to the right
> appropriatelly.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> ---
> kernel/futex.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c09424406560..5834df248f09 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1459,10 +1459,10 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
>
> static int futex_atomic_op_inuser(unsigned 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;
> + int op = (encoded_op & 0x70000000) >> 28;
> + int cmp = (encoded_op & 0x0f000000) >> 24;
> + int oparg = (encoded_op & 0x00fff000) >> 12;
> + int cmparg = encoded_op & 0x00000fff;
And it turned out that this one is actually invalid as it doesn't
preserve signedness on oparg and cmdarg. So please avoid applying this one.
thanks,
--
js
suse labs
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
On 03/05/2017, 12:39 AM, Stafford Horne wrote:
> On Sat, Mar 04, 2017 at 03:08:50PM -0800, H. Peter Anvin wrote:
>> <[email protected]>,Chris Metcalf <[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
>> From: [email protected]
>> Message-ID: <[email protected]>
>>
>> On March 4, 2017 1:38:05 PM PST, Stafford Horne <[email protected]> wrote:
>>> 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))))
>>>>
>>
>> Also, i strongly believe that making it syntactically cumbersome encodes people to open-code it which is bad...
>
> Right, I missed the signed vs unsigned bit.
What about this?
commit 811c8c60ea83727e77f92117e3301a4f30a66e8c
Author: Jiri Slaby <[email protected]>
Date: Fri Nov 4 13:38:34 2016 +0100
futex: make the encoded_op decoding readable
Decoding of encoded_op is a bit unreadable. It contains shifts to the
left and to the right by some constants. Make it clearly visible what
part of the bit mask is taken and shift the values only to the right
appropriately. And make sure sign extension takes place using
sign_extend32.
Signed-off-by: Jiri Slaby <[email protected]>
diff --git a/kernel/futex.c b/kernel/futex.c
index 0ead0756a593..f90314bd42cb 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1461,10 +1461,10 @@ futex_wake(u32 __user *uaddr, unsigned int
flags, int nr_wake, u32 bitset)
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;
+ int op = (encoded_op & 0x70000000) >> 28;
+ int cmp = (encoded_op & 0x0f000000) >> 24;
+ int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
+ int cmparg = sign_extend32(encoded_op & 0x00000fff, 12);
int oldval, ret;
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
thanks,
--
js
suse labs
On 03/06/17 00:46, Jiri Slaby 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;
> + int op = (encoded_op & 0x70000000) >> 28;
> + int cmp = (encoded_op & 0x0f000000) >> 24;
> + int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
> + int cmparg = sign_extend32(encoded_op & 0x00000fff, 12);
> int oldval, ret;
>
At least in the sign-extend case the mask is unnecessary.
This is why it would be nice to have both sign and zero extend for symmetry.
-hpa
Hi Jiri,
On Mon, Mar 6, 2017 at 9:46 AM, Jiri Slaby <[email protected]> wrote:
> futex: make the encoded_op decoding readable
>
> Decoding of encoded_op is a bit unreadable. It contains shifts to the
> left and to the right by some constants. Make it clearly visible what
> part of the bit mask is taken and shift the values only to the right
> appropriately. And make sure sign extension takes place using
> sign_extend32.
>
> Signed-off-by: Jiri Slaby <[email protected]>
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 0ead0756a593..f90314bd42cb 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1461,10 +1461,10 @@ futex_wake(u32 __user *uaddr, unsigned int
> flags, int nr_wake, u32 bitset)
>
> static int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
> {
> - int op = (encoded_op >> 28) & 7;
> - int cmp = (encoded_op >> 24) & 15;
At least for the two above (modulo 7 vs 15?), the old decoding code matched
the flow of operation in FUTEX_OP().
> - int oparg = (encoded_op << 8) >> 20;
> - int cmparg = (encoded_op << 20) >> 20;
> + int op = (encoded_op & 0x70000000) >> 28;
> + int cmp = (encoded_op & 0x0f000000) >> 24;
> + int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
> + int cmparg = sign_extend32(encoded_op & 0x00000fff, 12);
> int oldval, ret;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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
<[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
From: [email protected]
Message-ID: <[email protected]>
On March 8, 2017 8:16:49 PM PST, Rob Landley <[email protected]> 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.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Mar 08, 2017 at 08:36:30PM -0800, H. Peter Anvin wrote:
> <[email protected]>,Thomas Gleixner <[email protected]>,Ingo Molnar <[email protected]>,Chris Zankel <[email protected]>,Max Filippov <[email protected]>,Arnd Bergmann <[email protected]>,[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected],[email protected]
> From: [email protected]
> Message-ID: <[email protected]>
>
> On March 8, 2017 8:16:49 PM PST, Rob Landley <[email protected]> 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
On 03/03/2017 04: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).
Acked-by: Vineet Gupta <[email protected]> # Boot tested on ARC
Thx,
-Vineet