* [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
@ 2023-11-17 18:36 Gustavo A. R. Silva
2023-11-20 14:25 ` Naveen N Rao
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-17 18:36 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Christophe Leroy; +Cc: PowerPC
Hi all,
I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
to get your feedback on this, please:
In function 'do_byte_reverse',
inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~~~~~^~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
289 | up[2] = byterev_8(up[1]);
| ~~~~~~^~~~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
287 | up[3] = tmp;
| ~~~~~~^~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
The origing of the issue seems to be the following calls to function `do_vec_store()`, when
`size > 16`, or more precisely when `size == 32`
arch/powerpc/lib/sstep.c:
3436 case LOAD_VMX:
3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3438 return 0;
3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
3440 break;
...
3507 case STORE_VMX:
3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
3509 return 0;
3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
3511 break;
Inside `do_vec_store()`, we have that the size of `union u` is 16 bytes:
702 int size, struct pt_regs *regs,
703 bool cross_endian)
704 {
705 union {
706 __vector128 v;
707 u8 b[sizeof(__vector128)];
708 } u;
and then function `do_byte_reverse()` is called
721 if (unlikely(cross_endian))
722 do_byte_reverse(&u.b[ea & 0xf], size);
and if `size == 32`, the following piece of code tries to access
`ptr` outside of its boundaries:
260 static nokprobe_inline void do_byte_reverse(void *ptr, int nb)
261 {
262 switch (nb) {
...
281 case 32: {
282 unsigned long *up = (unsigned long *)ptr;
283 unsigned long tmp;
284
285 tmp = byterev_8(up[0]);
286 up[0] = byterev_8(up[3]);
287 up[3] = tmp;
288 tmp = byterev_8(up[2]);
289 up[2] = byterev_8(up[1]);
290 up[1] = tmp;
291 break;
292 }
The following patch is merely a test to illustrate how the value in `size` initially appears
to influence the warnings:
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index a4ab8625061a..86786957b736 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -3436,7 +3436,7 @@ int emulate_loadstore(struct pt_regs *regs, struct instruction_op *op)
case LOAD_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
- err = do_vec_load(op->reg, ea, size, regs, cross_endian);
+ err = do_vec_load(op->reg, ea, (size > 16) ? 16 : size, regs, cross_endian);
break;
#endif
#ifdef CONFIG_VSX
@@ -3507,7 +3507,7 @@ int emulate_loadstore(struct pt_regs *regs, struct instruction_op *op)
case STORE_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
- err = do_vec_store(op->reg, ea, size, regs, cross_endian);
+ err = do_vec_store(op->reg, ea, (size > 16) ? 16 : size, regs, cross_endian);
break;
#endif
#ifdef CONFIG_VSX
Is there something that I may be overlooking here? :)
Thanks!
--
Gustavo
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
2023-11-17 18:36 [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :) Gustavo A. R. Silva
@ 2023-11-20 14:25 ` Naveen N Rao
2023-11-20 14:33 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Naveen N Rao @ 2023-11-20 14:25 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: PowerPC, Nicholas Piggin
On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
> Hi all,
>
> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
> to get your feedback on this, please:
>
> In function 'do_byte_reverse',
> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> 287 | up[3] = tmp;
> | ~~~~~~^~~~~
> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
> 708 | } u;
> | ^
> In function 'do_byte_reverse',
> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> 289 | up[2] = byterev_8(up[1]);
> | ~~~~~~^~~~~~~~~~~~~~~~~~
> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
> 708 | } u;
> | ^
> In function 'do_byte_reverse',
> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> 287 | up[3] = tmp;
> | ~~~~~~^~~~~
> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
> 681 | } u = {};
> | ^
> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>
> The origing of the issue seems to be the following calls to function `do_vec_store()`, when
> `size > 16`, or more precisely when `size == 32`
>
> arch/powerpc/lib/sstep.c:
> 3436 case LOAD_VMX:
> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> 3438 return 0;
> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
> 3440 break;
> ...
> 3507 case STORE_VMX:
> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> 3509 return 0;
> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
> 3511 break;
LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
exceed 16. So, the warning looks incorrect to me.
- Naveen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
2023-11-20 14:25 ` Naveen N Rao
@ 2023-11-20 14:33 ` Gustavo A. R. Silva
2023-11-20 15:21 ` Naveen N Rao
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-20 14:33 UTC (permalink / raw)
To: Naveen N Rao; +Cc: PowerPC, Nicholas Piggin
On 11/20/23 08:25, Naveen N Rao wrote:
> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
>> Hi all,
>>
>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
>> to get your feedback on this, please:
>>
>> In function 'do_byte_reverse',
>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>> 287 | up[3] = tmp;
>> | ~~~~~~^~~~~
>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>> arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
>> 708 | } u;
>> | ^
>> In function 'do_byte_reverse',
>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>> 289 | up[2] = byterev_8(up[1]);
>> | ~~~~~~^~~~~~~~~~~~~~~~~~
>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>> arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
>> 708 | } u;
>> | ^
>> In function 'do_byte_reverse',
>> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>> 287 | up[3] = tmp;
>> | ~~~~~~^~~~~
>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>> 681 | } u = {};
>> | ^
>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>
>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when
>> `size > 16`, or more precisely when `size == 32`
>>
>> arch/powerpc/lib/sstep.c:
>> 3436 case LOAD_VMX:
>> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>> 3438 return 0;
>> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
>> 3440 break;
>> ...
>> 3507 case STORE_VMX:
>> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>> 3509 return 0;
>> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
>> 3511 break;
>
> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
> exceed 16. So, the warning looks incorrect to me.
Does that mean that this piece of code is never actually executed:
281 case 32: {
282 unsigned long *up = (unsigned long *)ptr;
283 unsigned long tmp;
284
285 tmp = byterev_8(up[0]);
286 up[0] = byterev_8(up[3]);
287 up[3] = tmp;
288 tmp = byterev_8(up[2]);
289 up[2] = byterev_8(up[1]);
290 up[1] = tmp;
291 break;
292 }
or rather, under which conditions the above code is executed, if any?
Thanks!
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
2023-11-20 14:33 ` Gustavo A. R. Silva
@ 2023-11-20 15:21 ` Naveen N Rao
2023-11-20 19:29 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Naveen N Rao @ 2023-11-20 15:21 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: PowerPC, Nicholas Piggin
On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote:
>
>
> On 11/20/23 08:25, Naveen N Rao wrote:
> > On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
> > > Hi all,
> > >
> > > I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
> > > to get your feedback on this, please:
> > >
> > > In function 'do_byte_reverse',
> > > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
> > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
> > > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> > > 287 | up[3] = tmp;
> > > | ~~~~~~^~~~~
> > > arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> > > arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
> > > 708 | } u;
> > > | ^
> > > In function 'do_byte_reverse',
> > > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
> > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
> > > arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> > > 289 | up[2] = byterev_8(up[1]);
> > > | ~~~~~~^~~~~~~~~~~~~~~~~~
> > > arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> > > arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
> > > 708 | } u;
> > > | ^
> > > In function 'do_byte_reverse',
> > > inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
> > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
> > > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
> > > 287 | up[3] = tmp;
> > > | ~~~~~~^~~~~
> > > arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
> > > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
> > > 681 | } u = {};
> > > | ^
> > > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
> > > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
> > >
> > > The origing of the issue seems to be the following calls to function `do_vec_store()`, when
> > > `size > 16`, or more precisely when `size == 32`
> > >
> > > arch/powerpc/lib/sstep.c:
> > > 3436 case LOAD_VMX:
> > > 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> > > 3438 return 0;
> > > 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
> > > 3440 break;
> > > ...
> > > 3507 case STORE_VMX:
> > > 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> > > 3509 return 0;
> > > 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
> > > 3511 break;
> >
> > LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
> > exceed 16. So, the warning looks incorrect to me.
>
> Does that mean that this piece of code is never actually executed:
>
> 281 case 32: {
> 282 unsigned long *up = (unsigned long *)ptr;
> 283 unsigned long tmp;
> 284
> 285 tmp = byterev_8(up[0]);
> 286 up[0] = byterev_8(up[3]);
> 287 up[3] = tmp;
> 288 tmp = byterev_8(up[2]);
> 289 up[2] = byterev_8(up[1]);
> 290 up[1] = tmp;
> 291 break;
> 292 }
>
> or rather, under which conditions the above code is executed, if any?
Please see git history to understand the commit that introduced that
code. You can do that by starting with a 'git blame' on the file. You'll
find that the commit that introduced the above hunk was af99da74333b
("powerpc/sstep: Support VSX vector paired storage access
instructions").
The above code path is hit via
do_vsx_load()->emulate_vsx_load()->do_byte_reverse()
- Naveen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
2023-11-20 15:21 ` Naveen N Rao
@ 2023-11-20 19:29 ` Gustavo A. R. Silva
2023-11-20 23:51 ` Michael Ellerman
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2023-11-20 19:29 UTC (permalink / raw)
To: Naveen N Rao, Balamuruhan S; +Cc: PowerPC, Nicholas Piggin
On 11/20/23 09:21, Naveen N Rao wrote:
> On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote:
>>
>>
>> On 11/20/23 08:25, Naveen N Rao wrote:
>>> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
>>>> Hi all,
>>>>
>>>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
>>>> to get your feedback on this, please:
>>>>
>>>> In function 'do_byte_reverse',
>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>> 287 | up[3] = tmp;
>>>> | ~~~~~~^~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>> 708 | } u;
>>>> | ^
>>>> In function 'do_byte_reverse',
>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>> 289 | up[2] = byterev_8(up[1]);
>>>> | ~~~~~~^~~~~~~~~~~~~~~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
>>>> 708 | } u;
>>>> | ^
>>>> In function 'do_byte_reverse',
>>>> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>> 287 | up[3] = tmp;
>>>> | ~~~~~~^~~~~
>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>> 681 | } u = {};
>>>> | ^
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>
>>>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when
>>>> `size > 16`, or more precisely when `size == 32`
>>>>
>>>> arch/powerpc/lib/sstep.c:
>>>> 3436 case LOAD_VMX:
>>>> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>> 3438 return 0;
>>>> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
>>>> 3440 break;
>>>> ...
>>>> 3507 case STORE_VMX:
>>>> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>> 3509 return 0;
>>>> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
>>>> 3511 break;
>>>
>>> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
>>> exceed 16. So, the warning looks incorrect to me.
>>
>> Does that mean that this piece of code is never actually executed:
>>
>> 281 case 32: {
>> 282 unsigned long *up = (unsigned long *)ptr;
>> 283 unsigned long tmp;
>> 284
>> 285 tmp = byterev_8(up[0]);
>> 286 up[0] = byterev_8(up[3]);
>> 287 up[3] = tmp;
>> 288 tmp = byterev_8(up[2]);
>> 289 up[2] = byterev_8(up[1]);
>> 290 up[1] = tmp;
>> 291 break;
>> 292 }
>>
>> or rather, under which conditions the above code is executed, if any?
>
> Please see git history to understand the commit that introduced that
> code. You can do that by starting with a 'git blame' on the file. You'll
> find that the commit that introduced the above hunk was af99da74333b
> ("powerpc/sstep: Support VSX vector paired storage access
> instructions").
>
> The above code path is hit via
> do_vsx_load()->emulate_vsx_load()->do_byte_reverse()
It seems there is another path (at least the one that triggers the
-Wstringop-overflow warnings):
emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse()
emulate_step() {
...
if (OP_IS_LOAD_STORE(type)) {
err = emulate_loadstore(regs, &op);
if (err)
return 0;
goto instr_done;
}
...
}
----> emulate_loadstore() {
...
#ifdef CONFIG_ALTIVEC
case LOAD_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
err = do_vec_load(op->reg, ea, size, regs, cross_endian); // with size == 32
break;
#endif
...
#ifdef CONFIG_ALTIVEC
case STORE_VMX:
if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
return 0;
err = do_vec_store(op->reg, ea, size, regs, cross_endian); // with size == 32
break;
#endif
...
}
----> do_vec_load() // Here is where we have the union of size 16 bytes:
{
...
union {
__vector128 v;
u8 b[sizeof(__vector128)];
} u = {};
...
if (unlikely(cross_endian))
do_byte_reverse(&u.b[ea & 0xf], size);
...
}
----> do_byte_reverse()
{
...
case 32: {
unsigned long *up = (unsigned long *)ptr; // this is a pointer to u.b
unsigned long tmp;
tmp = byterev_8(up[0]);
up[0] = byterev_8(up[3]);
up[3] = tmp; // and here...
tmp = byterev_8(up[2]);
up[2] = byterev_8(up[1]); // and here we are accessing ptr out-of-bounds
up[1] = tmp;
break;
}
...
}
This is where I'm trying to determine if there is a bug. If this path is actually
executed, then it seems we have an issue.
Is it possible that commit af99da74333b overlooked the path described above?
--
Gustavo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)
2023-11-20 19:29 ` Gustavo A. R. Silva
@ 2023-11-20 23:51 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2023-11-20 23:51 UTC (permalink / raw)
To: Gustavo A. R. Silva, Naveen N Rao, Balamuruhan S; +Cc: PowerPC, Nicholas Piggin
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> On 11/20/23 09:21, Naveen N Rao wrote:
>> On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote:
>>> On 11/20/23 08:25, Naveen N Rao wrote:
>>>> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote:
>>>>> Hi all,
>>>>>
>>>>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like
>>>>> to get your feedback on this, please:
>>>>>
>>>>> In function 'do_byte_reverse',
>>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>> 287 | up[3] = tmp;
>>>>> | ~~~~~~^~~~~
>>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>> 708 | } u;
>>>>> | ^
>>>>> In function 'do_byte_reverse',
>>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3,
>>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9:
>>>>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>> 289 | up[2] = byterev_8(up[1]);
>>>>> | ~~~~~~^~~~~~~~~~~~~~~~~~
>>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>>> arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into destination object 'u' of size 16
>>>>> 708 | } u;
>>>>> | ^
>>>>> In function 'do_byte_reverse',
>>>>> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3,
>>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9:
>>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=]
>>>>> 287 | up[3] = tmp;
>>>>> | ~~~~~~^~~~~
>>>>> arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
>>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>> 681 | } u = {};
>>>>> | ^
>>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16
>>>>>
>>>>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when
>>>>> `size > 16`, or more precisely when `size == 32`
>>>>>
>>>>> arch/powerpc/lib/sstep.c:
>>>>> 3436 case LOAD_VMX:
>>>>> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>>> 3438 return 0;
>>>>> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian);
>>>>> 3440 break;
>>>>> ...
>>>>> 3507 case STORE_VMX:
>>>>> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
>>>>> 3509 return 0;
>>>>> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian);
>>>>> 3511 break;
>>>>
>>>> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not
>>>> exceed 16. So, the warning looks incorrect to me.
>>>
>>> Does that mean that this piece of code is never actually executed:
>>>
>>> 281 case 32: {
>>> 282 unsigned long *up = (unsigned long *)ptr;
>>> 283 unsigned long tmp;
>>> 284
>>> 285 tmp = byterev_8(up[0]);
>>> 286 up[0] = byterev_8(up[3]);
>>> 287 up[3] = tmp;
>>> 288 tmp = byterev_8(up[2]);
>>> 289 up[2] = byterev_8(up[1]);
>>> 290 up[1] = tmp;
>>> 291 break;
>>> 292 }
>>>
>>> or rather, under which conditions the above code is executed, if any?
>>
>> Please see git history to understand the commit that introduced that
>> code. You can do that by starting with a 'git blame' on the file. You'll
>> find that the commit that introduced the above hunk was af99da74333b
>> ("powerpc/sstep: Support VSX vector paired storage access
>> instructions").
>>
>> The above code path is hit via
>> do_vsx_load()->emulate_vsx_load()->do_byte_reverse()
>
> It seems there is another path (at least the one that triggers the
> -Wstringop-overflow warnings):
>
> emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse()
>
> emulate_step() {
> ...
> if (OP_IS_LOAD_STORE(type)) {
The type comes from the op, which is determined in analyse_instr()
> err = emulate_loadstore(regs, &op);
> if (err)
> return 0;
> goto instr_done;
> }
> ...
> }
>
> ----> emulate_loadstore() {
> ...
> #ifdef CONFIG_ALTIVEC
> case LOAD_VMX:
> if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> return 0;
> err = do_vec_load(op->reg, ea, size, regs, cross_endian); // with size == 32
> break;
> #endif
> ...
> #ifdef CONFIG_ALTIVEC
> case STORE_VMX:
> if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC))
> return 0;
> err = do_vec_store(op->reg, ea, size, regs, cross_endian); // with size == 32
> break;
> #endif
> ...
> }
If you look at analyse_instr() the cases that produce an op with
type == LOAD_VMX/STORE_VMX never use a size of 32:
$ git grep -E "MKOP\((LOAD|STORE)_VMX" arch/powerpc/
arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 1);
arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 2);
arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 4);
arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 16);
arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 1);
arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 2);
arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 4);
arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 16);
So I don't think there's an actual bug.
But the code is very hard to follow and it would be very easy for a bug
to be introduced.
Déjà vu intensifies...
... and apparently I have a patch for this that I never sent out. Sorry! :}
I'll post it and see if the build bots are happy.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-20 23:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-17 18:36 [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :) Gustavo A. R. Silva
2023-11-20 14:25 ` Naveen N Rao
2023-11-20 14:33 ` Gustavo A. R. Silva
2023-11-20 15:21 ` Naveen N Rao
2023-11-20 19:29 ` Gustavo A. R. Silva
2023-11-20 23:51 ` Michael Ellerman
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).