| Message ID | 20231026021613.GI800259@ZenIV |
|---|---|
| State | New |
| Headers | show |
| Series | sparc32: fix a braino in fault handling in csum_and_copy_..._user() | expand |
Hi Al, On Thu, Oct 26, 2023 at 03:16:13AM +0100, Al Viro wrote: > Fault handler used to make non-trivial calls, so it needed > to set a stack frame up. Used to be > save ... - grab a stack frame, old %o... become %i... > .... > ret - go back to address originally in %o7, currently %i7 > restore - switch to previous stack frame, in delay slot > Non-trivial calls had been gone since ab5e8b331244 and that code should > have become > retl - go back to address in %o7 > clr %o0 - have return value set to 0 > What it had become instead was > ret - go back to address in %i7 - return address of *caller* > clr %o0 - have return value set to 0 > which is not good, to put it mildly - we forcibly return 0 from > csum_and_copy_{from,to}_iter() (which is what the call of that > thing had been inlined into) and do that without dropping the > stack frame of said csum_and_copy_..._iter(). Confuses the > hell out of the caller of csum_and_copy_..._iter(), obviously... I wonder how you managed to find this? Do you actually use sparc32 these days? You could also kill the EX2 define while touchign the file, it is no longer used after ab5e8b331244. > > Fixes: ab5e8b331244 "sparc32: propagate the calling conventions change down to __csum_partial_copy_sparc_generic()" > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > --- > arch/sparc/lib/checksum_32.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/sparc/lib/checksum_32.S b/arch/sparc/lib/checksum_32.S > index 84ad709cbecb..66eda40fce36 100644 > --- a/arch/sparc/lib/checksum_32.S > +++ b/arch/sparc/lib/checksum_32.S > @@ -453,5 +453,5 @@ ccslow: cmp %g1, 0 > * we only bother with faults on loads... */ > > cc_fault: > - ret > + retl > clr %o0 > -- > 2.39.2
On Fri, Oct 27, 2023 at 08:47:53PM +0200, Sam Ravnborg wrote: > Hi Al, > > On Thu, Oct 26, 2023 at 03:16:13AM +0100, Al Viro wrote: > > Fault handler used to make non-trivial calls, so it needed > > to set a stack frame up. Used to be > > save ... - grab a stack frame, old %o... become %i... > > .... > > ret - go back to address originally in %o7, currently %i7 > > restore - switch to previous stack frame, in delay slot > > Non-trivial calls had been gone since ab5e8b331244 and that code should > > have become > > retl - go back to address in %o7 > > clr %o0 - have return value set to 0 > > What it had become instead was > > ret - go back to address in %i7 - return address of *caller* > > clr %o0 - have return value set to 0 > > which is not good, to put it mildly - we forcibly return 0 from > > csum_and_copy_{from,to}_iter() (which is what the call of that > > thing had been inlined into) and do that without dropping the > > stack frame of said csum_and_copy_..._iter(). Confuses the > > hell out of the caller of csum_and_copy_..._iter(), obviously... > > I wonder how you managed to find this? Looking at the csum_and_copy_... instances on various architectures, noticing that and going "how the fuck could it possibly work and what moron had broken it? Oh, lovely - it couldn't, it doesn't and that moron had been myself ;-/" > Do you actually use sparc32 these > days? qemu image only, TBH - I have an SS20 box, but it hadn't booted for about 10 years... > You could also kill the EX2 define while touchign the file, > it is no longer used after ab5e8b331244. Er? No EX2 in checksum_32.S... There is one in copy_user.S, but that one _is_ used - copy_user_table_end: be copy_user_last7 andcc %g1, 4, %g0 EX(ldd [%o1], %g2, and %g1, 0xf) add %o0, 8, %o0 add %o1, 8, %o1 EX(st %g2, [%o0 - 0x08], and %g1, 0xf) EX2(st %g3, [%o0 - 0x04], and %g1, 0xf, %g1, sub %g1, 4) > > Fixes: ab5e8b331244 "sparc32: propagate the calling conventions change down to __csum_partial_copy_sparc_generic()" > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Hi Al, > > Looking at the csum_and_copy_... instances on various architectures, > noticing that and going "how the fuck could it possibly work and > what moron had broken it? Oh, lovely - it couldn't, it doesn't > and that moron had been myself ;-/" :-) > > > Do you actually use sparc32 these > > days? > > qemu image only, TBH - I have an SS20 box, but it hadn't booted for > about 10 years... Looked at my "museum". At least 5 pcs of sparc32 boxes but I guess the NVRAM needs repair on all of them. I have a LEON4 board somewhere on the shelf too. None of this powered up for years either - but I cannot make myself getting rid of it. Because maybe one day I find time... All that said - I have also once suggested to drop sun4m support, keeping only the LEON parts. Back then there was some resistance, but all for sentimental reasons and I can relate to that, since I kept the sparc32 boxes around. > > You could also kill the EX2 define while touchign the file, > > it is no longer used after ab5e8b331244. > > Er? No EX2 in checksum_32.S... There is one in copy_user.S, > but that one _is_ used - > > copy_user_table_end: > be copy_user_last7 > andcc %g1, 4, %g0 > > EX(ldd [%o1], %g2, and %g1, 0xf) > add %o0, 8, %o0 > add %o1, 8, %o1 > EX(st %g2, [%o0 - 0x08], and %g1, 0xf) > EX2(st %g3, [%o0 - 0x04], and %g1, 0xf, %g1, sub %g1, 4) Yeah, I'm blind. Somehow the grep output tricked me. Sam
diff --git a/arch/sparc/lib/checksum_32.S b/arch/sparc/lib/checksum_32.S index 84ad709cbecb..66eda40fce36 100644 --- a/arch/sparc/lib/checksum_32.S +++ b/arch/sparc/lib/checksum_32.S @@ -453,5 +453,5 @@ ccslow: cmp %g1, 0 * we only bother with faults on loads... */ cc_fault: - ret + retl clr %o0
Fault handler used to make non-trivial calls, so it needed to set a stack frame up. Used to be save ... - grab a stack frame, old %o... become %i... .... ret - go back to address originally in %o7, currently %i7 restore - switch to previous stack frame, in delay slot Non-trivial calls had been gone since ab5e8b331244 and that code should have become retl - go back to address in %o7 clr %o0 - have return value set to 0 What it had become instead was ret - go back to address in %i7 - return address of *caller* clr %o0 - have return value set to 0 which is not good, to put it mildly - we forcibly return 0 from csum_and_copy_{from,to}_iter() (which is what the call of that thing had been inlined into) and do that without dropping the stack frame of said csum_and_copy_..._iter(). Confuses the hell out of the caller of csum_and_copy_..._iter(), obviously... Fixes: ab5e8b331244 "sparc32: propagate the calling conventions change down to __csum_partial_copy_sparc_generic()" Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- arch/sparc/lib/checksum_32.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)