Re: [PATCH net] nfc: trf7970a: drain timeout_work and keep trf->lock valid across teardown

[Simon Horman]

On Tue, Jul 21, 2026 at 01:35:39PM +0000, Fan Wu wrote:
> The threaded IRQ handler and timeout work can wait on trf->lock while
> teardown destroys it. The timeout handler can also access the digital
> device after it has been freed.
> 
> Synchronize timeout_work before releasing the digital device, and leave
> the embedded mutex alive for devres cleanup to synchronize the IRQ
> handler before trf is freed: devm_request_threaded_irq() is registered
> after devm_kzalloc(), so devm_free_irq() runs before trf is released.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 165063f1dac4 ("NFC: trf7970a: Add driver with ISO/IEC 14443 Type 2 Tag Support")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> ---
>  drivers/nfc/trf7970a.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nfc/trf7970a.c b/drivers/nfc/trf7970a.c
> index d17c701..fa2f75a 100644
> --- a/drivers/nfc/trf7970a.c
> +++ b/drivers/nfc/trf7970a.c
> @@ -2206,13 +2206,13 @@ static int trf7970a_probe(struct spi_device *spi)
>  err_shutdown:
>  	trf7970a_shutdown(trf);
>  err_free_ddev:
> +	cancel_delayed_work_sync(&trf->timeout_work);
>  	nfc_digital_free_device(trf->ddev);
>  err_disable_vddio_regulator:
>  	regulator_disable(trf->vddio_regulator);
>  err_disable_vin_regulator:
>  	regulator_disable(trf->vin_regulator);
>  err_destroy_lock:
> -	mutex_destroy(&trf->lock);
>  	return ret;
>  }
>  
> @@ -2226,13 +2226,13 @@ static void trf7970a_remove(struct spi_device *spi)
>  
>  	mutex_unlock(&trf->lock);
>  
> +	cancel_delayed_work_sync(&trf->timeout_work);
> +
>  	nfc_digital_unregister_device(trf->ddev);
>  	nfc_digital_free_device(trf->ddev);

Hi Fan,

The AI-generated review of this patch on sashiko.dev [1] flags a number of
issues. Most of them seem to be pre-existing and I think those ones
can be treated in the context of possible follow-up.

But this one stands out as directly effecting this ptach:

  "Is it safe to cancel the delayed work before unregistering the device?

  "If concurrent operations from userspace (like bringing the interface up)
   happen while the device is still registered, they can execute
   trf7970a_switch_rf() and trf7970a_send_cmd(), which would re-arm the
   timeout work after we have already cancelled it.

  "When the re-armed work executes after we free trf->ddev below, won't
   this lead to a use-after-free? Should nfc_digital_unregister_device() be
   called before the work is cancelled?

[1] https://sashiko.dev/#/patchset/20260721133539.3195899-1-fanwu01%40zju.edu.cn

>  
>  	regulator_disable(trf->vddio_regulator);
>  	regulator_disable(trf->vin_regulator);
> -
> -	mutex_destroy(&trf->lock);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> -- 
> 2.34.1
>