From: zhong jiang <zhongjiang@huawei.com> To: Ingo Molnar <mingo@kernel.org> Cc: <akpm@linux-foundation.org>, <tglx@linutronix.de>, <mingo@redhat.com>, <minchan@kernel.org>, <mhocko@suse.com>, <hpa@zytor.com>, <x86@kernel.org>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, zhongjiang <zhongjiang@huawei.com> Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative Date: Wed, 28 Jun 2017 12:35:10 +0800 [thread overview] Message-ID: <595331FE.3090700@huawei.com> (raw) In-Reply-To: <20170621164036.4findvvz7jj4cvqo@gmail.com> Hi, Ingo Thank you for the comment. On 2017/6/22 0:40, Ingo Molnar wrote: > * zhong jiang <zhongjiang@huawei.com> wrote: > >> when shift expoment is negative, left shift alway zero. therefore, we >> modify the logic to avoid the warining. >> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com> >> --- >> 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 > > . >
WARNING: multiple messages have this Message-ID (diff)
From: zhong jiang <zhongjiang@huawei.com> To: Ingo Molnar <mingo@kernel.org> Cc: akpm@linux-foundation.org, tglx@linutronix.de, mingo@redhat.com, minchan@kernel.org, mhocko@suse.com, hpa@zytor.com, x86@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, zhongjiang <zhongjiang@huawei.com> Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative Date: Wed, 28 Jun 2017 12:35:10 +0800 [thread overview] Message-ID: <595331FE.3090700@huawei.com> (raw) In-Reply-To: <20170621164036.4findvvz7jj4cvqo@gmail.com> Hi, Ingo Thank you for the comment. On 2017/6/22 0:40, Ingo Molnar wrote: > * zhong jiang <zhongjiang@huawei.com> wrote: > >> when shift expoment is negative, left shift alway zero. therefore, we >> modify the logic to avoid the warining. >> >> Signed-off-by: zhong jiang <zhongjiang@huawei.com> >> --- >> 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 > > . > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-06-28 4:36 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-21 11:43 [PATCH] futex: avoid undefined behaviour when shift exponent is negative zhong jiang 2017-06-21 16:40 ` Ingo Molnar 2017-06-21 16:40 ` Ingo Molnar 2017-06-28 4:35 ` zhong jiang [this message] 2017-06-28 4:35 ` zhong jiang 2017-06-28 21:43 ` hpa 2017-06-28 21:43 ` hpa 2017-06-29 2:12 ` zhong jiang 2017-06-29 2:12 ` zhong jiang 2017-06-29 4:29 ` hpa 2017-06-29 4:29 ` hpa 2017-06-29 5:57 ` zhong jiang 2017-06-29 5:57 ` zhong jiang 2017-06-28 22:13 ` Thomas Gleixner 2017-06-28 22:13 ` Thomas Gleixner 2017-06-29 1:54 ` zhong jiang 2017-06-29 1:54 ` zhong jiang 2017-06-29 6:33 ` Thomas Gleixner 2017-06-29 6:33 ` Thomas Gleixner 2017-06-29 7:04 ` zhong jiang 2017-06-29 7:04 ` zhong jiang 2017-08-25 5:21 ` zhong jiang 2017-08-25 5:21 ` zhong jiang 2017-08-25 21:13 ` Thomas Gleixner 2017-08-25 21:13 ` Thomas Gleixner 2017-08-26 2:51 ` zhong jiang 2017-08-26 2:51 ` zhong jiang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=595331FE.3090700@huawei.com \ --to=zhongjiang@huawei.com \ --cc=akpm@linux-foundation.org \ --cc=hpa@zytor.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ --cc=minchan@kernel.org \ --cc=mingo@kernel.org \ --cc=mingo@redhat.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.