Re: [PATCH net] nfc: st-nci: drain se_info timers on remove before NCI teardown

[Simon Horman <horms@kernel.org>]

On Wed, Jul 22, 2026 at 03:47:29AM +0000, Fan Wu wrote:
> st_nci_se_init() initializes two se_info timers (bwi_timer and
> se_active_timer) in probe, but st_nci_se_deinit(), the teardown helper
> that drains both timers, has no caller in drivers/nfc/st-nci/:
> st_nci_remove() tears the device down with ndlc_close /
> nci_unregister_device / nci_free_device and never reaches it (dead since
> the st21nfcb -> st-nci rename). The bwi_timer callback
> st_nci_se_wt_timeout() calls nci_hci_send_event(), i.e. it uses the NCI
> core. Both callbacks dereference info->se_info, and the bwi_timer callback
> also dereferences info->ndlc->ndev.
> info is devm_kzalloc()'d, so a timer armed during operation survives
> st_nci_remove, fires after the devm release, and dereferences freed
> memory (and a half-torn-down NCI core).
> 
> Call st_nci_se_deinit(ndev) from st_nci_remove() before
> nci_unregister_device(ndev): the bwi callback reaches into the NCI core,
> so the timers must be drained while the NCI workqueues still exist.
> Convert both drains to timer_shutdown_sync() and drop the bwi_active /
> se_active predicate guards. timer_shutdown_sync() is harmless for an
> inactive timer and prevents later mod_timer() calls from rearming it, so
> any residual HCI work during nci_unregister_device()'s workqueue teardown
> cannot rearm a se_info timer.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: ed06aeefdac3 ("nfc: st-nci: Rename st21nfcb to st-nci")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

Hi Fan,

The AI-generated review of this patch on sashiko.dev [1]
flags several issues. I'm passing on those marked as pre-existing
there, as I think they are best considered in the context of
possible follow-up. But I am wondering if you could look at
the ones I've pasted in-line below.

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

> ---
>  drivers/nfc/st-nci/core.c | 1 +
>  drivers/nfc/st-nci/se.c   | 6 ++----
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nfc/st-nci/core.c b/drivers/nfc/st-nci/core.c
> index a367136..57dbc4a 100644
> --- a/drivers/nfc/st-nci/core.c
> +++ b/drivers/nfc/st-nci/core.c
> @@ -166,6 +166,7 @@ void st_nci_remove(struct nci_dev *ndev)
>  
>  	ndlc_close(info->ndlc);
>  
> +	st_nci_se_deinit(ndev);
>  	nci_unregister_device(ndev);
>  	nci_free_device(ndev);
>  }

  "Can moving st_nci_se_deinit() before nci_unregister_device() cause a
   deadlock with userspace operations?

  "If a userspace process initiates an SE command via netlink, it acquires
   device_lock and waits in st_nci_control_se() for the se_active_timer
   completion.

  "Concurrently, if device removal executes st_nci_remove(), it calls
   st_nci_se_deinit() which shuts down the timer without signaling
   completion. The netlink thread will hang indefinitely holding device_lock.

  "When st_nci_remove() proceeds to nci_unregister_device(), it calls
   nfc_unregister_rfkill(), which attempts to acquire the same device_lock.
   Would this result in an ABBA deadlock?

> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
> index 607ec76..8ab18cb 100644
> --- a/drivers/nfc/st-nci/se.c
> +++ b/drivers/nfc/st-nci/se.c
> @@ -751,10 +751,8 @@ void st_nci_se_deinit(struct nci_dev *ndev)
>  {
>  	struct st_nci_info *info = nci_get_drvdata(ndev);
>  
> -	if (info->se_info.bwi_active)
> -		timer_delete_sync(&info->se_info.bwi_timer);
> -	if (info->se_info.se_active)
> -		timer_delete_sync(&info->se_info.se_active_timer);
> +	timer_shutdown_sync(&info->se_info.bwi_timer);
> +	timer_shutdown_sync(&info->se_info.se_active_timer);

  "Does unconditionally shutting down bwi_timer here leak the se_io_ctx
   and hang the netlink socket?

  "Because the PHY is disabled just prior by ndlc_close(), no hardware
   response can arrive. The timeout callback st_nci_se_wt_timeout() would
   normally free se_io_ctx via info->se_info.cb() and reply to the socket.

  "By shutting down the timer without resolving pending I/O callbacks, are
   we leaking this memory and leaving the netlink caller waiting indefinitely?

>  
>  	info->se_info.se_active = false; info->se_info.bwi_active = false;
>  	-- 2.34.1
>