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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.dumazet@gmail.com>
>> Sent: Monday, August 31, 2020 3:15 PM
>> To: Tuong Tong Lien <tuong.t.lien@dektech.com.au>; 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:
>>> 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.
> 
>>
>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>>> Signed-off-by: Tuong Lien <tuong.t.lien@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.

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.