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

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

[Tuong Lien]

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")
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);
 		list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
 			crypto_free_aead(tfm_entry->tfm);
 			list_del(&tfm_entry->list);
@@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val)
  */
 static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
 {
-	struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
+	struct tipc_tfm **tfm_entry;
+	struct crypto_aead *tfm;
 
+	tfm_entry = get_cpu_ptr(aead->tfm_entry);
 	*tfm_entry = list_next_entry(*tfm_entry, list);
-	return (*tfm_entry)->tfm;
+	tfm = (*tfm_entry)->tfm;
+	put_cpu_ptr(tfm_entry);
+
+	return tfm;
 }
 
 /**
-- 
2.26.2

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

[David Miller]

From: Tuong Lien <tuong.t.lien@dektech.com.au>
Date: Sun, 30 Aug 2020 02:37:55 +0700

> 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")
> Acked-by: Jon Maloy <jmaloy@redhat.com>
> Signed-off-by: Tuong Lien <tuong.t.lien@dektech.com.au>

Applied and queued up for -stable.

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

[Eric Dumazet]

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' ?

> 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.

>  		list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
>  			crypto_free_aead(tfm_entry->tfm);
>  			list_del(&tfm_entry->list);
> @@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val)
>   */
>  static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
>  {
> -	struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
> +	struct tipc_tfm **tfm_entry;
> +	struct crypto_aead *tfm;
>  
> +	tfm_entry = get_cpu_ptr(aead->tfm_entry);
>  	*tfm_entry = list_next_entry(*tfm_entry, list);
> -	return (*tfm_entry)->tfm;
> +	tfm = (*tfm_entry)->tfm;
> +	put_cpu_ptr(tfm_entry);

Again, this looks suspicious to me. I can not explain why this would be safe.

> +
> +	return tfm;
>  }
>  
>  /**
> 

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

[Tuong Tong Lien]

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.

BR/Tuong
> 
> >  		list_for_each_entry_safe(tfm_entry, tmp, &head->list, list) {
> >  			crypto_free_aead(tfm_entry->tfm);
> >  			list_del(&tfm_entry->list);
> > @@ -399,10 +400,15 @@ static void tipc_aead_users_set(struct tipc_aead __rcu *aead, int val)
> >   */
> >  static struct crypto_aead *tipc_aead_tfm_next(struct tipc_aead *aead)
> >  {
> > -	struct tipc_tfm **tfm_entry = this_cpu_ptr(aead->tfm_entry);
> > +	struct tipc_tfm **tfm_entry;
> > +	struct crypto_aead *tfm;
> >
> > +	tfm_entry = get_cpu_ptr(aead->tfm_entry);
> >  	*tfm_entry = list_next_entry(*tfm_entry, list);
> > -	return (*tfm_entry)->tfm;
> > +	tfm = (*tfm_entry)->tfm;
> > +	put_cpu_ptr(tfm_entry);
> 
> Again, this looks suspicious to me. I can not explain why this would be safe.
> 
> > +
> > +	return tfm;
> >  }
> >
> >  /**
> >

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

[Eric Dumazet]

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.

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

[Tuong Tong Lien]

> -----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...

BR/Tuong
> 
> 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.
> 
> 
> 
> 

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

[Eric Dumazet]

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 ?

Having a per-cpu list is not interrupt safe without special care.

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

[Tuong Tong Lien]

> -----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...

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

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

[Eric Dumazet]

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.

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

[Tuong Tong Lien]

> -----Original Message-----
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Sent: Tuesday, September 1, 2020 8:15 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 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.
Ok, I've got your concern now. Actually when writing this code, I had the same thought as you, but decided to relax it because of the following reasons:
1. I don't want to use any locking methods here that can lead to competition (thus affect overall performance...);
2. The list is not an usual list but a fixed "ring" of persistent elements (no one will insert/remove any element after it is created);
3. It does _not_ matter at all if the function calls will result in the same element, or one call points to the 1st element while another at the same time points to the 3rd one, etc. as long as it returns an element in the list. Also, the per-cpu pointer is _not_ required to exactly point to the next element, but needs to be moved on this or next time..., so just relaxing!
4. Isn't a "write" to the per-cpu variable atomic?

> 
> _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.
Sorry, I went further than necessary...

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

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

[Eric Dumazet]

On 9/1/20 10:52 AM, Tuong Tong Lien wrote:

> Ok, I've got your concern now. Actually when writing this code, I had the same thought as you, but decided to relax it because of the following reasons:
> 1. I don't want to use any locking methods here that can lead to competition (thus affect overall performance...);
> 2. The list is not an usual list but a fixed "ring" of persistent elements (no one will insert/remove any element after it is created);
> 3. It does _not_ matter at all if the function calls will result in the same element, or one call points to the 1st element while another at the same time points to the 3rd one, etc. as long as it returns an element in the list. Also, the per-cpu pointer is _not_ required to exactly point to the next element, but needs to be moved on this or next time..., so just relaxing!
> 4. Isn't a "write" to the per-cpu variable atomic?
> 

I think I will give up, this code is clearly racy, and will consider TIPC as broken.

Your patch only silenced syzbot report, but the bug is still there.

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

[Tuong Tong Lien]

> -----Original Message-----
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Sent: Wednesday, September 2, 2020 2:11 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 9/1/20 10:52 AM, Tuong Tong Lien wrote:
> 
> > Ok, I've got your concern now. Actually when writing this code, I had the same thought as you, but decided to relax it because of the
> following reasons:
> > 1. I don't want to use any locking methods here that can lead to competition (thus affect overall performance...);
> > 2. The list is not an usual list but a fixed "ring" of persistent elements (no one will insert/remove any element after it is created);
> > 3. It does _not_ matter at all if the function calls will result in the same element, or one call points to the 1st element while another
> at the same time points to the 3rd one, etc. as long as it returns an element in the list. Also, the per-cpu pointer is _not_ required to
> exactly point to the next element, but needs to be moved on this or next time..., so just relaxing!
> > 4. Isn't a "write" to the per-cpu variable atomic?
> >
> 
> I think I will give up, this code is clearly racy, and will consider TIPC as broken.
> 
> Your patch only silenced syzbot report, but the bug is still there.
Hi Eric,
Sorry but could you please tell me why you think it is "racy"... and the bug is still there...? Thanks!
I agreed we could make it in some brighter ways, but for now by disabling preemption prior to the per-cpu variable access is fine enough? Also lets say even in case the code is interrupted by BH or interrupts..., we should have no issue.

BR/Tuong
>