> -----Original Message-----
> From: Eric Dumazet <eric.duma...@gmail.com>
> Sent: Monday, August 31, 2020 4:48 PM
> To: Tuong Tong Lien <tuong.t.l...@dektech.com.au>; Eric Dumazet
> <eric.duma...@gmail.com>; da...@davemloft.net;
> jma...@redhat.com; ma...@donjonn.com; ying....@windriver.com;
> netdev@vger.kernel.org
> Cc: tipc-discuss...@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:
> > Hi Eric,
> >
> > Thanks for your comments, please see my answers inline.
> >
> >> -----Original Message-----
> >> From: Eric Dumazet <eric.duma...@gmail.com>
> >> Sent: Monday, August 31, 2020 3:15 PM
> >> To: Tuong Tong Lien <tuong.t.l...@dektech.com.au>; da...@davemloft.net;
> >> jma...@redhat.com; ma...@donjonn.com;
> >> ying....@windriver.com; netdev@vger.kernel.org
> >> Cc: tipc-discuss...@lists.sourceforge.net
> >> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
> >>
> >>
> >>
> >> On 8/29/20 12:37 PM, Tuong Lien wrote:
> >>> 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+263f8c0d007dc09b2...@syzkaller.appspotmail.com' ?
> > Well, really I detected the issue during my testing instead, didn't know if
> > it was reported by syzbot too.
> >
> >>
> >>> Acked-by: Jon Maloy <jma...@redhat.com>
> >>> Signed-off-by: Tuong Lien <tuong.t.l...@dektech.com.au>
> >>> ---
> >>> 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...
BR/Tuong
>
> 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.
>
>
>
>
