Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <171c13bb-9fc9-0807-e872-6859dfa2603d@linaro.org>
Date:   Tue, 17 May 2022 13:39:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH net] NFC: hci: fix sleep in atomic context bugs in
 nfc_hci_hcp_message_tx
Content-Language: en-US
To:     duoming@zju.edu.cn
Cc:     linux-kernel@vger.kernel.org, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        gregkh@linuxfoundation.org, alexander.deucher@amd.com,
        broonie@kernel.org, netdev@vger.kernel.org
References: <20220516021028.54063-1-duoming@zju.edu.cn>
 <d5fdfe27-a6de-3030-ce51-9f4f45d552f3@linaro.org>
 <6aba1413.196eb.180cc609bf1.Coremail.duoming@zju.edu.cn>
 <ea2af2f9-002a-5681-4293-a05718ce9dcd@linaro.org>
 <fc6a78c.196ab.180d1a98cc9.Coremail.duoming@zju.edu.cn>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <fc6a78c.196ab.180d1a98cc9.Coremail.duoming@zju.edu.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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