Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751669AbdF2CNC (ORCPT ); Wed, 28 Jun 2017 22:13:02 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:9315 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751580AbdF2CM6 (ORCPT ); Wed, 28 Jun 2017 22:12:58 -0400 Message-ID: <595461F4.3020300@huawei.com> Date: Thu, 29 Jun 2017 10:12:04 +0800 From: zhong jiang User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: CC: Ingo Molnar , , , , , , , , Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative 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> In-Reply-To: <568AC6DF-7E6D-4F10-BD41-D43195629C13@zytor.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.29.68] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0B0201.5954621B.005F,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: a1fe1dece7a650512b3b68b6c8204819 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3824 Lines: 122 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