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

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

On 9/1/20 5:18 AM, Tuong Tong Lien wrote:
> 
> 
>> -----Original Message-----
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Sent: Monday, August 31, 2020 7:48 PM
>> To: Tuong Tong Lien <tuong.t.lien@dektech.com.au>; Eric Dumazet <eric.dumazet@gmail.com>; 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/31/20 3:05 AM, Tuong Tong Lien wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Sent: Monday, August 31, 2020 4:48 PM
>>>> To: Tuong Tong Lien <tuong.t.lien@dektech.com.au>; Eric Dumazet <eric.dumazet@gmail.com>; 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/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.
>>> 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...

tipc_aead_tfm_next() is _not_ read-only, since it contains :

*tfm_entry = list_next_entry(*tfm_entry, list);

If tipc_aead_tfm_next() can be called both from process context and irq context,
using a percpu variable to track a cursor in a list is unsafe.

_Unless_ special care is taken by callers to make sure irqs are disabled.

RCU does not protect this, not sure why you mention RCU at all.

To be re-entrant, each thread should have its own cursor, usually stored in an automatic variable,
not in a per-cpu location.