Message-ID: <20250713081148.3880-1-hdanton@sina.com>
Date: Sun, 13 Jul 2025 16:11:47 +0800
From: Hillf Danton <hdanton@...a.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc: syzbot+592e2ab8775dbe0bf09a@...kaller.appspotmail.com,
	LKML <linux-kernel@...r.kernel.org>,
	Sean Young <sean@...s.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [PATCH] media: imon: make send_packet() more robust

[loop Alan in]
On Sun, 13 Jul 2025 16:50:08 +0900 Tetsuo Handa wrote:
> syzbot is reporting that imon has three problems which result in hung tasks
> due to forever holding device lock.
> 
> First problem is that when usb_rx_callback_intf0() once got -EPROTO error
> after ictx->dev_present_intf0 became true, usb_rx_callback_intf0()
> resubmits urb after printk(), and resubmitted urb causes
> usb_rx_callback_intf0() to again get -EPROTO error. This results in
> printk() flooding (RCU stalls).
> 
> Commit 92f461517d22 ("media: ir_toy: do not resubmit broken urb") changed
> ir_toy module not to resubmit when irtoy_in_callback() got -EPROTO error.
> We should do similar thing for imon.
> 
> Basically, I think that imon should refrain from resubmitting urb when
> callback function got an error. But since I don't know which error codes
> should retry resubmitting urb, this patch handles only union of error codes
> chosen from modules in drivers/media/rc/ directory which handles -EPROTO
> error (i.e. ir_toy, mceusb and igorplugusb).
> 
> We need to decide whether to call usb_unlink_urb() when we got -EPROTO
> error. ir_toy and mceusb call usb_unlink_urb() but igorplugusb does not
> due to commit 5e4029056263 ("media: igorplugusb: remove superfluous
> usb_unlink_urb()"). This patch calls usb_unlink_urb() because description
> of usb_unlink_urb() suggests that it is OK to call.
> 
> Second problem is that when usb_rx_callback_intf0() once got -EPROTO error
> before ictx->dev_present_intf0 becomes true, usb_rx_callback_intf0() always
> resubmits urb due to commit 8791d63af0cf ("[media] imon: don't wedge
> hardware after early callbacks"). If some errors should stop resubmitting
> urb regardless of whether configuring the hardware has completed or not,
> what that commit is doing is wrong. The ictx->dev_present_intf0 test was
> introduced by commit 6f6b90c9231a ("[media] imon: don't parse scancodes
> until intf configured"), but that commit did not call usb_unlink_urb()
> when usb_rx_callback_intf0() got an error. Move the ictx->dev_present_intf0
> test to immediately before imon_incoming_packet() so that we can call
> usb_unlink_urb() as needed, or the first problem explained above happens
> without printk() flooding (i.e. hung task).
> 
> Third problem is that when usb_rx_callback_intf0() is not called for some
> reason (e.g. flaky hardware; the reproducer for this problem sometimes
> prevents usb_rx_callback_intf0() from being called),
> wait_for_completion_interruptible() in send_packet() never returns (i.e.
> hung task). As a workaround for such situation, change send_packet() to
> wait for completion with 10 seconds of timeout.
> 
> Also, move mutex_trylock() in imon_ir_change_protocol() to the beginning,
> for memcpy() which modifies ictx->usb_tx_buf should be protected by
> ictx->lock.
> 
> Also, verify at the beginning of send_packet() that ictx->lock is held
> in case send_packet() is by error called from imon_ir_change_protocol()
> when mutex_trylock() failed due to concurrent requests.
> 
> Link: https://syzkaller.appspot.com/bug?extid=592e2ab8775dbe0bf09a
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> ---
> #syz test
> 
>  drivers/media/rc/imon.c | 69 +++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index f5221b018808..3469a401a572 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -598,6 +598,8 @@ static int send_packet(struct imon_context *ictx)
>  	int retval = 0;
>  	struct usb_ctrlrequest *control_req = NULL;
>  
> +	lockdep_assert_held(&ictx->lock);
> +
>  	/* Check if we need to use control or interrupt urb */
>  	if (!ictx->tx_control) {
>  		pipe = usb_sndintpipe(ictx->usbdev_intf0,
> @@ -645,12 +647,15 @@ static int send_packet(struct imon_context *ictx)
>  		smp_rmb(); /* ensure later readers know we're not busy */
>  		pr_err_ratelimited("error submitting urb(%d)\n", retval);
>  	} else {
> -		/* Wait for transmission to complete (or abort) */
> -		retval = wait_for_completion_interruptible(
> -				&ictx->tx.finished);
> -		if (retval) {
> +		/* Wait for transmission to complete (or abort or timeout) */
> +		retval = wait_for_completion_interruptible_timeout(&ictx->tx.finished, 10 * HZ);

Is the underlying hardware is not stable if the submitted urb failed to
complete within 10 seconds for example? In the product environment is it
making sense to ask for change to BOM, bill of material, if 10s timedout
could be reliably reproduced twice a month?

> +		if (retval <= 0) {
>  			usb_kill_urb(ictx->tx_urb);
>  			pr_err_ratelimited("task interrupted\n");
> +			if (retval < 0)
> +				ictx->tx.status = retval;
> +			else
> +				ictx->tx.status = -ETIMEDOUT;
>  		}
>  
>  		ictx->tx.busy = false;
> @@ -1121,7 +1126,7 @@ static int imon_ir_change_protocol(struct rc_dev *rc, u64 *rc_proto)
>  	int retval;
>  	struct imon_context *ictx = rc->priv;
>  	struct device *dev = ictx->dev;
> -	bool unlock = false;
> +	const bool unlock = mutex_trylock(&ictx->lock);
>  	unsigned char ir_proto_packet[] = {
>  		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x86 };
>  
> @@ -1148,8 +1153,6 @@ static int imon_ir_change_protocol(struct rc_dev *rc, u64 *rc_proto)
>  
>  	memcpy(ictx->usb_tx_buf, &ir_proto_packet, sizeof(ir_proto_packet));
>  
> -	unlock = mutex_trylock(&ictx->lock);
> -
>  	retval = send_packet(ictx);
>  	if (retval)
>  		goto out;
> @@ -1745,14 +1748,6 @@ static void usb_rx_callback_intf0(struct urb *urb)
>  	if (!ictx)
>  		return;
>  
> -	/*
> -	 * if we get a callback before we're done configuring the hardware, we
> -	 * can't yet process the data, as there's nowhere to send it, but we
> -	 * still need to submit a new rx URB to avoid wedging the hardware
> -	 */
> -	if (!ictx->dev_present_intf0)
> -		goto out;
> -
>  	switch (urb->status) {
>  	case -ENOENT:		/* usbcore unlink successful! */
>  		return;
> @@ -1761,16 +1756,30 @@ static void usb_rx_callback_intf0(struct urb *urb)
>  		break;
>  
>  	case 0:
> -		imon_incoming_packet(ictx, urb, intfnum);
> +		/*
> +		 * if we get a callback before we're done configuring the hardware, we
> +		 * can't yet process the data, as there's nowhere to send it, but we
> +		 * still need to submit a new rx URB to avoid wedging the hardware
> +		 */
> +		if (ictx->dev_present_intf0)
> +			imon_incoming_packet(ictx, urb, intfnum);
>  		break;
>  
> +	case -ECONNRESET:
> +	case -EILSEQ:
> +	case -EPROTO:
> +	case -EPIPE:
> +		dev_warn(ictx->dev, "imon %s: status(%d)\n",
> +			 __func__, urb->status);
> +		usb_unlink_urb(urb);
> +		return;
> +
>  	default:
>  		dev_warn(ictx->dev, "imon %s: status(%d): ignored\n",
>  			 __func__, urb->status);
>  		break;
>  	}
>  
> -out:
>  	usb_submit_urb(ictx->rx_urb_intf0, GFP_ATOMIC);
>  }
>  
> @@ -1786,14 +1795,6 @@ static void usb_rx_callback_intf1(struct urb *urb)
>  	if (!ictx)
>  		return;
>  
> -	/*
> -	 * if we get a callback before we're done configuring the hardware, we
> -	 * can't yet process the data, as there's nowhere to send it, but we
> -	 * still need to submit a new rx URB to avoid wedging the hardware
> -	 */
> -	if (!ictx->dev_present_intf1)
> -		goto out;
> -
>  	switch (urb->status) {
>  	case -ENOENT:		/* usbcore unlink successful! */
>  		return;
> @@ -1802,16 +1803,30 @@ static void usb_rx_callback_intf1(struct urb *urb)
>  		break;
>  
>  	case 0:
> -		imon_incoming_packet(ictx, urb, intfnum);
> +		/*
> +		 * if we get a callback before we're done configuring the hardware, we
> +		 * can't yet process the data, as there's nowhere to send it, but we
> +		 * still need to submit a new rx URB to avoid wedging the hardware
> +		 */
> +		if (ictx->dev_present_intf1)
> +			imon_incoming_packet(ictx, urb, intfnum);
>  		break;
>  
> +	case -ECONNRESET:
> +	case -EILSEQ:
> +	case -EPROTO:
> +	case -EPIPE:
> +		dev_warn(ictx->dev, "imon %s: status(%d)\n",
> +			 __func__, urb->status);
> +		usb_unlink_urb(urb);
> +		return;
> +
>  	default:
>  		dev_warn(ictx->dev, "imon %s: status(%d): ignored\n",
>  			 __func__, urb->status);
>  		break;
>  	}
>  
> -out:
>  	usb_submit_urb(ictx->rx_urb_intf1, GFP_ATOMIC);
>  }
>  
> -- 
> 2.50.1
> 
> 
> 

Powered by blists - more mailing lists