Re: [PATCH] nfc: pn533: add timeout to USB frame send completion wait

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch gives the device a new way to trigger a failure
  (-ETIMEDOUT from pn533_usb_send_frame() when OUT stalls for more than…
- [Medium] On timeout, pn533_usb_send_frame() calls
  usb_kill_urb(phy->out_urb) and returns -ETIMEDOUT unconditionally.
- [Low] Missing Fixes: tag (and Cc: stable).

--

nfc: pn533: add timeout to USB frame send completion wait

This patch replaces the unbounded wait for the out URB in
pn533_usb_send_frame() with a 1 second timeout. On timeout the URB is
killed and -ETIMEDOUT is returned, so a device that never completes the
transfer no longer hangs probe or the command in progress.

Closes: https://syzbot.org/bug?extid=1c5368988052dc2d8201

> Reported-by: syzbot+1c5368988052dc2d8201@syzkaller.appspotmail.com
> Closes: https://syzbot.org/bug?extid=1c5368988052dc2d8201
> Signed-off-by: Zhao Feng <zhaofeng2@cmss.chinamobile.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? The unbounded
wait_for_completion(&arg.done) and the context save/restore around it
came from:

Fixes: 9dab880d675b ("nfc: pn533: Wait for out_urb's completion in pn533_usb_send_frame()")

> diff --git a/drivers/nfc/pn533/usb.c b/drivers/nfc/pn533/usb.c
> index efb07f944fce2..60e80e818d2e6 100644
> --- a/drivers/nfc/pn533/usb.c
> +++ b/drivers/nfc/pn533/usb.c

[ ... ]

> @@ -184,7 +187,12 @@ static int pn533_usb_send_frame(struct pn533 *dev,
>  	if (rc)
>  		return rc;
>  
> -	wait_for_completion(&arg.done);
> +	if (!wait_for_completion_timeout(&arg.done,
> +					 msecs_to_jiffies(PN533_USB_SEND_TIMEOUT_MS))) {
> +		usb_kill_urb(phy->out_urb);
> +		phy->out_urb->context = cntx;
> +		return -ETIMEDOUT;
> +	}
>  	phy->out_urb->context = cntx;
>  
>  	if (dev->protocol_type == PN533_PROTO_REQ_RESP) {

[Severity: Medium]
Is it safe to return -ETIMEDOUT here without checking the final status
of out_urb?

The OUT transfer can complete between wait_for_completion_timeout()
returning 0 and the call to usb_kill_urb(). In that case usb_kill_urb()
returns at once because pn533_out_complete() has already run. The device
has the whole command, but the caller is told the send failed.
usb_start_wait_urb() looks at the URB status after killing it on timeout
for this reason.

A PN533 frame can also span several 64-byte bulk packets. The device may
accept part of the frame before it stalls, which leaves a partial frame
in its parser.

In both cases the early return skips pn533_submit_urb_for_ack() and
pn533_submit_urb_for_response(), so the device's ACK or response is
never read. __pn533_send_async() then does:

	rc = pn533_send_cmd_frame(dev, cmd);
	if (rc) {
		dev->cmd = NULL;
		goto error;
	}

This leaves cmd_pending at 0, so the next command is sent right away.
Its pn533_recv_ack() would see the stale ACK, and its in_urb would get
the abandoned command's response. pn533_recv_frame() only compares the
command code:

	} else if (!pn533_rx_frame_is_cmd_response(dev, skb->data)) {
		nfc_err(dev->dev, "It it not the response to the last command\n");
		dev->cmd->status = -EIO;
	}

When two commands in a row share a code, such as repeated InDataExchange
from pn533_transceive(), could the stale response be accepted as the
reply to the new request? The driver would then stay one response
behind. Commands with different codes would fail with -EIO instead.

The driver already has a protocol-level abort, pn533_usb_send_ack(),
which pn533_usb_abort_cmd() uses. The comment in pn533_usb_abort_cmd()
also says ACR122U sends broken responses when URBs are cancelled before
the chip responds.

Should the timeout path send the ACK abort or reset the device so that
the two sides stay in step?

[Severity: High]
Does the commit message claim that "the command in progress fails
gracefully instead of hanging the task" hold for queued commands?

When dev->cmd_pending is set, __pn533_send_async() adds the command to
dev->cmd_queue and returns 0. pn533_send_cmd_sync() then sleeps in:

	wait_for_completion(&arg.done);

Later pn533_wq_cmd() takes the command off the queue and sends it. If
pn533_usb_send_frame() now returns -ETIMEDOUT, the error path in
pn533_wq_cmd() is:

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

This path never calls cmd->complete_cb, never clears cmd_pending and
never re-queues cmd_work.

Could the pn533_send_cmd_sync() caller then sleep in
TASK_UNINTERRUPTIBLE forever? That is the same hung task, just moved to
the nfc netlink caller.

One way to reach it:

nfc_dev_down()
  pn533_stop_poll()   (abort_cmd is a no-op on ACR122U, poll stays pending)
  pn533_dev_down()
    pn533_rf_field()
      pn533_set_configuration()
        pn533_send_cmd_sync()   (queued behind the pending command)

Async callers are affected too:

- The pn533_data_exchange_arg allocated by pn533_transceive() would
  leak, and its data_exchange callback would never run. For rawsock this
  also leaks the sock_hold() reference.
- With cmd_pending stuck at 1, every later command is queued and never
  sent until unbind.
- pn53x_common_clean() then frees the queued cmds without freeing their
  cmd->req skbs.

The pn533_wq_cmd() error handling is older than this patch, but this
patch lets the device trigger it. Should pn533_wq_cmd() call
cmd->complete_cb with ERR_PTR(rc) and keep the queue moving as part of
this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023025602.1746-1-zhaofeng2%40cmss.chinamobile.com