From: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp
[ Upstream commit eecd203ada43a4693ce6fdd3a58ae10c7819252c ]
syzbot is reporting that imon has three problems which result in hung tasks due to forever holding device lock [1].
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).
Alan Stern commented [2] that
In theory it's okay to resubmit _if_ the driver has a robust error-recovery scheme (such as giving up after some fixed limit on the number of errors or after some fixed time has elapsed, perhaps with a time delay to prevent a flood of errors). Most drivers don't bother to do this; they simply give up right away. This makes them more vulnerable to short-term noise interference during USB transfers, but in reality such interference is quite rare. There's nothing really wrong with giving up right away.
but imon has a poor error-recovery scheme which just retries forever; this behavior should be fixed.
Since I'm not sure whether it is safe for imon users to give up upon any error code, this patch takes care of only union of error codes chosen from modules in drivers/media/rc/ directory which handle -EPROTO error (i.e. ir_toy, mceusb and igorplugusb).
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"). Move the ictx->dev_present_intf0 test introduced by commit 6f6b90c9231a ("[media] imon: don't parse scancodes until intf configured") to immediately before imon_incoming_packet(), 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 timeout of 10 seconds.
Link: https://syzkaller.appspot.com/bug?extid=592e2ab8775dbe0bf09a [1] Link: https://lkml.kernel.org/r/d6da6709-d799-4be3-a695-850bddd6eb24@rowland.harva... [2] Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Signed-off-by: Sean Young sean@mess.org Signed-off-by: Hans Verkuil hverkuil+cisco@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
This is a targeted robustness fix for the SoundGraph iMON IR/display USB driver that addresses real-world hangs and RCU stalls reported by syzbot, with small, contained changes limited to `drivers/media/rc/imon.c`. It fits stable backport rules: fixes important bugs, avoids features/architecture changes, and aligns error handling with peer RC drivers.
Key fixes and rationale - Stop infinite RX error loops (-EPROTO flood): - Current code resubmits the URB on all errors, which can loop endlessly on persistent USB errors such as -EPROTO, spamming logs and causing RCU stalls. - The patch handles specific transient/bus/pipe errors by logging and not resubmitting, breaking the error loop: - Adds early-return without resubmit on `-ECONNRESET`, `-EILSEQ`, `-EPROTO`, `-EPIPE` in both callbacks. - This mirrors other rc drivers’ behavior (see below). - Code reference (pre-change): `drivers/media/rc/imon.c:1757` shows an unconditional “resubmit on error” path via `goto out` and fallthrough to `usb_submit_urb()`, with “ignored” logging in `default:` (e.g. `drivers/media/rc/imon.c:1765` onward for intf0; similar for intf1 at `drivers/media/rc/imon.c:1806`). - Fix early-callback behavior before interface is configured: - Previously, if an early callback happened before `dev_present_intf{0,1}` was set, the code always resubmitted (due to the pre-switch `if (!dev_present) goto out;`), which can contribute to error loops without even processing packets. - The patch moves the `dev_present_intf{0,1}` check into the `case 0:` (success) branch so that only valid data is processed, while error handling goes through the new non-resubmit paths. This prevents unnecessary requeueing during pre-configured phases and avoids wedging/hung tasks without printk flood. - Code reference (pre-change): the pre-switch gating at `drivers/media/rc/imon.c:1757-1764` (intf0) and `drivers/media/rc/imon.c:1798-1805` (intf1) causes unconditional resubmit on any pre-configured callback. - Prevent indefinite TX wait (hung tasks): - Currently `send_packet()` waits indefinitely for the TX completion and only wakes on interrupt, hanging if the TX callback never arrives (e.g., flaky hardware). - The patch changes the wait to `wait_for_completion_interruptible_timeout()` with a 10s timeout, kills the URB on timeout, and sets a sensible error status (-ETIMEDOUT on timeout, or the negative retval on signal), then reports failure. - Code reference (pre-change): `drivers/media/rc/imon.c:653-659` uses `wait_for_completion_interruptible(&ictx->tx.finished)` with no timeout. The patch replaces this with a 10*HZ timeout and sets `ictx->tx.status` appropriately after `usb_kill_urb()`.
Consistency with other rc drivers - This driver’s new error handling aligns with other `drivers/media/rc/` drivers which do not resubmit on transient/bus errors: - `mceusb`: avoids resubmit for `-ECONNRESET`, `-ENOENT`, `-EILSEQ`, `-EPROTO`, `-ESHUTDOWN` in RX (drivers/media/rc/mceusb.c:1360). - `igorplugusb`: returns without resubmit on `-EPROTO`, `-ECONNRESET`, `-ENOENT`, `-ESHUTDOWN` (drivers/media/rc/igorplugusb.c:100). - `ir_toy`: returns/unlinks on `-ECONNRESET`, `-ENOENT`, `-ESHUTDOWN`, `-EPROTO`, `-EPIPE` (drivers/media/rc/ir_toy.c:212).
Scope and risk assessment - Small, self-contained changes in a single driver file; no ABI/API changes; no architectural shifts. - Fixes reproducible hangs and RCU stalls (syzbot report), clearly improving stability. - Behavior trade-off (not resubmitting on certain USB errors) matches established patterns in similar drivers, reducing regression risk. - Interaction with earlier imon commits: - 6f6b90c9231a “[media] imon: don't parse scancodes until intf configured” introduced early-return gating using `dev_present_*`. - 8791d63af0cf “[media] imon: don't wedge hardware after early callbacks” ensured resubmit even before configuration. - This patch refines both behaviors: only resubmit when it makes sense (successful RX or non-specified errors), and bound TX wait time.
Why this belongs in stable - Fixes real bugs affecting users (hung tasks, potential RCU stall/flooded logs). - Minimal and surgical per-driver changes; no new features. - Matches stable policy and conventions across related drivers. - Signed by subsystem maintainers (Sean Young, Hans Verkuil), indicating maintainership approval.
Conclusion - Backporting this commit will prevent hangs and error floods in `imon` with low risk and improve robustness without altering kernel-wide behavior.
drivers/media/rc/imon.c | 61 +++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index cf3e6e43c0c7e..8668d53c0d426 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -650,12 +650,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); + 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; @@ -1754,14 +1757,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; @@ -1770,16 +1765,29 @@ 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); + 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); }
@@ -1795,14 +1803,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; @@ -1811,16 +1811,29 @@ 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); + 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); }