| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7f81c053-ba60-a4d2-23d6-d0f032acbcff@189.cn>
Date: Mon, 22 May 2023 19:55:45 +0800
From: Sui Jingfeng <15330273260@....cn>
To: Jani Nikula <jani.nikula@...ux.intel.com>,
Sui Jingfeng <15330273260@....cn>,
David Laight <David.Laight@...LAB.COM>,
Li Yi <liyi@...ngson.cn>
Cc: Thomas Zimmermann <tzimmermann@...e.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"loongson-kernel@...ts.loongnix.cn"
<loongson-kernel@...ts.loongnix.cn>
Subject: Re: [PATCH] drm/drm_vblank.c: avoid unsigned int to signed int cast
Hi,
On 2023/5/22 19:29, Jani Nikula wrote:
> On Thu, 18 May 2023, Sui Jingfeng <15330273260@....cn> wrote:
>> On 2023/5/17 18:59, David Laight wrote:
>>> From: 15330273260@....cn
>>>> Sent: 16 May 2023 18:30
>>>>
>>>> From: Sui Jingfeng <suijingfeng@...ngson.cn>
>>>>
>>>> Both mode->crtc_htotal and mode->crtc_vtotal are u16 type,
>>>> mode->crtc_htotal * mode->crtc_vtotal will results a unsigned type.
>>> Nope, u16 gets promoted to 'signed int' and the result of the
>>> multiply is also signed.
>> I believe that signed or unsigned is dependent on the declaration.
>>
>> I am talk about the math, while you are talking about compiler.
>>
>> I admit that u16 gets promoted to 'signed int' is true, but this is
>> irrelevant,
>>
>> the point is how to understand the returned value.
>>
>>
>> How does the compiler generate the code is one thing, how do we
>> interpret the result is another
>>
>> How does the compiler generate the code is NOT determined by us, while
>> how do we interpret the result is determined by us.
>>
>>
>> I believe that using a u32 type to interpret the result(u16 * u16) is
>> always true, it is true in the perspective of *math*.
>>
>> Integer promotions is the details of C program language. If the result
>> of the multiply is signed, then there are risks that
>>
>> the result is negative, what's the benefit to present this risk to the
>> programmer?
>>
>> What's the benefit to tell me(and others) that u16 * u16 yield a signed
>> value? and can be negative?
>>
>> Using int type as the return type bring concerns to the programmer and
>> the user of the function,
>>
>> even though this is not impossible in practice.
> In general, do not use unsigned types in arithmethic to avoid negative
> values, because most people will be tripped over by integer promotion
> rules, and you'll get negative values anyway.
>
> I'll bet most people will be surprised to see what this prints:
>
> #include <stdio.h>
> #include <stdint.h>
>
> int main(void)
> {
> uint16_t x = 0xffff;
> uint16_t y = 0xffff;
> uint64_t z = x * y;
>
> printf("0x%016lx\n", z);
> printf("%ld\n", z);
Here, please replace the "%ld\n" with the "%lu\n", then you will see the
difference.
you are casting the variable 'z' to signed value, "%d" is for printing
signed value, and "%u" is for printing unsigned value.
Your simple code explained exactly why you are still in confusion,
that is u16 * u16 can yield a negative value if you use the int as the
return type. Because it overflowed.
> printf("%d\n", x * y);
> }
>
> And it's not that different from what you have below. Your patch doesn't
> change anything, and doesn't make it any less confusing.
>
> BR,
> Jani.
>
>
>>>> Using a u32 is enough to store the result, but considering that the
>>>> result will be casted to u64 soon after. We use a u64 type directly.
>>>> So there no need to cast it to signed type and cast back then.
>>> ....
>>>> - int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>>>> + u64 frame_size = mode->crtc_htotal * mode->crtc_vtotal;
>>> ...
>>>> - framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
>>>> + framedur_ns = div_u64(frame_size * 1000000, dotclock);
>>> The (u64) cast is there to extend the value to 64bits, not
>>> because the original type is signed.
>> Sorry about my expression, I think my sentence did not mention anything
>> about 'because the original type is signed'.
>>
>> In the contrary, my patch eliminated the concerns to the reviewer. It
>> say that the results of the multiply can't be negative.
>>
>> My intent is to tell the compiler we want a unsigned return type, but
>> GCC emit 'imul' instruction for the multiply......
>>
>> I'm using u64 as the return type, because div_u64() function accept a
>> u64 type value as its first argument.
>>
>>> The compiler will detect that the old code is a 32x32 multiply
>>> where a 64bit result is needed, that may not be true for the
>>> changed code (it would need to track back as far as the u16s).
>> I don't believe my code could be wrong.
>>
>> when you use the word 'may', you are saying that it could be wrong after
>> apply my patch.
>>
>> Then you have to find at least one test example to prove you point, in
>> which case my codes generate wrong results.
>>
>> Again I don't believe you could find one.
>>
>>> It is not uncommon to force a 64bit result from a multiply
>>> by making the constant 64bit. As in:
>>> div_u64(frame_size * 1000000ULL, dotclock);
>> In fact, After apply this patch, the ASM code generated is same with before.
>>
>> This may because the GCC is smart enough to generate optimized code in
>> either case,
>>
>> I think It could be different with a different optimization-level.
>>
>> I have tested this patch on three different architecture, I can not
>> find error still.
>>
>> Below is the assembly extract on x86-64: because GCC generate the same
>> code in either case,
>>
>> so I pasted only one copy here.
>>
>>
>> 0000000000000530 <drm_calc_timestamping_constants>:
>> 530: f3 0f 1e fa endbr64
>> 534: e8 00 00 00 00 callq 539
>> <drm_calc_timestamping_constants+0x9>
>> 539: 55 push %rbp
>> 53a: 48 89 e5 mov %rsp,%rbp
>> 53d: 41 57 push %r15
>> 53f: 41 56 push %r14
>> 541: 41 55 push %r13
>> 543: 41 54 push %r12
>> 545: 53 push %rbx
>> 546: 48 83 ec 18 sub $0x18,%rsp
>> 54a: 4c 8b 3f mov (%rdi),%r15
>> 54d: 41 8b 87 6c 01 00 00 mov 0x16c(%r15),%eax
>> 554: 85 c0 test %eax,%eax
>> 556: 0f 84 ec 00 00 00 je 648
>> <drm_calc_timestamping_constants+0x118>
>> 55c: 44 8b 87 90 00 00 00 mov 0x90(%rdi),%r8d
>> 563: 49 89 fc mov %rdi,%r12
>> 566: 44 39 c0 cmp %r8d,%eax
>> 569: 0f 86 40 01 00 00 jbe 6af
>> <drm_calc_timestamping_constants+0x17f>
>> 56f: 44 8b 76 1c mov 0x1c(%rsi),%r14d
>> 573: 49 8b 8f 40 01 00 00 mov 0x140(%r15),%rcx
>> 57a: 48 89 f3 mov %rsi,%rbx
>> 57d: 45 85 f6 test %r14d,%r14d
>> 580: 0f 8e d5 00 00 00 jle 65b
>> <drm_calc_timestamping_constants+0x12b>
>> 586: 0f b7 43 2a movzwl 0x2a(%rbx),%eax
>> 58a: 49 63 f6 movslq %r14d,%rsi
>> 58d: 31 d2 xor %edx,%edx
>> 58f: 48 89 c7 mov %rax,%rdi
>> 592: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax
>> 599: 48 f7 f6 div %rsi
>> 59c: 31 d2 xor %edx,%edx
>> 59e: 48 89 45 d0 mov %rax,-0x30(%rbp)
>> 5a2: 0f b7 43 38 movzwl 0x38(%rbx),%eax
>> 5a6: 0f af c7 imul %edi,%eax
>> 5a9: 48 98 cltq
>> 5ab: 48 69 c0 40 42 0f 00 imul $0xf4240,%rax,%rax
>> 5b2: 48 f7 f6 div %rsi
>> 5b5: 41 89 c5 mov %eax,%r13d
>> 5b8: f6 43 18 10 testb $0x10,0x18(%rbx)
>> 5bc: 74 0a je 5c8
>> <drm_calc_timestamping_constants+0x98>
>> 5be: 41 c1 ed 1f shr $0x1f,%r13d
>> 5c2: 41 01 c5 add %eax,%r13d
>> 5c5: 41 d1 fd sar %r13d
>> 5c8: 4b 8d 04 c0 lea (%r8,%r8,8),%rax
>> 5cc: 48 89 de mov %rbx,%rsi
>> 5cf: 49 8d 3c 40 lea (%r8,%rax,2),%rdi
>> 5d3: 8b 45 d0 mov -0x30(%rbp),%eax
>> 5d6: 48 c1 e7 04 shl $0x4,%rdi
>> 5da: 48 01 cf add %rcx,%rdi
>> 5dd: 89 47 78 mov %eax,0x78(%rdi)
>> 5e0: 48 83 ef 80 sub $0xffffffffffffff80,%rdi
>> 5e4: 44 89 6f f4 mov %r13d,-0xc(%rdi)
>> 5e8: e8 00 00 00 00 callq 5ed
>> <drm_calc_timestamping_constants+0xbd>
>> 5ed: 0f b7 53 2e movzwl 0x2e(%rbx),%edx
>> 5f1: 0f b7 43 38 movzwl 0x38(%rbx),%eax
>> 5f5: 44 0f b7 4b 2a movzwl 0x2a(%rbx),%r9d
>> 5fa: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>> 5ff: 4d 85 ff test %r15,%r15
>> 602: 0f 84 87 00 00 00 je 68f
>> <drm_calc_timestamping_constants+0x15f>
>> 608: 49 8b 77 08 mov 0x8(%r15),%rsi
>> 60c: 52 push %rdx
>> 60d: 31 ff xor %edi,%edi
>> 60f: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>> 616: 50 push %rax
>> 617: 31 d2 xor %edx,%edx
>> 619: e8 00 00 00 00 callq 61e
>> <drm_calc_timestamping_constants+0xee>
>> 61e: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>> 623: 4d 8b 7f 08 mov 0x8(%r15),%r15
>> 627: 5f pop %rdi
>> 628: 41 59 pop %r9
>> 62a: 8b 45 d0 mov -0x30(%rbp),%eax
>> 62d: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>> 634: 4c 89 fe mov %r15,%rsi
>> 637: 45 89 f1 mov %r14d,%r9d
>> 63a: 31 d2 xor %edx,%edx
>> 63c: 31 ff xor %edi,%edi
>> 63e: 50 push %rax
>> 63f: 41 55 push %r13
>> 641: e8 00 00 00 00 callq 646
>> <drm_calc_timestamping_constants+0x116>
>> 646: 59 pop %rcx
>> 647: 5e pop %rsi
>> 648: 48 8d 65 d8 lea -0x28(%rbp),%rsp
>> 64c: 5b pop %rbx
>> 64d: 41 5c pop %r12
>> 64f: 41 5d pop %r13
>> 651: 41 5e pop %r14
>> 653: 41 5f pop %r15
>> 655: 5d pop %rbp
>> 656: e9 00 00 00 00 jmpq 65b
>> <drm_calc_timestamping_constants+0x12b>
>> 65b: 41 8b 54 24 60 mov 0x60(%r12),%edx
>> 660: 49 8b 7f 08 mov 0x8(%r15),%rdi
>> 664: 44 89 45 c4 mov %r8d,-0x3c(%rbp)
>> 668: 45 31 ed xor %r13d,%r13d
>> 66b: 48 c7 c6 00 00 00 00 mov $0x0,%rsi
>> 672: 48 89 4d c8 mov %rcx,-0x38(%rbp)
>> 676: e8 00 00 00 00 callq 67b
>> <drm_calc_timestamping_constants+0x14b>
>> 67b: c7 45 d0 00 00 00 00 movl $0x0,-0x30(%rbp)
>> 682: 44 8b 45 c4 mov -0x3c(%rbp),%r8d
>> 686: 48 8b 4d c8 mov -0x38(%rbp),%rcx
>> 68a: e9 39 ff ff ff jmpq 5c8
>> <drm_calc_timestamping_constants+0x98>
>> 68f: 52 push %rdx
>> 690: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>> 697: 31 d2 xor %edx,%edx
>> 699: 31 f6 xor %esi,%esi
>> 69b: 50 push %rax
>> 69c: 31 ff xor %edi,%edi
>> 69e: e8 00 00 00 00 callq 6a3
>> <drm_calc_timestamping_constants+0x173>
>> 6a3: 45 8b 44 24 60 mov 0x60(%r12),%r8d
>> 6a8: 58 pop %rax
>> 6a9: 5a pop %rdx
>> 6aa: e9 7b ff ff ff jmpq 62a
>> <drm_calc_timestamping_constants+0xfa>
>> 6af: 49 8b 7f 08 mov 0x8(%r15),%rdi
>> 6b3: 4c 8b 67 50 mov 0x50(%rdi),%r12
>> 6b7: 4d 85 e4 test %r12,%r12
>> 6ba: 74 25 je 6e1
>> <drm_calc_timestamping_constants+0x1b1>
>> 6bc: e8 00 00 00 00 callq 6c1
>> <drm_calc_timestamping_constants+0x191>
>> 6c1: 48 c7 c1 00 00 00 00 mov $0x0,%rcx
>> 6c8: 4c 89 e2 mov %r12,%rdx
>> 6cb: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
>> 6d2: 48 89 c6 mov %rax,%rsi
>> 6d5: e8 00 00 00 00 callq 6da
>> <drm_calc_timestamping_constants+0x1aa>
>> 6da: 0f 0b ud2
>> 6dc: e9 67 ff ff ff jmpq 648
>> <drm_calc_timestamping_constants+0x118>
>> 6e1: 4c 8b 27 mov (%rdi),%r12
>> 6e4: eb d6 jmp 6bc
>> <drm_calc_timestamping_constants+0x18c>
>> 6e6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
>> 6ed: 00 00 00
>> 6f0: 90 nop
>> 6f1: 90 nop
>> 6f2: 90 nop
>> 6f3: 90 nop
>> 6f4: 90 nop
>> 6f5: 90 nop
>> 6f6: 90 nop
>> 6f7: 90 nop
>> 6f8: 90 nop
>> 6f9: 90 nop
>> 6fa: 90 nop
>> 6fb: 90 nop
>> 6fc: 90 nop
>> 6fd: 90 nop
>> 6fe: 90 nop
>> 6ff: 90 nop
>>
>>
>>> David
>>>
>>> -
>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>>> Registration No: 1397386 (Wales)
>>>
Powered by blists - more mailing lists