From: hpa@zytor.com
To: zhong jiang <zhongjiang@huawei.com>
Cc: Ingo Molnar <mingo@kernel.org>,
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
Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative
Date: Wed, 28 Jun 2017 21:29:17 -0700 [thread overview]
Message-ID: <ECBE6E45-EED4-4BEA-B758-D7217E10E1FD@zytor.com> (raw)
In-Reply-To: <595461F4.3020300@huawei.com>
On June 28, 2017 7:12:04 PM PDT, zhong jiang <zhongjiang@huawei.com> wrote:
>On 2017/6/29 5:43, hpa@zytor.com wrote:
>> On June 27, 2017 9:35:10 PM PDT, zhong jiang <zhongjiang@huawei.com>
>wrote:
>>> 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
>>>>
>>>> .
>>>>
>> 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.
next prev parent reply other threads:[~2017-06-29 4:34 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1498045437-7675-1-git-send-email-zhongjiang@huawei.com>
2017-06-21 16:40 ` [PATCH] futex: avoid undefined behaviour when shift exponent is negative Ingo Molnar
2017-06-28 4:35 ` zhong jiang
2017-06-28 21:43 ` hpa
2017-06-29 2:12 ` zhong jiang
2017-06-29 4:29 ` hpa [this message]
2017-06-29 5:57 ` zhong jiang
2017-06-28 22:13 ` Thomas Gleixner
2017-06-29 1:54 ` zhong jiang
2017-06-29 6:33 ` Thomas Gleixner
2017-06-29 7:04 ` zhong jiang
2017-08-25 5:21 ` zhong jiang
2017-08-25 21:13 ` Thomas Gleixner
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=ECBE6E45-EED4-4BEA-B758-D7217E10E1FD@zytor.com \
--to=hpa@zytor.com \
--cc=akpm@linux-foundation.org \
--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 \
--cc=zhongjiang@huawei.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).