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.
BR/Tuong
>
> > list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
> > crypto_free_aead(tfm_entry->tfm);
> > list_del(&tfm_entry->list);
> > @@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead
> > __rcu *aead, int val)
> > */
> > static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
> > {
> > - struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
> > + struct tipc_tfm **tfm_entry;
> > + struct crypto_aead *tfm;
> >
> > + tfm_entry = get_cpu_ptr(aead->tfm_entry);
> > *tfm_entry = list_next_entry(*tfm_entry, list);
> > - return (*tfm_entry)->tfm;
> > + tfm = (*tfm_entry)->tfm;
> > + put_cpu_ptr(tfm_entry);
>
> Again, this looks suspicious to me. I can not explain why this would be safe.
>
> > +
> > + return tfm;
> > }
> >
> > /**
> >
