Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751751AbdF2EeK convert rfc822-to-8bit (ORCPT ); Thu, 29 Jun 2017 00:34:10 -0400 Received: from terminus.zytor.com ([65.50.211.136]:53781 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751577AbdF2EeC (ORCPT ); Thu, 29 Jun 2017 00:34:02 -0400 Date: Wed, 28 Jun 2017 21:29:17 -0700 User-Agent: K-9 Mail for Android In-Reply-To: <595461F4.3020300@huawei.com> References: <1498045437-7675-1-git-send-email-zhongjiang@huawei.com> <20170621164036.4findvvz7jj4cvqo@gmail.com> <595331FE.3090700@huawei.com> <568AC6DF-7E6D-4F10-BD41-D43195629C13@zytor.com> <595461F4.3020300@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative To: zhong jiang CC: Ingo Molnar , akpm@linux-foundation.org, tglx@linutronix.de, mingo@redhat.com, minchan@kernel.org, mhocko@suse.com, x86@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org From: hpa@zytor.com Message-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4222 Lines: 135 On June 28, 2017 7:12:04 PM PDT, zhong jiang wrote: >On 2017/6/29 5:43, hpa@zytor.com wrote: >> On June 27, 2017 9:35:10 PM PDT, zhong jiang >wrote: >>> Hi, Ingo >>> >>> Thank you for the comment. >>> On 2017/6/22 0:40, Ingo Molnar wrote: >>>> * zhong jiang wrote: >>>> >>>>> when shift expoment is negative, left shift alway zero. therefore, >>> we >>>>> modify the logic to avoid the warining. >>>>> >>>>> Signed-off-by: zhong jiang >>>>> --- >>>>> arch/x86/include/asm/futex.h | 8 ++++++-- >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/x86/include/asm/futex.h >>> b/arch/x86/include/asm/futex.h >>>>> index b4c1f54..2425fca 100644 >>>>> --- a/arch/x86/include/asm/futex.h >>>>> +++ b/arch/x86/include/asm/futex.h >>>>> @@ -49,8 +49,12 @@ static inline int futex_atomic_op_inuser(int >>> encoded_op, u32 __user *uaddr) >>>>> int cmparg = (encoded_op << 20) >> 20; >>>>> int oldval = 0, ret, tem; >>>>> >>>>> - if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) >>>>> - oparg = 1 << oparg; >>>>> + if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) { >>>>> + if (oparg >= 0) >>>>> + oparg = 1 << oparg; >>>>> + else >>>>> + oparg = 0; >>>>> + } >>>> Could we avoid all these complications by using an unsigned type? >>> I think it is not feasible. a negative shift exponent is likely >>> existence and reasonable. >>> as the above case, oparg is a negative is common. >>> >>> I think it can be avoided by following change. >>> >>> diff --git a/arch/x86/include/asm/futex.h >>> b/arch/x86/include/asm/futex.h >>> index b4c1f54..3205e86 100644 >>> --- a/arch/x86/include/asm/futex.h >>> +++ b/arch/x86/include/asm/futex.h >>> @@ -50,7 +50,7 @@ static inline int futex_atomic_op_inuser(int >>> encoded_op, u32 __user *uaddr) >>> int oldval = 0, ret, tem; >>> >>> if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) >>> - oparg = 1 << oparg; >>> + oparg = safe_shift(1, oparg); >>> >>> if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) >>> return -EFAULT; >>> diff --git a/drivers/video/fbdev/core/fbmem.c >>> b/drivers/video/fbdev/core/fbmem.c >>> index 069fe79..b4edda3 100644 >>> --- a/drivers/video/fbdev/core/fbmem.c >>> +++ b/drivers/video/fbdev/core/fbmem.c >>> @@ -190,11 +190,6 @@ char* fb_get_buffer_offset(struct fb_info >*info, >>> struct fb_pixmap *buf, u32 size >>> >>> #ifdef CONFIG_LOGO >>> >>> -static inline unsigned safe_shift(unsigned d, int n) >>> -{ >>> - return n < 0 ? d >> -n : d << n; >>> -} >>> - >>> static void fb_set_logocmap(struct fb_info *info, >>> const struct linux_logo *logo) >>> { >>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >>> index d043ada..f3b8856 100644 >>> --- a/include/linux/kernel.h >>> +++ b/include/linux/kernel.h >>> @@ -841,6 +841,10 @@ static inline void ftrace_dump(enum >>> ftrace_dump_mode oops_dump_mode) { } >>> */ >>> #define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi) >>> >>> +static inline unsigned safe_shift(unsigned d, int n) >>> +{ >>> + return n < 0 ? d >> -n : d << n; >>> +} >>> >>> Thansk >>> zhongjiang >>> >>>> Thanks, >>>> >>>> Ingo >>>> >>>> . >>>> >> What makes it reasonable? It is totally ill-defined and doesn't do >anything useful now? > Thanks you for comments. > >Maybe I mismake the meaning. I test the negative cases in x86 , all >case is zero. so I come to a conclusion. > >zj.c:15:8: warning: left shift count is negative >[-Wshift-count-negative] > j = 1 << -2048; > ^ >[root@localhost zhongjiang]# ./zj >j = 0 >j.c:15:8: warning: left shift count is negative >[-Wshift-count-negative] > j = 1 << -2047; > ^ >[root@localhost zhongjiang]# ./zj >j = 0 > >I insmod a module into kernel to test the testcasts, all of the result >is zero. > >I wonder whether I miss some point or not. Do you point out to me? >please > >Thanks >zhongjiang > > When you use compile-time constants, the compiler generates the value at compile time, which can be totally different. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.