Re: [PATCH] nfc: pn533: free queued command skbs during cleanup

[David Heidelberg]

On 06/07/2026 10:39, raoxu wrote:
> From: Xu Rao <raoxu@xxxxxxxxxxxxx>
>
> pn53x_common_clean() destroys the workqueue and releases the common
> PN533 state. It purges resp_q, but leaves fragment_skb untouched and
> drops queued pn533_cmd objects without freeing their request skbs.
>
> Queued commands are created by __pn533_send_async() when another command
> is pending. At that point the queued command owns cmd->req until it is
> sent or discarded. Fragmented transfers can also leave skbs queued in
> fragment_skb.
>
> Purge fragment_skb and free queued command request skbs before freeing
> the command objects.
>
> Signed-off-by: Xu Rao <raoxu@xxxxxxxxxxxxx>
> ---
>   drivers/nfc/pn533/pn533.c | 2 ++
>   1 file changed, 2 insertions(+)
Hello Xu,

thank you for working on this. Could you append Fixes tag and add Cc for stable?

btw. you may want to also look at Sashiko output below [1].
These issues are preexisting, just in case you wanted to fix more of pn533.


commit 6e3289c813a4d66537faa65e9880657bebff74b1
Author: Xu Rao <raoxu@xxxxxxxxxxxxx>

    nfc: pn533: free queued command skbs during cleanup

    pn53x_common_clean() destroys the workqueue and releases the common
    PN533 state. It purges resp_q, but leaves fragment_skb untouched and
    drops queued pn533_cmd objects without freeing their request skbs.

> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index d7bdbc82e2ba2..de2b7f4d708c6 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c
> @@ -2799,9 +2799,11 @@ void pn53x_common_clean(struct pn533 *priv)
>  	destroy_workqueue(priv->wq);

This isn't a bug introduced by this patch, but is there a use-after-free
risk in the disconnect paths involving this cleanup?

In drivers/nfc/pn533/usb.c:pn533_usb_disconnect(), the core driver state
is freed via pn53x_common_clean() before hardware transport operations are
stopped:

	pn53x_common_clean(phy->priv);
	...
	usb_kill_urb(phy->in_urb);

When usb_kill_urb() cancels the in-flight URB, the completion callback
pn533_recv_ack() executes with -ENOENT and dereferences the already freed
priv pointer to queue work on this destroyed workqueue:

	case -ENOENT:
		dev_dbg(&phy->udev->dev,
			"The urb has been stopped (status %d)\n",
			urb->status);
		goto sched_wq;
	...
	sched_wq:
		queue_work(priv->wq, &priv->cmd_complete_work);

A similar ordering issue appears in
drivers/nfc/pn533/uart.c:pn532_uart_remove() where priv is freed before
timer_shutdown_sync(&pn532->cmd_timeout).

>
>  	skb_queue_purge(&priv->resp_q);
> +	skb_queue_purge(&priv->fragment_skb);
>
>  	list_for_each_entry_safe(cmd, n, &priv->cmd_queue, queue) {
>  		list_del(&cmd->queue);
> +		dev_kfree_skb(cmd->req);
>  		kfree(cmd);

This is a pre-existing issue, but does dropping queued commands here cause
callers to hang?

When commands are removed from the queue and freed, their complete_cb is
never invoked. Callers waiting on wait_for_completion() inside
pn533_send_cmd_sync() could hang forever in TASK_UNINTERRUPTIBLE, and
async callers might leak memory allocated for complete_cb_context.

This is also a pre-existing issue, but does this cleanup routine also need
to free the actively executing command?

If the device is removed while a command is actively in-flight, it seems
priv->cmd and its associated cmd->req SKB might be leaked here when priv
is ultimately freed, since the active command is not in the cmd_queue.

This is another pre-existing issue, but does a transmission failure
permanently stall the command queue elsewhere in this file?

In drivers/nfc/pn533/pn533.c:pn533_wq_cmd(), if the physical layer
transmission fails:

	rc = dev->phy_ops->send_frame(dev, cmd->req);
	if (rc < 0) {
		dev->cmd = NULL;
		dev_kfree_skb(cmd->req);
		kfree(cmd);
		return;
	}

The function frees the command but skips calling its complete_cb (hanging
the caller) and fails to reset dev->cmd_pending = 0. This seems like it
would permanently stall all subsequent commands in the cmd_queue.

>  	}
>


[1] 
https://sashiko.dev/#/patchset/3B4941C5D2E3FEE0%2B20260706083926.798888-1-raoxu%40uniontech.com