[PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[Duoming Zhou]

There are sleep in atomic context bugs when the request to secure
element of st21nfca is timeout. The root cause is that kzalloc and
alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
which is a timer handler. The call tree shows the execution paths that
could lead to bugs:

   (Interrupt context)
st21nfca_se_wt_timeout
  nfc_hci_send_event
    nfc_hci_hcp_message_tx
      kzalloc(..., GFP_KERNEL) //may sleep
      alloc_skb(..., GFP_KERNEL) //may sleep

This patch changes allocation mode of kzalloc and alloc_skb from
GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
sleeping. The GFP_ATOMIC flag makes memory allocation operation
could be used in atomic context.

Fixes: 8b8d2e08bf0d ("NFC: HCI support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 net/nfc/hci/hcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
index 05c60988f59..1caf9c2086f 100644
--- a/net/nfc/hci/hcp.c
+++ b/net/nfc/hci/hcp.c
@@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
 	int hci_len, err;
 	bool firstfrag = true;
 
-	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
+	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
 	if (cmd == NULL)
 		return -ENOMEM;
 
@@ -58,7 +58,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
 			  data_link_len + ndev->tx_tailroom;
 		hci_len -= data_link_len;
 
-		skb = alloc_skb(skb_len, GFP_KERNEL);
+		skb = alloc_skb(skb_len, GFP_ATOMIC);
 		if (skb == NULL) {
 			err = -ENOMEM;
 			goto out_skb_err;
-- 
2.17.1

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[Krzysztof Kozlowski]

On 16/05/2022 04:10, Duoming Zhou wrote:
> There are sleep in atomic context bugs when the request to secure
> element of st21nfca is timeout. The root cause is that kzalloc and
> alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
> which is a timer handler. The call tree shows the execution paths that
> could lead to bugs:
> 
>    (Interrupt context)
> st21nfca_se_wt_timeout
>   nfc_hci_send_event
>     nfc_hci_hcp_message_tx
>       kzalloc(..., GFP_KERNEL) //may sleep
>       alloc_skb(..., GFP_KERNEL) //may sleep
> 
> This patch changes allocation mode of kzalloc and alloc_skb from
> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
> sleeping. The GFP_ATOMIC flag makes memory allocation operation
> could be used in atomic context.
> 
> Fixes: 8b8d2e08bf0d ("NFC: HCI support")
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>  net/nfc/hci/hcp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
> index 05c60988f59..1caf9c2086f 100644
> --- a/net/nfc/hci/hcp.c
> +++ b/net/nfc/hci/hcp.c
> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
>  	int hci_len, err;
>  	bool firstfrag = true;
>  
> -	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
> +	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);

No, this does not look correct. This function can sleep, so it can use
GFP_KERNEL. Please just look at the function before replacing any flags...



Best regards,
Krzysztof

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[duoming]

Hello,

On Mon, 16 May 2022 08:44:39 +0200 Krzysztof wrote:

> > There are sleep in atomic context bugs when the request to secure
> > element of st21nfca is timeout. The root cause is that kzalloc and
> > alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
> > which is a timer handler. The call tree shows the execution paths that
> > could lead to bugs:
> > 
> >    (Interrupt context)
> > st21nfca_se_wt_timeout
> >   nfc_hci_send_event
> >     nfc_hci_hcp_message_tx
> >       kzalloc(..., GFP_KERNEL) //may sleep
> >       alloc_skb(..., GFP_KERNEL) //may sleep
> > 
> > This patch changes allocation mode of kzalloc and alloc_skb from
> > GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
> > sleeping. The GFP_ATOMIC flag makes memory allocation operation
> > could be used in atomic context.
> > 
> > Fixes: 8b8d2e08bf0d ("NFC: HCI support")
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> >  net/nfc/hci/hcp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
> > index 05c60988f59..1caf9c2086f 100644
> > --- a/net/nfc/hci/hcp.c
> > +++ b/net/nfc/hci/hcp.c
> > @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
> >  	int hci_len, err;
> >  	bool firstfrag = true;
> >  
> > -	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
> > +	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
> 
> No, this does not look correct. This function can sleep, so it can use
> GFP_KERNEL. Please just look at the function before replacing any flags...

I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep.

I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context.
The call tree shows the execution paths that could lead to bugs:

st21nfca_hci_se_io()
  mod_timer(&info->se_info.bwi_timer,...)
    st21nfca_se_wt_timeout()  //timer handler, interrupt context
      nfc_hci_send_event()
        nfc_hci_hcp_message_tx()
          kzalloc(..., GFP_KERNEL) //may sleep
          alloc_skb(..., GFP_KERNEL) //may sleep

What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx()
is also improper.

Please correct me, If you think I am wrong. Thanks for your time.

Best regards,
Duoming Zhou

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[Krzysztof Kozlowski]

On 16/05/2022 12:18, duoming@zju.edu.cn wrote:
> Hello,
> 
> On Mon, 16 May 2022 08:44:39 +0200 Krzysztof wrote:
> 
>>> There are sleep in atomic context bugs when the request to secure
>>> element of st21nfca is timeout. The root cause is that kzalloc and
>>> alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
>>> which is a timer handler. The call tree shows the execution paths that
>>> could lead to bugs:
>>>
>>>    (Interrupt context)
>>> st21nfca_se_wt_timeout
>>>   nfc_hci_send_event
>>>     nfc_hci_hcp_message_tx
>>>       kzalloc(..., GFP_KERNEL) //may sleep
>>>       alloc_skb(..., GFP_KERNEL) //may sleep
>>>
>>> This patch changes allocation mode of kzalloc and alloc_skb from
>>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
>>> sleeping. The GFP_ATOMIC flag makes memory allocation operation
>>> could be used in atomic context.
>>>
>>> Fixes: 8b8d2e08bf0d ("NFC: HCI support")
>>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
>>> ---
>>>  net/nfc/hci/hcp.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
>>> index 05c60988f59..1caf9c2086f 100644
>>> --- a/net/nfc/hci/hcp.c
>>> +++ b/net/nfc/hci/hcp.c
>>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
>>>  	int hci_len, err;
>>>  	bool firstfrag = true;
>>>  
>>> -	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
>>> +	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
>>
>> No, this does not look correct. This function can sleep, so it can use
>> GFP_KERNEL. Please just look at the function before replacing any flags...
> 
> I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep.

Why? because in line 93 it uses a mutex (which is a sleeping primitve).

> 
> I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context.
> The call tree shows the execution paths that could lead to bugs:
> 
> st21nfca_hci_se_io()
>   mod_timer(&info->se_info.bwi_timer,...)
>     st21nfca_se_wt_timeout()  //timer handler, interrupt context
>       nfc_hci_send_event()
>         nfc_hci_hcp_message_tx()
>           kzalloc(..., GFP_KERNEL) //may sleep
>           alloc_skb(..., GFP_KERNEL) //may sleep
> 
> What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx()
> is also improper.
> 
> Please correct me, If you think I am wrong. Thanks for your time.

Your patch is not correct in current semantics of this function. This
function can sleep (because it uses a mutex), so the mistake is rather
calling it from interrupt context.


Best regards,
Krzysztof

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[duoming]

Hello,

On Mon, 16 May 2022 12:43:07 +0200 Krzysztof wrote:

> >>> There are sleep in atomic context bugs when the request to secure
> >>> element of st21nfca is timeout. The root cause is that kzalloc and
> >>> alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
> >>> which is a timer handler. The call tree shows the execution paths that
> >>> could lead to bugs:
> >>>
> >>>    (Interrupt context)
> >>> st21nfca_se_wt_timeout
> >>>   nfc_hci_send_event
> >>>     nfc_hci_hcp_message_tx
> >>>       kzalloc(..., GFP_KERNEL) //may sleep
> >>>       alloc_skb(..., GFP_KERNEL) //may sleep
> >>>
> >>> This patch changes allocation mode of kzalloc and alloc_skb from
> >>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
> >>> sleeping. The GFP_ATOMIC flag makes memory allocation operation
> >>> could be used in atomic context.
> >>>
> >>> Fixes: 8b8d2e08bf0d ("NFC: HCI support")
> >>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> >>> ---
> >>>  net/nfc/hci/hcp.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
> >>> index 05c60988f59..1caf9c2086f 100644
> >>> --- a/net/nfc/hci/hcp.c
> >>> +++ b/net/nfc/hci/hcp.c
> >>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
> >>>  	int hci_len, err;
> >>>  	bool firstfrag = true;
> >>>  
> >>> -	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
> >>> +	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
> >>
> >> No, this does not look correct. This function can sleep, so it can use
> >> GFP_KERNEL. Please just look at the function before replacing any flags...
> > 
> > I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep.
> 
> Why? because in line 93 it uses a mutex (which is a sleeping primitve).
> 
> > 
> > I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context.
> > The call tree shows the execution paths that could lead to bugs:
> > 
> > st21nfca_hci_se_io()
> >   mod_timer(&info->se_info.bwi_timer,...)
> >     st21nfca_se_wt_timeout()  //timer handler, interrupt context
> >       nfc_hci_send_event()
> >         nfc_hci_hcp_message_tx()
> >           kzalloc(..., GFP_KERNEL) //may sleep
> >           alloc_skb(..., GFP_KERNEL) //may sleep
> > 
> > What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx()
> > is also improper.
> > 
> > Please correct me, If you think I am wrong. Thanks for your time.
> 
> Your patch is not correct in current semantics of this function. This
> function can sleep (because it uses a mutex), so the mistake is rather
> calling it from interrupt context.

We have to call nfc_hci_send_event() in st21nfca_se_wt_timeout(), because we need to send 
a reset request as recovery procedure. I think replace GFP_KERNEL to GFP_ATOMIC and replace
mutex_lock to spin_lock in nfc_hci_hcp_message_tx() is better.

What's more, in order to synchronize with other functions related with hci message TX, 
We need to replace the mutex_lock(&hdev->msg_tx_mutex) to spin_lock in other functions
as well. I sent "patch v2" just now.

Best regards,
Duoming Zhou

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[Krzysztof Kozlowski]

On 17/05/2022 12:56, duoming@zju.edu.cn wrote:
> Hello,
> 
> On Mon, 16 May 2022 12:43:07 +0200 Krzysztof wrote:
> 
>>>>> There are sleep in atomic context bugs when the request to secure
>>>>> element of st21nfca is timeout. The root cause is that kzalloc and
>>>>> alloc_skb with GFP_KERNEL parameter is called in st21nfca_se_wt_timeout
>>>>> which is a timer handler. The call tree shows the execution paths that
>>>>> could lead to bugs:
>>>>>
>>>>>    (Interrupt context)
>>>>> st21nfca_se_wt_timeout
>>>>>   nfc_hci_send_event
>>>>>     nfc_hci_hcp_message_tx
>>>>>       kzalloc(..., GFP_KERNEL) //may sleep
>>>>>       alloc_skb(..., GFP_KERNEL) //may sleep
>>>>>
>>>>> This patch changes allocation mode of kzalloc and alloc_skb from
>>>>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from
>>>>> sleeping. The GFP_ATOMIC flag makes memory allocation operation
>>>>> could be used in atomic context.
>>>>>
>>>>> Fixes: 8b8d2e08bf0d ("NFC: HCI support")
>>>>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
>>>>> ---
>>>>>  net/nfc/hci/hcp.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c
>>>>> index 05c60988f59..1caf9c2086f 100644
>>>>> --- a/net/nfc/hci/hcp.c
>>>>> +++ b/net/nfc/hci/hcp.c
>>>>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe,
>>>>>  	int hci_len, err;
>>>>>  	bool firstfrag = true;
>>>>>  
>>>>> -	cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL);
>>>>> +	cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC);
>>>>
>>>> No, this does not look correct. This function can sleep, so it can use
>>>> GFP_KERNEL. Please just look at the function before replacing any flags...
>>>
>>> I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep.
>>
>> Why? because in line 93 it uses a mutex (which is a sleeping primitve).
>>
>>>
>>> I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context.
>>> The call tree shows the execution paths that could lead to bugs:
>>>
>>> st21nfca_hci_se_io()
>>>   mod_timer(&info->se_info.bwi_timer,...)
>>>     st21nfca_se_wt_timeout()  //timer handler, interrupt context
>>>       nfc_hci_send_event()
>>>         nfc_hci_hcp_message_tx()
>>>           kzalloc(..., GFP_KERNEL) //may sleep
>>>           alloc_skb(..., GFP_KERNEL) //may sleep
>>>
>>> What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx()
>>> is also improper.
>>>
>>> Please correct me, If you think I am wrong. Thanks for your time.
>>
>> Your patch is not correct in current semantics of this function. This
>> function can sleep (because it uses a mutex), so the mistake is rather
>> calling it from interrupt context.
> 
> We have to call nfc_hci_send_event() in st21nfca_se_wt_timeout(), because we need to send 
> a reset request as recovery procedure. I think replace GFP_KERNEL to GFP_ATOMIC and replace
> mutex_lock to spin_lock in nfc_hci_hcp_message_tx() is better.
> 
> What's more, in order to synchronize with other functions related with hci message TX, 
> We need to replace the mutex_lock(&hdev->msg_tx_mutex) to spin_lock in other functions
> as well. I sent "patch v2" just now.

You sent v2 one minute before replying here... that's not how discussion
work. Please do not sent next version before reaching some consensus.


Best regards,
Krzysztof

Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[duoming]

Hello,

On Tue, 17 May 2022 13:39:44 +0200 wrote:

> You sent v2 one minute before replying here... that's not how discussion
> work. Please do not sent next version before reaching some consensus.

I am sorry. Before reaching some consensus, I will not send next version in the future.
Thanks for your guidance.

Best regards,
Duoming Zhou