Re: [PATCH] net: nfc: fix deadlock between nfc_unregister_device and rfkill_fop_write

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/12/2025 06:49, Deepanshu Kartikey wrote:
> A deadlock can occur between nfc_unregister_device() and rfkill_fop_write()
> due to lock ordering inversion between device_lock and rfkill_global_mutex.
> 
> The problematic lock order is:
> 
> Thread A (rfkill_fop_write):
>   rfkill_fop_write()
>     mutex_lock(&rfkill_global_mutex)
>       rfkill_set_block()
>         nfc_rfkill_set_block()
>           nfc_dev_down()
>             device_lock(&dev->dev)    <- waits for device_lock
> 
> Thread B (nfc_unregister_device):
>   nfc_unregister_device()
>     device_lock(&dev->dev)
>       rfkill_unregister()
>         mutex_lock(&rfkill_global_mutex)  <- waits for rfkill_global_mutex
> 
> This creates a classic ABBA deadlock scenario.
> 
> Fix this by moving rfkill_unregister() and rfkill_destroy() outside the
> device_lock critical section. To ensure safety, set shutting_down flag
> first and store rfkill pointer in a local variable before releasing the
> lock. The shutting_down flag ensures that nfc_dev_down() and nfc_dev_up()
> will bail out early if called during device unregistration.
> 
> Reported-by: syzbot+4ef89409a235d804c6c2@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4ef89409a235d804c6c2

You need Fixes and CC-stable tags.

So this was introduced by previous fix from Lin Ma... All these random
drive-bys by various people based on syzbot are just patching buggy code
with more buggy code. :/ No one cares too look more than just the syzbot
report.

And you as well. If you fix ABBA here, you should look and fix in other
places as well. There is exactly same order of locks in
nfc_register_device(). That's the register path which should not be a
problem, maybe except false LOCKDEP positives, but you should explain
that in commit msg why leaving ABBA there is safe.




> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  net/nfc/core.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index ae1c842f9c64..201d2b95432b 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -1154,7 +1154,7 @@ EXPORT_SYMBOL(nfc_register_device);
>  void nfc_unregister_device(struct nfc_dev *dev)
>  {
>  	int rc;
> -

Do not remove blank line.

> +	struct rfkill *rfk = NULL;
>  	pr_debug("dev_name=%s\n", dev_name(&dev->dev));
>  
>  	rc = nfc_genl_device_removed(dev);
> @@ -1164,13 +1164,17 @@ void nfc_unregister_device(struct nfc_dev *dev)
>  
>  	device_lock(&dev->dev);
>  	if (dev->rfkill) {
> -		rfkill_unregister(dev->rfkill);
> -		rfkill_destroy(dev->rfkill);
> +		rfk = dev->rfkill;
>  		dev->rfkill = NULL;
>  	}
>  	dev->shutting_down = true;
>  	device_unlock(&dev->dev);

1. So your code allows concurrent thread nfc_rfkill_set_block() to be
called at this spot
2. Original thread of unregistering will shortly later call
device_del(), which goes through lock+kill+unlock,
3. Then the concurrent thread proceeds to device_lock() and all other
things with freed device.

You just replaced one issue with another issue, right?

Best regards,
Krzysztof