Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671797 > unrolled thread

Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] futex: avoid undefined behaviour when shift exponent is  negative Ingo Molnar <mingo@kernel.org> - 2017-06-21 18:50 +0200
    Re: [PATCH] futex: avoid undefined behaviour when shift exponent  is negative zhong jiang <zhongjiang@huawei.com> - 2017-06-28 06:40 +0200
      Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative hpa@zytor.com - 2017-06-28 23:50 +0200
        Re: [PATCH] futex: avoid undefined behaviour when shift exponent  is negative zhong jiang <zhongjiang@huawei.com> - 2017-06-29 04:20 +0200
          Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative hpa@zytor.com - 2017-06-29 06:40 +0200
            Re: [PATCH] futex: avoid undefined behaviour when shift exponent  is negative zhong jiang <zhongjiang@huawei.com> - 2017-06-29 08:10 +0200
      Re: [PATCH] futex: avoid undefined behaviour when shift exponent is  negative Thomas Gleixner <tglx@linutronix.de> - 2017-06-29 00:20 +0200
        Re: [PATCH] futex: avoid undefined behaviour when shift exponent  is negative zhong jiang <zhongjiang@huawei.com> - 2017-06-29 04:00 +0200
          Re: [PATCH] futex: avoid undefined behaviour when shift exponent is  negative Thomas Gleixner <tglx@linutronix.de> - 2017-06-29 08:40 +0200
            Re: [PATCH] futex: avoid undefined behaviour when shift exponent  is negative zhong jiang <zhongjiang@huawei.com> - 2017-06-29 09:10 +0200

#1671797 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

* 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?

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1676366 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

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
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1677207 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

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?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1677321 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

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
 
 

[toc] | [prev] | [next] | [standalone]

#1677376 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

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.

[toc] | [prev] | [next] | [standalone]

#1677421 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

On 2017/6/29 12:29, hpa@zytor.com wrote:
> 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.
 yes, I test that. Thanks.

 Thanks
 zhongjiang

[toc] | [prev] | [next] | [standalone]

#1677231

On Wed, 28 Jun 2017, zhong jiang wrote:
> 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.

What is reasonable about a negative shift value?

> as the above case, oparg is a negative is common.

That's simply wrong. If oparg is negative and the SHIFT bit is set then the
result is undefined today and there is no way that this can be used at
all.

On x86:

   1 << -1	= 0x80000000
   1 << -2048	= 0x00000001
   1 << -2047	= 0x00000002

Anything using a shift value < 0 or > 31 will get crap as a
result. Rightfully so because it's just undefined.

Yes I know that the insanity of user space is unlimited, but anything
attempting this is so broken that we cannot break it further by making that
shift arg unsigned and actually limit it to 0-31

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1677315 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

Hi, Thomas

Thank you for clarification.
On 2017/6/29 6:13, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, zhong jiang wrote:
>> 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.
> What is reasonable about a negative shift value?
>
>> as the above case, oparg is a negative is common.
> That's simply wrong. If oparg is negative and the SHIFT bit is set then the
> result is undefined today and there is no way that this can be used at
> all.
>
> On x86:
>
>    1 << -1	= 0x80000000
>    1 << -2048	= 0x00000001
>    1 << -2047	= 0x00000002
  but I test the cases in x86_64 all is zero.   I wonder whether it is related to gcc or not

  zj.c:15:8: warning: left shift count is negative [-Wshift-count-negative]
  j = 1 << -2048;
        ^
[root@localhost zhongjiang]# ./zj
j = 0

 Thanks
 zhongjiang
> Anything using a shift value < 0 or > 31 will get crap as a
> result. Rightfully so because it's just undefined.
>
> Yes I know that the insanity of user space is unlimited, but anything
> attempting this is so broken that we cannot break it further by making that
> shift arg unsigned and actually limit it to 0-31
> Thanks,
>
> 	tglx
>
>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1677438

On Thu, 29 Jun 2017, zhong jiang wrote:
> On 2017/6/29 6:13, Thomas Gleixner wrote:
> > That's simply wrong. If oparg is negative and the SHIFT bit is set then the
> > result is undefined today and there is no way that this can be used at
> > all.
> >
> > On x86:
> >
> >    1 << -1	= 0x80000000
> >    1 << -2048	= 0x00000001
> >    1 << -2047	= 0x00000002
>   but I test the cases in x86_64 all is zero.   I wonder whether it is related to gcc or not
> 
>   zj.c:15:8: warning: left shift count is negative [-Wshift-count-negative]
>   j = 1 << -2048;
>         ^
> [root@localhost zhongjiang]# ./zj
> j = 0

Which is not a surprise because the compiler can detect it as the shift is
a constant. oparg is not so constant ...

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1677450 — Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

On 2017/6/29 14:33, Thomas Gleixner wrote:
> On Thu, 29 Jun 2017, zhong jiang wrote:
>> On 2017/6/29 6:13, Thomas Gleixner wrote:
>>> That's simply wrong. If oparg is negative and the SHIFT bit is set then the
>>> result is undefined today and there is no way that this can be used at
>>> all.
>>>
>>> On x86:
>>>
>>>    1 << -1	= 0x80000000
>>>    1 << -2048	= 0x00000001
>>>    1 << -2047	= 0x00000002
>>   but I test the cases in x86_64 all is zero.   I wonder whether it is related to gcc or not
>>
>>   zj.c:15:8: warning: left shift count is negative [-Wshift-count-negative]
>>   j = 1 << -2048;
>>         ^
>> [root@localhost zhongjiang]# ./zj
>> j = 0
> Which is not a surprise because the compiler can detect it as the shift is
> a constant. oparg is not so constant ...
  I get it. Thanks
 
  Thanks
  zhongjiang
> Thanks,
>
> 	tglx
>
> .
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web