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

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.

Why no lock is required ? This seems hard to believe, given lack of clear explanations anywhere
in commit fc1b6d6de220 ("tipc: introduce TIPC encryption & authentication").

If the object can be used without locking, it should be marked const.

tipc_aead_tfm_next() has side effects that I really can not understand in SMP world,
and presumably with soft interrupts in UP as well.