Re: [tipc-discussion] [net] tipc: fix using smp_processor_id() in preemptible

[Tuong T. L. <tuo...@de...>]

> -----Original Message-----
> From: Eric Dumazet <eri...@gm...>
> Sent: Monday, August 31, 2020 7:48 PM
> To: Tuong Tong Lien <tuo...@de...>; Eric Dumazet <eri...@gm...>; da...@da...;
> jm...@re...; ma...@do...; yin...@wi...; ne...@vg...
> Cc: tip...@li...
> Subject: Re: [net] tipc: fix using smp_processor_id() in preemptible
> 
> 
> 
> On 8/31/20 3:05 AM, Tuong Tong Lien wrote:
> >
> >
> >> -----Original Message-----
> >> From: Eric Dumazet <eri...@gm...>
> >> Sent: Monday, August 31, 2020 4:48 PM
> >> To: Tuong Tong Lien <tuo...@de...>; Eric Dumazet <eri...@gm...>; da...@da...;
> >> jm...@re...; ma...@do...; yin...@wi...; ne...@vg...
> >> Cc: tip...@li...
> >> 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 <eri...@gm...>
> >>>> Sent: Monday, August 31, 2020 3:15 PM
> >>>> To: Tuong Tong Lien <tuo...@de...>; da...@da...; jm...@re...; ma...@do...;
> >>>> yin...@wi...; ne...@vg...
> >>>> Cc: tip...@li...
> >>>> 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: syz...@sy...' ?
> >>> Well, really I detected the issue during my testing instead, didn't know if it was reported by syzbot too.
> >>>
> >>>>
> >>>>> Acked-by: Jon Maloy <jm...@re...>
> >>>>> Signed-off-by: Tuong Lien <tuo...@de...>
> >>>>> ---
> >>>>>  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 ?
> 
Why is it unsafe? Its "parent" object is already managed by RCU mechanism. Also, it is never modified but just "read-only" in all cases...

BR/Tuong
> Having a per-cpu list is not interrupt safe without special care.
> 
>