Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

media: imon: make send_packet() more robust

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.harvard.edu [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>

authored by

Tetsuo Handa and committed by
Hans Verkuil
eecd203a 4f4098c5

+37 -24
+37 -24
drivers/media/rc/imon.c
··· 645 645 smp_rmb(); /* ensure later readers know we're not busy */ 646 646 pr_err_ratelimited("error submitting urb(%d)\n", retval); 647 647 } else { 648 - /* Wait for transmission to complete (or abort) */ 649 - retval = wait_for_completion_interruptible( 650 - &ictx->tx.finished); 651 - if (retval) { 648 + /* Wait for transmission to complete (or abort or timeout) */ 649 + retval = wait_for_completion_interruptible_timeout(&ictx->tx.finished, 10 * HZ); 650 + if (retval <= 0) { 652 651 usb_kill_urb(ictx->tx_urb); 653 652 pr_err_ratelimited("task interrupted\n"); 653 + if (retval < 0) 654 + ictx->tx.status = retval; 655 + else 656 + ictx->tx.status = -ETIMEDOUT; 654 657 } 655 658 656 659 ictx->tx.busy = false; ··· 1748 1745 if (!ictx) 1749 1746 return; 1750 1747 1751 - /* 1752 - * if we get a callback before we're done configuring the hardware, we 1753 - * can't yet process the data, as there's nowhere to send it, but we 1754 - * still need to submit a new rx URB to avoid wedging the hardware 1755 - */ 1756 - if (!ictx->dev_present_intf0) 1757 - goto out; 1758 - 1759 1748 switch (urb->status) { 1760 1749 case -ENOENT: /* usbcore unlink successful! */ 1761 1750 return; ··· 1756 1761 break; 1757 1762 1758 1763 case 0: 1759 - imon_incoming_packet(ictx, urb, intfnum); 1764 + /* 1765 + * if we get a callback before we're done configuring the hardware, we 1766 + * can't yet process the data, as there's nowhere to send it, but we 1767 + * still need to submit a new rx URB to avoid wedging the hardware 1768 + */ 1769 + if (ictx->dev_present_intf0) 1770 + imon_incoming_packet(ictx, urb, intfnum); 1760 1771 break; 1772 + 1773 + case -ECONNRESET: 1774 + case -EILSEQ: 1775 + case -EPROTO: 1776 + case -EPIPE: 1777 + dev_warn(ictx->dev, "imon %s: status(%d)\n", 1778 + __func__, urb->status); 1779 + return; 1761 1780 1762 1781 default: 1763 1782 dev_warn(ictx->dev, "imon %s: status(%d): ignored\n", ··· 1779 1770 break; 1780 1771 } 1781 1772 1782 - out: 1783 1773 usb_submit_urb(ictx->rx_urb_intf0, GFP_ATOMIC); 1784 1774 } 1785 1775 ··· 1794 1786 if (!ictx) 1795 1787 return; 1796 1788 1797 - /* 1798 - * if we get a callback before we're done configuring the hardware, we 1799 - * can't yet process the data, as there's nowhere to send it, but we 1800 - * still need to submit a new rx URB to avoid wedging the hardware 1801 - */ 1802 - if (!ictx->dev_present_intf1) 1803 - goto out; 1804 - 1805 1789 switch (urb->status) { 1806 1790 case -ENOENT: /* usbcore unlink successful! */ 1807 1791 return; ··· 1802 1802 break; 1803 1803 1804 1804 case 0: 1805 - imon_incoming_packet(ictx, urb, intfnum); 1805 + /* 1806 + * if we get a callback before we're done configuring the hardware, we 1807 + * can't yet process the data, as there's nowhere to send it, but we 1808 + * still need to submit a new rx URB to avoid wedging the hardware 1809 + */ 1810 + if (ictx->dev_present_intf1) 1811 + imon_incoming_packet(ictx, urb, intfnum); 1806 1812 break; 1813 + 1814 + case -ECONNRESET: 1815 + case -EILSEQ: 1816 + case -EPROTO: 1817 + case -EPIPE: 1818 + dev_warn(ictx->dev, "imon %s: status(%d)\n", 1819 + __func__, urb->status); 1820 + return; 1807 1821 1808 1822 default: 1809 1823 dev_warn(ictx->dev, "imon %s: status(%d): ignored\n", ··· 1825 1811 break; 1826 1812 } 1827 1813 1828 - out: 1829 1814 usb_submit_urb(ictx->rx_urb_intf1, GFP_ATOMIC); 1830 1815 } 1831 1816