Re: [PATCH v2 20/20] ppc: propagate the calling conventions change down to csum_partial_copy_generic()

Hi Al,

On Fri, Jul 24, 2020 at 02:25:46AM +0100, Al Viro wrote:

From: Al Viro <viro@zeniv.linux.org.uk>

... and get rid of the pointless fallback in the wrappers.  On error it used
to zero the unwritten area and calculate the csum of the entire thing.  Not
wanting to do it in assembler part had been very reasonable; doing that in
the first place, OTOH...  In case of an error the caller discards the data
we'd copied, along with whatever checksum it might've had.

This patch is causing crashes in WireGuard's CI over at
https://www.wireguard.com/build-status/ . Apparently sending a simple
network packet winds up triggering refcount_t's warn-on-saturate code. I
don't know if the new assembly failed to reset some flag or if something
else is up. I can start digging into it if you want, but I thought I
should let you know first about the issue. The splat follows below.

Thanks,
Jason

$ ping -c 10 -f -W 1 192.168.241.1
PING 192.168.241.1 (192.168.241.1) 56(84) bytes of data.
[    1.432922] ------------[ cut here ]------------
[    1.433069] refcount_t: saturated; leaking memory.
[    1.433344] WARNING: CPU: 3 PID: 90 at refcount_warn_saturate+0x100/0x1bc
[    1.433646] CPU: 3 PID: 90 Comm: ping Not tainted 5.9.0+ #3
[    1.433797] NIP:  c01a6fa0 LR: c01a6fa0 CTR: c01ccbec
[    1.433964] REGS: cfacfb80 TRAP: 0700   Not tainted  (5.9.0+)
[    1.434102] MSR:  00029000 <CE,EE,ME>  CR: 28022404  XER: 00000000
[    1.434345]
[    1.434345] GPR00: c01a6fa0 cfacfc38 cf8eeae0 00000026 3fffefff cfacfa90 cfacfaa0 00021000
[    1.434345] GPR08: 0f4a1000 00000000 c08b4674 c0918704 42022404 00000000 cfa34180 00000000
[    1.434345] GPR16: 00000000 cf8ef004 00000000 00000000 00000040 00000000 00000000 cfbac230
[    1.434345] GPR24: cfacfce8 c02a802c 00000000 cfa34180 cfacfc58 c02aa53c 55c0a4ff 00000000
[    1.435471] NIP [c01a6fa0] refcount_warn_saturate+0x100/0x1bc
[    1.435615] LR [c01a6fa0] refcount_warn_saturate+0x100/0x1bc
[    1.435825] Call Trace:
[    1.435922] [cfacfc38] [c01a6fa0] refcount_warn_saturate+0x100/0x1bc (unreliable)
[    1.436149] [cfacfc48] [c02a7f14] __ip_append_data.isra.0+0x8a8/0xde0
[    1.436302] [cfacfce8] [c02a84e0] ip_append_data.part.0+0x94/0xf0
[    1.436438] [cfacfd18] [c02dffe0] raw_sendmsg+0x298/0xa84
[    1.436544] [cfacfe48] [c020b9ec] __sys_sendto+0xdc/0x13c
[    1.436641] [cfacff38] [c000f1dc] ret_from_syscall+0x0/0x38
[    1.436824] --- interrupt: c01 at 0xb7e44f00
[    1.436824]     LR = 0xb7e21ba0
[    1.437038] Instruction dump:
[    1.437239] 3d20c092 39291bc1 89490001 2c0a0000 4082ff64 3c60c040 7c0802a6 39400001
[    1.437439] 38633b74 90010014 99490001 4be9b6e1 <0fe00000> 80010014 7c0803a6 4bffff38
[    1.437753] ---[ end trace aaa4b4788958d0a6 ]---

68 more lines

On Thu, Oct 15, 2020 at 12:51 AM Linus Torvalds
[off-list ref] wrote:

On Wed, Oct 14, 2020 at 3:27 PM Jason A. Donenfeld [off-list ref] wrote:

quoted

This patch is causing crashes in WireGuard's CI over at
https://www.wireguard.com/build-status/ . Apparently sending a simple
network packet winds up triggering refcount_t's warn-on-saturate code. I

Ouch.

The C parts look fairly straightforward, and I don't see how they
could cause that odd refcount issue.

So I assume it's the low-level asm code conversion that is buggy. And
it's apparently the 32-bit conversion, since your ppc64 status looks
fine.

I think it's this instruction:

        addi    r1,r1,16

that should be removed from the function exit, because Al removed the

-       stwu    r1,-16(r1)

I just tried that about a minute ago, and indeed that seems to be
what's up. Problem goes away without it. I'll send a patch shortly.

