From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Date: Sat, 04 Mar 2017 13:05:50 +0000 Subject: Re: [PATCH 1/3] futex: remove duplicated code Message-Id: <20170304130550.GT21222@n2100.armlinux.org.uk> List-Id: References: <20170303122712.13353-1-jslaby@suse.cz> In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jiri Slaby Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Catalin Marinas , Will Deacon , Richard Kuo , Tony Luck , Fenghua Yu , Michal Simek , Ralf Baechle , Jonas Bonn , Stefan Kristiansson , Stafford Horne , "James E.J. Bottomley" , Helge Deller , Benjamin Herrenschmidt , Paul Mackerras 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH 1/3] futex: remove duplicated code Date: Sat, 4 Mar 2017 13:05:50 +0000 Message-ID: <20170304130550.GT21222@n2100.armlinux.org.uk> References: <20170303122712.13353-1-jslaby@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Catalin Marinas , Will Deacon , Richard Kuo , Tony Luck , Fenghua Yu , Michal Simek , Ralf Baechle , Jonas Bonn , Stefan Kristiansson , Stafford Horne , "James E.J. Bottomley" , Helge Deller , Benjamin Herrenschmidt , Paul Mackerras , To: Jiri Slaby Return-path: In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips List-subscribe: List-owner: List-post: List-archive: 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752302AbdCDNOg (ORCPT ); Sat, 4 Mar 2017 08:14:36 -0500 Received: from pandora.armlinux.org.uk ([78.32.30.218]:50552 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751989AbdCDNOb (ORCPT ); Sat, 4 Mar 2017 08:14:31 -0500 Date: Sat, 4 Mar 2017 13:05:50 +0000 From: Russell King - ARM Linux To: Jiri Slaby Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Catalin Marinas , Will Deacon , Richard Kuo , Tony Luck , Fenghua Yu , Michal Simek , Ralf Baechle , Jonas Bonn , Stefan Kristiansson , Stafford Horne , "James E.J. Bottomley" , Helge Deller , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Martin Schwidefsky , Heiko Carstens , Yoshinori Sato , Rich Felker , "David S. Miller" , Chris Metcalf , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Chris Zankel , Max Filippov , Arnd Bergmann , x86@kernel.org, linux-alpha@vger.kernel.org, linux-snps-arc@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org, linux-mips@linux-mips.org, openrisc@lists.librecores.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, linux-xtensa@linux-xtensa.org, linux-arch@vger.kernel.org Subject: Re: [PATCH 1/3] futex: remove duplicated code Message-ID: <20170304130550.GT21222@n2100.armlinux.org.uk> References: <20170303122712.13353-1-jslaby@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH 1/3] futex: remove duplicated code Date: Sat, 4 Mar 2017 13:05:50 +0000 Message-ID: <20170304130550.GT21222@n2100.armlinux.org.uk> References: <20170303122712.13353-1-jslaby@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Jiri Slaby Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Catalin Marinas , Will Deacon , Richard Kuo , Tony Luck , Fenghua Yu , Michal Simek , Ralf Baechle , Jonas Bonn , Stefan Kristiansson , Stafford Horne , "James E.J. Bottomley" , Helge Deller , Benjamin Herrenschmidt , Paul Mackerras List-Id: linux-arch.vger.kernel.org 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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@armlinux.org.uk (Russell King - ARM Linux) Date: Sat, 4 Mar 2017 13:05:50 +0000 Subject: [PATCH 1/3] futex: remove duplicated code In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> References: <20170303122712.13353-1-jslaby@suse.cz> List-ID: Message-ID: <20170304130550.GT21222@n2100.armlinux.org.uk> To: linux-snps-arc@lists.infradead.org On Fri, Mar 03, 2017@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 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@armlinux.org.uk (Russell King - ARM Linux) Date: Sat, 4 Mar 2017 13:05:50 +0000 Subject: [PATCH 1/3] futex: remove duplicated code In-Reply-To: <20170303122712.13353-1-jslaby@suse.cz> References: <20170303122712.13353-1-jslaby@suse.cz> Message-ID: <20170304130550.GT21222@n2100.armlinux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 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.