On 8/31/20 3:05 AM, Tuong Tong Lien wrote:

quoted

-----Original Message-----
From: Eric Dumazet <redacted>
Sent: Monday, August 31, 2020 4:48 PM
To: Tuong Tong Lien <redacted>; Eric Dumazet <redacted>; davem@davemloft.net;
jmaloy@redhat.com; maloy@donjonn.com; ying.xue@windriver.com; netdev@vger.kernel.org
Cc: tipc-discussion@lists.sourceforge.net
Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible



On 8/31/20 1:33 AM, Tuong Tong Lien wrote:

quoted

Hi Eric,

Thanks for your comments, please see my answers inline.

quoted

-----Original Message-----
From: Eric Dumazet <redacted>
Sent: Monday, August 31, 2020 3:15 PM
To: Tuong Tong Lien <redacted>; davem@davemloft.net; jmaloy@redhat.com; maloy@donjonn.com;
ying.xue@windriver.com; netdev@vger.kernel.org
Cc: tipc-discussion@lists.sourceforge.net
Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible



On 8/29/20 12:37 PM, Tuong Lien wrote:

quoted

The 'this_cpu_ptr()' is used to obtain the AEAD key' TFM on the current
CPU for encryption, however the execution can be preemptible since it's
actually user-space context, so the 'using smp_processor_id() in
preemptible' has been observed.

We fix the issue by using the 'get/put_cpu_ptr()' API which consists of
a 'preempt_disable()' instead.

Fixes: fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication")

Have you forgotten ' Reported-by: syzbot+263f8c0d007dc09b2dda@syzkaller.appspotmail.com' ?

Well, really I detected the issue during my testing instead, didn't know if it was reported by syzbot too.

quoted

quoted

Acked-by: Jon Maloy <jmaloy@redhat.com>
Signed-off-by: Tuong Lien <redacted>
---
 net/tipc/crypto.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index c38babaa4e57..7c523dc81575 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c

@@ -326,7 +326,8 @@ static void tipc_aead_free(struct rcu_head *rp)
 	if (aead->cloned) {
 		tipc_aead_put(aead->cloned);
 	} else {
-		head = *this_cpu_ptr(aead->tfm_entry);
+		head = *get_cpu_ptr(aead->tfm_entry);
+		put_cpu_ptr(aead->tfm_entry);

Why is this safe ?

I think that this very unusual construct needs a comment, because this is not obvious.

This really looks like an attempt to silence syzbot to me.

No, this is not to silence syzbot but really safe.
This is because the "aead->tfm_entry" object is "common" between CPUs, there is only its pointer to be the "per_cpu" one. So just

trying to lock the process on the current CPU or 'preempt_disable()', taking the per-cpu pointer and dereferencing to the actual
"tfm_entry" object... is enough. Later on, that’s fine to play with the actual object without any locking.

Why using per cpu pointers, if they all point to a common object ?

This makes the code really confusing.

Sorry for making you confused. Yes, the code is a bit ugly and could be made in some other ways... The initial idea is to not touch or change the same pointer variable in different CPUs so avoid a penalty with the cache hits/misses...

What makes this code interrupt safe ?

Having a per-cpu list is not interrupt safe without special care.