> -----Original Message----- > From: Eric Dumazet <[email protected]> > Sent: Monday, August 31, 2020 7:48 PM > To: Tuong Tong Lien <[email protected]>; Eric Dumazet > <[email protected]>; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected] > Cc: [email protected] > 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 <[email protected]> > >> Sent: Monday, August 31, 2020 4:48 PM > >> To: Tuong Tong Lien <[email protected]>; Eric Dumazet > >> <[email protected]>; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected] > >> Cc: [email protected] > >> 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 <[email protected]> > >>>> Sent: Monday, August 31, 2020 3:15 PM > >>>> To: Tuong Tong Lien <[email protected]>; [email protected]; > >>>> [email protected]; [email protected]; > >>>> [email protected]; [email protected] > >>>> Cc: [email protected] > >>>> 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: > >>>> [email protected]' ? > >>> Well, really I detected the issue during my testing instead, didn't know > >>> if it was reported by syzbot too. > >>> > >>>> > >>>>> Acked-by: Jon Maloy <[email protected]> > >>>>> Signed-off-by: Tuong Lien <[email protected]> > >>>>> --- > >>>>> 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. > > _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
