Re: [PATCH] net: nfc: fix deadlock between nfc_unregister_device and rfkill_fop_write
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2025-12-17 07:31:49
Also in:
lkml
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=4ef89409a235d804c6c2You 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.
quoted hunk ↗ jump to hunk
Signed-off-by: Deepanshu Kartikey <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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