Re: [PATCH v2] usbip: vudc: reject submits after endpoint state changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usbip: vudc: v_recv_cmd_submit() -ESHUTDOWN return wedges connection
- [Medium] usbip: vudc: v_timer() and vep_disable() ignore queued URB generation
--

commit 7bd17351ae8c46943597685b7fdc609430437627
Author: Sung Byeongchan <tjdqudcks0424@naver.com>

usbip: vudc: reject submits after endpoint state changes

This commit adds a per-endpoint generation counter to vudc endpoints and
snapshots endpoint state under udc->lock in v_recv_cmd_submit(). Before
queuing an incoming urb_p onto udc->urb_queue, it revalidates that the
endpoint descriptor is still present and the generation has not changed.

> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 5ef88117965d1..e3800d5e83a30 100644
> --- a/drivers/usb/usbip/vudc_dev.c
> +++ b/drivers/usb/usbip/vudc_dev.c

[ ... ]

> @@ -270,6 +271,7 @@ static int vep_disable(struct usb_ep *_ep)
>  
>  	spin_lock_irqsave(&udc->lock, flags);
>  	ep->desc = NULL;
> +	ep->generation++;
>  	nuke(udc, ep);
>  	spin_unlock_irqrestore(&udc->lock, flags);

[Severity: Medium]
What happens to urbs that were already validated and added to
udc->urb_queue before vep_disable() runs?

In drivers/usb/usbip/vudc_dev.c, vep_disable() clears ep->desc, increments
ep->generation, and calls nuke(udc, ep), which only drains gadget requests
from ep->req_queue while leaving udc->urb_queue untouched.

When v_timer() in drivers/usb/usbip/vudc_transfer.c iterates
udc->urb_queue, it neither checks ep->desc nor compares ep->generation
against urb_p->ep_generation:

drivers/usb/usbip/vudc_transfer.c:v_timer() {
	...
	list_for_each_entry_safe(urb_p, tmp, &udc->urb_queue, urb_entry) {
		struct urb *urb = urb_p->urb;

		ep = urb_p->ep;
		if (urb->unlinked)
			goto return_urb;
		if (timer->state != VUDC_TR_RUNNING)
			continue;

		if (!ep) {
			urb->status = -EPROTO;
			goto return_urb;
		}
		...
		switch (ep->type) {
		...
		default:
treat_control_like_bulk:
			total -= transfer(udc, urb, ep, limit);
		}
		if (urb->status == -EINPROGRESS)
			continue;
}

If the endpoint remains disabled, transfer() sees an empty ep->req_queue
and leaves urb->status unchanged, keeping urb_p queued on udc->urb_queue
and re-arming tr_timer every millisecond. If the endpoint is later
re-enabled via vep_enable(), v_timer() processes the stale urb_p from the
old generation against the new endpoint state and dispatches on the new
ep->type while v_send_ret_submit() uses the snapshotted urb_p->type.

Should vep_disable() or v_timer() also fail queued entries on
udc->urb_queue whose endpoint descriptor was cleared or whose
ep->generation no longer matches urb_p->ep_generation?

> diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
> index 51bb70837b902..3c459c7b57b5f 100644
> --- a/drivers/usb/usbip/vudc_rx.c
> +++ b/drivers/usb/usbip/vudc_rx.c

[ ... ]

> @@ -175,6 +179,13 @@ static int v_recv_cmd_submit(struct vudc *udc,
>  		goto free_urbp;
>  
>  	spin_lock_irqsave(&udc->lock, flags);
> +	if (urb_p->ep != &udc->ep[0] &&
> +	    (!urb_p->ep->desc ||
> +	     urb_p->ep->generation != urb_p->ep_generation)) {
> +		spin_unlock_irqrestore(&udc->lock, flags);
> +		ret = -ESHUTDOWN;
> +		goto free_urbp;
> +	}
>  	v_kick_timer(udc, jiffies);
>  	list_add_tail(&urb_p->urb_entry, &udc->urb_queue);
>  	spin_unlock_irqrestore(&udc->lock, flags);

[Severity: High]
Can returning this error from v_recv_cmd_submit() wedge the usbip
connection and vudc device state?

By the time v_recv_cmd_submit() reaches this check in
drivers/usb/usbip/vudc_rx.c, usbip_recv_xbuff() and usbip_recv_iso() have
already consumed the full submit payload from ud->tcp_socket. Jumping to
free_urbp drops urb_p without queuing a completion via
v_enqueue_ret_submit() and returns the negative error through v_rx_pdu() to
v_rx_loop():

drivers/usb/usbip/vudc_rx.c:v_rx_pdu() {
	...
	case USBIP_CMD_SUBMIT:
		ret = v_recv_cmd_submit(udc, &pdu);
		break;
	...
	return ret;
}

drivers/usb/usbip/vudc_rx.c:v_rx_loop() {
	...
	while (!kthread_should_stop()) {
		if (usbip_event_happened(ud))
			break;
		ret = v_rx_pdu(ud);
		if (ret < 0) {
			pr_warn("v_rx exit with error %d", ret);
			break;
		}
	}
	return ret;
}

Unlike other fatal error paths in v_recv_cmd_submit() and v_rx_pdu(), this
path does not call usbip_event_add(), so vudc_shutdown() and
vudc_device_reset() in drivers/usb/usbip/vudc_dev.c are never scheduled:

drivers/usb/usbip/vudc_dev.c:vudc_shutdown() {
	...
	if (ud->tcp_socket)
		kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);

	if (ud->tcp_rx) {
		kthread_stop_put(ud->tcp_rx);
		ud->tcp_rx = NULL;
	}
	if (ud->tcp_tx) {
		kthread_stop_put(ud->tcp_tx);
		ud->tcp_tx = NULL;
	}
	...
}

When ud->tcp_rx exits on its own without an event queued, ud->tcp_tx
remains asleep on udc->tx_waitq, ud->tcp_socket stays open with no reader,
the client hangs waiting for a submit reply, and udc->connected and
ud->status remain stuck in the active state.

Since the stream framing on ud->tcp_socket is still intact after reading
the payload, should v_recv_cmd_submit() complete the rejected urb_p back to
the client via v_enqueue_ret_submit() and return 0, or trigger connection
teardown via usbip_event_add()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020181832.461892-1-tjdqudcks0424@naver.com?part=1