Jason

On Thu, Oct 15, 2020 at 12:53 AM Linus Torvalds
[off-list ref] wrote:

On Wed, Oct 14, 2020 at 3:51 PM Linus Torvalds
[off-list ref] wrote:

quoted

I think it's this instruction:

        addi    r1,r1,16

that should be removed from the function exit, because Al removed the

-       stwu    r1,-16(r1)

on function entry.

So I think you end up with a corrupt stack pointer and basically
random behavior.

Mind trying that? (This is obviously all in
arch/powerpc/lib/checksum_32.S, the csum_partial_copy_generic()
function).

Patch attached to make it easier to test.

NOTE! This is ENTIRELY untested. My ppc asm is so rusty that I might
be barking entirely up the wrong tree, and I just made things much
worse.

Indeed - exactly the same thing that's in my tree. Writing a commit
message for it now.

Jason

A recent change to the checksum code removed usage of some extra
arguments, alongside with storage on the stack for those, and the stack
pointer no longer needed to be adjusted in the function prologue. But, a
left over subtraction wasn't removed in the function epilogue, causing
the function to return with the stack pointer moved 16 bytes away from
where it should have. This corrupted local state and lead to weird
crashes. This commit simply removes the leftover instruction from the
epilogue.

Fixes: 70d65cd555c5 ("ppc: propagate the calling conventions change down to csum_partial_copy_generic()")
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/powerpc/lib/checksum_32.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/lib/checksum_32.S b/arch/powerpc/lib/checksum_32.S
index ec5cd2dede35..27d9070617df 100644
--- a/arch/powerpc/lib/checksum_32.S
+++ b/arch/powerpc/lib/checksum_32.S

@@ -236,7 +236,6 @@ _GLOBAL(csum_partial_copy_generic)
 	slwi	r0,r0,8
 	adde	r12,r12,r0
 66:	addze	r3,r12
-	addi	r1,r1,16
 	beqlr+	cr7
 	rlwinm	r3,r3,8,0,31	/* odd destination address: rotate one byte */
 	blr

-- 
2.28.0

On Wed, Oct 14, 2020 at 3:51 PM Linus Torvalds
[off-list ref] wrote:

I think it's this instruction:

        addi    r1,r1,16

that should be removed from the function exit, because Al removed the

-       stwu    r1,-16(r1)

on function entry.

So I think you end up with a corrupt stack pointer and basically
random behavior.

Mind trying that? (This is obviously all in
arch/powerpc/lib/checksum_32.S, the csum_partial_copy_generic()
function).

Patch attached to make it easier to test.

NOTE! This is ENTIRELY untested. My ppc asm is so rusty that I might
be barking entirely up the wrong tree, and I just made things much
worse.

                 Linus

On Wed, Oct 14, 2020 at 3:27 PM Jason A. Donenfeld [off-list ref] wrote:

This patch is causing crashes in WireGuard's CI over at
https://www.wireguard.com/build-status/ . Apparently sending a simple
network packet winds up triggering refcount_t's warn-on-saturate code. I

Ouch.

The C parts look fairly straightforward, and I don't see how they
could cause that odd refcount issue.

So I assume it's the low-level asm code conversion that is buggy. And
it's apparently the 32-bit conversion, since your ppc64 status looks
fine.

I think it's this instruction:

        addi    r1,r1,16

that should be removed from the function exit, because Al removed the

-       stwu    r1,-16(r1)

on function entry.

So I think you end up with a corrupt stack pointer and basically
random behavior.

Mind trying that? (This is obviously all in
arch/powerpc/lib/checksum_32.S, the csum_partial_copy_generic()
function).

               Linus

Thanks - applied and pushed out.

             Linus

On Wed, Oct 14, 2020 at 03:51:00PM -0700, Linus Torvalds wrote:

On Wed, Oct 14, 2020 at 3:27 PM Jason A. Donenfeld [off-list ref] wrote:

quoted

This patch is causing crashes in WireGuard's CI over at
https://www.wireguard.com/build-status/ . Apparently sending a simple
network packet winds up triggering refcount_t's warn-on-saturate code. I

Ouch.

The C parts look fairly straightforward, and I don't see how they
could cause that odd refcount issue.

So I assume it's the low-level asm code conversion that is buggy. And
it's apparently the 32-bit conversion, since your ppc64 status looks
fine.

I think it's this instruction:

        addi    r1,r1,16

that should be removed from the function exit, because Al removed the

-       stwu    r1,-16(r1)

on function entry.

So I think you end up with a corrupt stack pointer and basically
random behavior.

Gyahh...  ACK, and I really wonder how the hell has it managed to avoid
crashing on testing.

Mea culpa, folks.