Message ID | 20220607143019.135154-1-msc@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | powerpc: Fix VSX register number on __strncpy_power9 [BZ #29197] | expand |
On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote: > __strncpy_power9 initializes VR 18 with zeroes to be used throughout the > code, including when zero-padding the destination string. However, the > v18 reference was mistakenly being used for stxv and stxvl, which take a > VSX vector as operand. The code ended up using the uninitialized VSR 18 > register by mistake. > > Both occurrences have been changed to use the proper VSX number for VR 18 > (i.e. VSR 50). > > Tested on powerpc, powerpc64 and powerpc64le. > > Suggested-by: Kewen Lin <linkw@gcc.gnu.org> > --- > sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > index ae23161316..deb94671cc 100644 > --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > @@ -352,7 +352,7 @@ L(zero_padding_loop): > cmpldi cr6,r5,16 /* Check if length was reached. */ > ble cr6,L(zero_padding_end) > > - stxv v18,0(r11) > + stxv 32+v18,0(r11) > addi r11,r11,16 > addi r5,r5,-16 > > @@ -360,7 +360,7 @@ L(zero_padding_loop): > > L(zero_padding_end): > sldi r10,r5,56 /* stxvl wants size in top 8 bits */ > - stxvl v18,r11,r10 /* Partial store */ > + stxvl 32+v18,r11,r10 /* Partial store */ > blr > > .align 4 LGTM, will you also backport this as needed too? If this wasn't caught by a test-case, I think adding one would be desirable too.
Paul E Murphy <murphyp@linux.ibm.com> writes: > On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote: >> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the >> code, including when zero-padding the destination string. However, the >> v18 reference was mistakenly being used for stxv and stxvl, which take a >> VSX vector as operand. The code ended up using the uninitialized VSR 18 >> register by mistake. >> Both occurrences have been changed to use the proper VSX number for VR 18 >> (i.e. VSR 50). >> Tested on powerpc, powerpc64 and powerpc64le. >> Suggested-by: Kewen Lin <linkw@gcc.gnu.org> >> --- >> sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >> b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >> index ae23161316..deb94671cc 100644 >> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >> @@ -352,7 +352,7 @@ L(zero_padding_loop): >> cmpldi cr6,r5,16 /* Check if length was reached. */ >> ble cr6,L(zero_padding_end) >> - stxv v18,0(r11) >> + stxv 32+v18,0(r11) >> addi r11,r11,16 >> addi r5,r5,-16 >> @@ -360,7 +360,7 @@ L(zero_padding_loop): >> L(zero_padding_end): >> sldi r10,r5,56 /* stxvl wants size in top 8 bits */ >> - stxvl v18,r11,r10 /* Partial store */ >> + stxvl 32+v18,r11,r10 /* Partial store */ >> blr >> .align 4 > > LGTM, will you also backport this as needed too? Yes, that's the idea. Ideally to all affected releases. > If this wasn't caught by a test-case, I think adding one would be desirable too. string/test-strncpy.c already checks if the bytes beyond the terminating NUL have been properly set [1]. But the issue is that vs18 is also zero when the test starts, so even with the bug the string ends up in the expected state. To actually trigger the bug we would have to fill vs18 with something != NUL before calling __strncpy_power9 (as is done in the reproducer). But since this is very Power-specific and the test is generic, I'm not sure what's the best way forward. -- Matheus Castanho
On 6/7/22 10:37, Paul E Murphy via Libc-alpha wrote: > LGTM, will you also backport this as needed too? > > If this wasn't caught by a test-case, I think adding one would be desirable too. Please commit the fix first and we can add a test case later. This particular class of bug is often tricky to test. Given that you have ACK'd the bug please feel free to backport to all affected release branches. I'm interested in backporting this fix downstream :-)
On 07/06/2022 20:00, Matheus Castanho via Libc-alpha wrote: > __strncpy_power9 initializes VR 18 with zeroes to be used throughout the > code, including when zero-padding the destination string. However, the > v18 reference was mistakenly being used for stxv and stxvl, which take a > VSX vector as operand. The code ended up using the uninitialized VSR 18 > register by mistake. > > Both occurrences have been changed to use the proper VSX number for VR 18 > (i.e. VSR 50). > > Tested on powerpc, powerpc64 and powerpc64le. > > Suggested-by: Kewen Lin <linkw@gcc.gnu.org> Signed-off-by is the right tag here given that it is pretty much the same patch that's in the bugzilla. > --- > sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > index ae23161316..deb94671cc 100644 > --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S > @@ -352,7 +352,7 @@ L(zero_padding_loop): > cmpldi cr6,r5,16 /* Check if length was reached. */ > ble cr6,L(zero_padding_end) > > - stxv v18,0(r11) > + stxv 32+v18,0(r11) > addi r11,r11,16 > addi r5,r5,-16 > > @@ -360,7 +360,7 @@ L(zero_padding_loop): > > L(zero_padding_end): > sldi r10,r5,56 /* stxvl wants size in top 8 bits */ > - stxvl v18,r11,r10 /* Partial store */ > + stxvl 32+v18,r11,r10 /* Partial store */ > blr > > .align 4
On 6/7/22 9:50 AM, Matheus Castanho wrote: > > Paul E Murphy <murphyp@linux.ibm.com> writes: > >> On 6/7/22 9:30 AM, Matheus Castanho via Libc-alpha wrote: >>> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the >>> code, including when zero-padding the destination string. However, the >>> v18 reference was mistakenly being used for stxv and stxvl, which take a >>> VSX vector as operand. The code ended up using the uninitialized VSR 18 >>> register by mistake. >>> Both occurrences have been changed to use the proper VSX number for VR 18 >>> (i.e. VSR 50). >>> Tested on powerpc, powerpc64 and powerpc64le. >>> Suggested-by: Kewen Lin <linkw@gcc.gnu.org> >>> --- >>> sysdeps/powerpc/powerpc64/le/power9/strncpy.S | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >>> b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >>> index ae23161316..deb94671cc 100644 >>> --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >>> +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S >>> @@ -352,7 +352,7 @@ L(zero_padding_loop): >>> cmpldi cr6,r5,16 /* Check if length was reached. */ >>> ble cr6,L(zero_padding_end) >>> - stxv v18,0(r11) >>> + stxv 32+v18,0(r11) >>> addi r11,r11,16 >>> addi r5,r5,-16 >>> @@ -360,7 +360,7 @@ L(zero_padding_loop): >>> L(zero_padding_end): >>> sldi r10,r5,56 /* stxvl wants size in top 8 bits */ >>> - stxvl v18,r11,r10 /* Partial store */ >>> + stxvl 32+v18,r11,r10 /* Partial store */ >>> blr >>> .align 4 >> >> LGTM, will you also backport this as needed too? > > Yes, that's the idea. Ideally to all affected releases. > >> If this wasn't caught by a test-case, I think adding one would be desirable too. > > string/test-strncpy.c already checks if the bytes beyond the terminating > NUL have been properly set [1]. But the issue is that vs18 is also zero > when the test starts, so even with the bug the string ends up in the > expected state. > > To actually trigger the bug we would have to fill vs18 with something != > NUL before calling __strncpy_power9 (as is done in the reproducer). But > since this is very Power-specific and the test is generic, I'm not sure > what's the best way forward. Yes, agreed. This is a hard thing to catch. As Carlos noted, please commit this; it is a fairly obvious fix (to me). I don't have any practical ideas at the moment for randomizing unused caller-saved registers in this case.
Siddhesh Poyarekar <siddhesh@gotplt.org> writes: > On 07/06/2022 20:00, Matheus Castanho via Libc-alpha wrote: >> __strncpy_power9 initializes VR 18 with zeroes to be used throughout the >> code, including when zero-padding the destination string. However, the >> v18 reference was mistakenly being used for stxv and stxvl, which take a >> VSX vector as operand. The code ended up using the uninitialized VSR 18 >> register by mistake. >> Both occurrences have been changed to use the proper VSX number for VR 18 >> (i.e. VSR 50). >> Tested on powerpc, powerpc64 and powerpc64le. >> Suggested-by: Kewen Lin <linkw@gcc.gnu.org> > > Signed-off-by is the right tag here given that it is pretty much the same patch > that's in the bugzilla. > Ack. I'll change it before pushing. Thanks, Matheus Castanho
"Carlos O'Donell" <carlos@redhat.com> writes: > On 6/7/22 10:37, Paul E Murphy via Libc-alpha wrote: >> LGTM, will you also backport this as needed too? >> >> If this wasn't caught by a test-case, I think adding one would be desirable too. > > Please commit the fix first and we can add a test case later. > > This particular class of bug is often tricky to test. > > Given that you have ACK'd the bug please feel free to backport to all affected release branches. > > I'm interested in backporting this fix downstream :-) Pushed and backported to all affected versions: 2.33, 2.34 and 2.35 -- Matheus Castanho
diff --git a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S index ae23161316..deb94671cc 100644 --- a/sysdeps/powerpc/powerpc64/le/power9/strncpy.S +++ b/sysdeps/powerpc/powerpc64/le/power9/strncpy.S @@ -352,7 +352,7 @@ L(zero_padding_loop): cmpldi cr6,r5,16 /* Check if length was reached. */ ble cr6,L(zero_padding_end) - stxv v18,0(r11) + stxv 32+v18,0(r11) addi r11,r11,16 addi r5,r5,-16 @@ -360,7 +360,7 @@ L(zero_padding_loop): L(zero_padding_end): sldi r10,r5,56 /* stxvl wants size in top 8 bits */ - stxvl v18,r11,r10 /* Partial store */ + stxvl 32+v18,r11,r10 /* Partial store */ blr .align 4