On 8/31/20 3:05 AM, Tuong Tong Lien wrote:
>
>
>> -----Original Message-----
>> From: Eric Dumazet <eric.duma...@gmail.com>
>> Sent: Monday, August 31, 2020 4:48 PM
>> To: Tuong Tong Lien <tuong.t.l...@dektech.com.au>; Eric Dumazet
>> <eric.duma...@gmail.com>; 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/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.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.
>>
>> 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 ?
Having a per-cpu list is not interrupt safe without special care.
