Re: [PATCH net 08/12] can: usb: f81604: handle short interrupt urb messages properly

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Mar 03, 2026 at 03:23:19PM +0100, Paolo Abeni wrote:
> On 3/2/26 4:16 PM, Marc Kleine-Budde wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > If an interrupt urb is received that is not the correct length, properly
> > detect it and don't attempt to treat the data as valid.
> > 
> > Cc: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > Cc: Vincent Mailhol <mailhol@kernel.org>
> > Cc: stable@kernel.org
> > Assisted-by: gkh_clanker_2000
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Link: https://patch.msgid.link/2026022331-opal-evaluator-a928@gregkh
> > Fixes: 88da17436973 ("can: usb: f81604: add Fintek F81604 support")
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/can/usb/f81604.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> > index 76578063ac82..c61bd30d1765 100644
> > --- a/drivers/net/can/usb/f81604.c
> > +++ b/drivers/net/can/usb/f81604.c
> > @@ -620,6 +620,12 @@ static void f81604_read_int_callback(struct urb *urb)
> >  		netdev_info(netdev, "%s: Int URB aborted: %pe\n", __func__,
> >  			    ERR_PTR(urb->status));
> >  
> > +	if (urb->actual_length < sizeof(*data)) {
> > +		netdev_warn(netdev, "%s: short int URB: %u < %zu\n",
> > +			    __func__, urb->actual_length, sizeof(*data));
> > +		goto resubmit_urb;
> > +	}
> > +
> >  	switch (urb->status) {
> >  	case 0: /* success */
> >  		break;
> 
> AI says:
> 
> ---
> Should the length check happen after the status check instead of before it?
> With the current ordering, if a URB completes with both a terminal error
> status (like -ESHUTDOWN, -ENOENT, -EPIPE, or -EPROTO) and insufficient
> data length, the code will jump to resubmit_urb instead of returning
> immediately.
> 
> Looking at the switch statement that follows:
> 
> >  	switch (urb->status) {
> >  	case 0: /* success */
> >  		break;
> >
> > 	case -ENOENT:
> > 	case -EPIPE:
> > 	case -EPROTO:
> > 	case -ESHUTDOWN:
> > 		return;
> 
> Terminal error codes should cause immediate return without resubmission,
> but the length check bypasses this. This could lead to inappropriate URB
> resubmissions when the device is shutting down or has been disconnected.
> ---
> 
> IDK if 'status' is valid in case of short URB, possibly the patch code
> is fine, but please have a look. Again, not blocking the PR.

status will be valid in case of a short URB, so all should be fine.

thanks,

greg k-h