Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751861AbdF2F7H (ORCPT ); Thu, 29 Jun 2017 01:59:07 -0400 Received: from szxga01-in.huawei.com ([45.249.212.187]:9249 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751638AbdF2F67 (ORCPT ); Thu, 29 Jun 2017 01:58:59 -0400 Message-ID: <595496D8.9010208@huawei.com> Date: Thu, 29 Jun 2017 13:57:44 +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> <595461F4.3020300@huawei.com> In-Reply-To: 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.0A020203.59549712.0126,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: b6c8ba6b18d9afc09f18d43088ba3023 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4387 Lines: 137 On 2017/6/29 12:29, hpa@zytor.com wrote: > 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. yes, I test that. Thanks. Thanks zhongjiang