diff mbox series

nfc/nci: fix task hung in nfc_targets_found

Message ID tencent_E44436084AA874977705670A3CDD37BE9609@qq.com
State New
Headers
Series nfc/nci: fix task hung in nfc_targets_found |

Commit Message

Edward Adam Davis Jan. 14, 2024, 8:20 a.m. UTC 
  nci_start_poll() holds the dev->mutex required by the kworker of nci_close_device(),
and the related tasks are as follows:
|cpu0                          |cpu1                                           |cpu2                      |
|nci_close_device()            |                                               |                          |
|mutex_lock(&ndev->req_lock);  |                                               |                          |
|...                           |nfc_genl_start_poll()                          |                          |
|flush_workqueue(ndev->rx_wq)  |mutex_lock(&dev->genl_data.genl_data_mutex);   |                          |
|                              |nfc_start_poll()                               |                          |
|              	               |device_lock(&dev->dev);                        |process_one_work()        |
|                              |nci_start_poll()                               |nfc_targets_found()       |
|                              |nci_request()                                  |device_lock(&dev->dev);   |
|                              |mutex_lock(&ndev->req_lock);                   |                          |

Therefore, before applying for req_lock in nci_request(), it should be determined
whether the execution of nci_close_device() has already begun.

Reported-and-tested-by: syzbot+2b131f51bb4af224ab40@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 net/nfc/nci/core.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Krzysztof Kozlowski Jan. 15, 2024, 9:36 a.m. UTC | #1 
On 14/01/2024 09:20, Edward Adam Davis wrote:
> nci_start_poll() holds the dev->mutex required by the kworker of nci_close_device(),
> and the related tasks are as follows:
> |cpu0                          |cpu1                                           |cpu2                      |
> |nci_close_device()            |                                               |                          |
> |mutex_lock(&ndev->req_lock);  |                                               |                          |
> |...                           |nfc_genl_start_poll()                          |                          |
> |flush_workqueue(ndev->rx_wq)  |mutex_lock(&dev->genl_data.genl_data_mutex);   |                          |
> |                              |nfc_start_poll()                               |                          |
> |              	               |device_lock(&dev->dev);                        |process_one_work()        |
> |                              |nci_start_poll()                               |nfc_targets_found()       |
> |                              |nci_request()                                  |device_lock(&dev->dev);   |
> |                              |mutex_lock(&ndev->req_lock);                   |                          |
> 
> Therefore, before applying for req_lock in nci_request(), it should be determined
> whether the execution of nci_close_device() has already begun.
> 
> Reported-and-tested-by: syzbot+2b131f51bb4af224ab40@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  net/nfc/nci/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 6c9592d05120..9a277228a875 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -145,6 +145,8 @@ inline int nci_request(struct nci_dev *ndev,
>  {
>  	int rc;
>  
> +	if (test_bit(NCI_UNREG, &ndev->flags))
> +		return -ENODEV;

nci_close_device() clears the NCI_UP, which is tested here, just after
acquiring mutex. And there is explicit comment about it just below your
code. Why it is not relevant?

Your code looks really unnecessary, at least with that code flow from
commit msg. Especially considering you do it outside of mutex, so how
does it solve anything?

Best regards,
Krzysztof
Tetsuo Handa Jan. 15, 2024, 11:08 a.m. UTC | #2 
On 2024/01/15 18:36, Krzysztof Kozlowski wrote:
>> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
>> index 6c9592d05120..9a277228a875 100644
>> --- a/net/nfc/nci/core.c
>> +++ b/net/nfc/nci/core.c
>> @@ -145,6 +145,8 @@ inline int nci_request(struct nci_dev *ndev,
>>  {
>>  	int rc;
>>  
>> +	if (test_bit(NCI_UNREG, &ndev->flags))
>> +		return -ENODEV;
> 
> nci_close_device() clears the NCI_UP, which is tested here, just after
> acquiring mutex. And there is explicit comment about it just below your
> code. Why it is not relevant?

Because the deadlock happens at mutex_lock(&ndev->req_lock), which is
before test_bit(NCI_UP, &ndev->flags) is called. Please see
https://lkml.kernel.org/r/d314e471-0251-461e-988d-70add0c6ebf6@I-love.SAKURA.ne.jp .

> 
> Your code looks really unnecessary, at least with that code flow from
> commit msg. Especially considering you do it outside of mutex, so how
> does it solve anything?
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 15, 2024, 11:15 a.m. UTC | #3 
On 15/01/2024 12:08, Tetsuo Handa wrote:
> On 2024/01/15 18:36, Krzysztof Kozlowski wrote:
>>> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
>>> index 6c9592d05120..9a277228a875 100644
>>> --- a/net/nfc/nci/core.c
>>> +++ b/net/nfc/nci/core.c
>>> @@ -145,6 +145,8 @@ inline int nci_request(struct nci_dev *ndev,
>>>  {
>>>  	int rc;
>>>  
>>> +	if (test_bit(NCI_UNREG, &ndev->flags))
>>> +		return -ENODEV;
>>
>> nci_close_device() clears the NCI_UP, which is tested here, just after
>> acquiring mutex. And there is explicit comment about it just below your
>> code. Why it is not relevant?
> 
> Because the deadlock happens at mutex_lock(&ndev->req_lock), which is
> before test_bit(NCI_UP, &ndev->flags) is called. Please see
> https://lkml.kernel.org/r/d314e471-0251-461e-988d-70add0c6ebf6@I-love.SAKURA.ne.jp .

I see, yet still this code looks like moving or copying existing
test_bit(NCI_UP) outside of mutex, which is usually not the correct
solution for deadlocks. First of all, flags are supposed to be
manipulated under lock, so the code here can be quite re-ordered. What
stops the CPU to test negative in above (so: !NCI_UNREG) and then
execute nci_close_device before waiting on mutex here? Nothing.

The problem seems to be locking and solution is not to add one if()
outside of locking.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 6c9592d05120..9a277228a875 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -145,6 +145,8 @@  inline int nci_request(struct nci_dev *ndev,
 {
 	int rc;
 
+	if (test_bit(NCI_UNREG, &ndev->flags))
+		return -ENODEV;
 	/* Serialize all requests */
 	mutex_lock(&ndev->req_lock);
 	/* check the state after obtaing the lock against any races