Re: [PATCH] Input: yealink - abort URB submission on fatal errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] input: yealink: infinite loop on non-fatal URB errors causing RCU stall
--

commit 61760b94680b93c5045c5b8ad2cc263df49cc507
Author: Slawomir Stepien <sst@poczta.fm>

Input: yealink - abort URB submission on fatal errors

This commit modifies URB error handling in the yealink driver to abort
submission on fatal errors like -ECONNRESET. For non-fatal errors, it skips
processing and attempts to rate-limit console messages while resubmitting.

Link: https://syzkaller.appspot.com/ai_job?id=89a6c0c7-a0c4-4f05-b1b1-e36a92e373e0

> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 560f895a00cda..f0ec4c5ea94c2 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -405,9 +405,19 @@ static void urb_irq_callback(struct urb *urb)
>  	struct yealink_dev *yld = urb->context;
>  	int ret, status = urb->status;
>  
> -	if (status)
> -		dev_err(&yld->intf->dev, "%s - urb status %d\n",
> -			__func__, status);
> +	switch (status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +	case -ENODEV:
> +		return;
> +	default:
> +		dev_err_ratelimited(&yld->intf->dev, "%s - urb status %d\n",
> +				    __func__, status);
> +		goto resubmit;
> +	}
>  
>  	switch (yld->irq_data->cmd) {
>  	case CMD_KEYPRESS:

[ ... ]

>  	yealink_do_idle_tasks(yld);
>  
> +resubmit:

[Severity: High]
Does this goto cause an unbreakable infinite loop when a non-fatal error
occurs?

Because this skips yealink_do_idle_tasks(), the state machine no longer
advances. Since this directly resubmits the control URB without yielding or
advancing state, and because control URBs are not rate-limited by the host
controller's polling interval, will a persistently failing USB device trap
the CPU in a tight softirq spin? 

Could this trigger the exact RCU stall this patch intended to fix?

>  	if (!yld->shutdown) {
>  		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
>  		if (ret && ret != -EPERM)
> -			dev_err(&yld->intf->dev,
> -				"%s - usb_submit_urb failed %d\n",
> -				__func__, ret);
> +			dev_err_ratelimited(&yld->intf->dev,
> +					    "%s - usb_submit_urb failed %d\n",
> +					    __func__, ret);
>  	}
>  }
>  
> @@ -443,9 +454,21 @@ static void urb_ctl_callback(struct urb *urb)
>  	struct yealink_dev *yld = urb->context;
>  	int ret = 0, status = urb->status;
>  
> -	if (status)
> -		dev_err(&yld->intf->dev, "%s - urb status %d\n",
> -			__func__, status);
> +	switch (status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +	case -ENODEV:
> +		return;
> +	default:
> +		dev_err_ratelimited(&yld->intf->dev, "%s - urb status %d\n",
> +				    __func__, status);
> +		if (!yld->shutdown)
> +			ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
> +		goto out;

[Severity: High]
Does this introduce a similar infinite loop in the control URB callback?

Previously, if a non-fatal error occurred, the switch statement below could
evaluate to CMD_KEYPRESS or CMD_SCANCODE, which would submit the rate-limited
interrupt URB (urb_irq). 

By unconditionally resubmitting urb_ctl here and jumping to out, we bypass
the normal state machine progression and continuously submit the
non-rate-limited control URB. Can this lead to an un-throttled CPU spin if
the device repeatedly returns a non-fatal hardware error?

> +	}
>  
>  	switch (yld->ctl_data->cmd) {
>  	case CMD_KEYPRESS:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/fcf3ed99-54ce-4049-9f7d-611827e7adad@mail.kernel.org?part=1