Re: [PATCH] usbnet: remove generic hard_header_len check

["Bjørn Mork" <bjorn@mork.no>]

Emil Goode <emilgoode@gmail.com> writes:

> This patch removes a generic hard_header_len check from the usbnet
> module that is causing dropped packages under certain circumstances
> for devices that send rx packets that cross urb boundaries.
>
> One example is the AX88772B which occasionally send rx packets that
> cross urb boundaries where the remaining partial packet is sent with
> no hardware header. When the buffer with a partial packet is of less
> number of octets than the value of hard_header_len the buffer is
> discarded by the usbnet module.
>
> With AX88772B this can be reproduced by using ping with a packet
> size between 1965-1976.
>
> The bug has been reported here:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=29082
>
> This patch introduces the following changes:
> - Removes the generic hard_header_len check in the rx_complete
>   function in the usbnet module.
> - Introduces a ETH_HLEN check for skbs that are not cloned from
>   within a rx_fixup callback.
> - For safety a hard_header_len check is added to each rx_fixup
>   callback function that could be affected by this change.
>   These extra checks could possibly be removed by someone
>   who has the hardware to test.
>
> The changes place full responsibility on the rx_fixup callback
> functions that clone skbs to only pass valid skbs to the
> usbnet_skb_return function.
>
> Signed-off-by: Emil Goode <emilgoode@gmail.com>
> Reported-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>


Great work!  Looks good to me.

Just a couple of questions:

> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index ff5c871..b2e2aee 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -80,10 +80,10 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  {
>  	__be16 proto;
>  
> -	/* usbnet rx_complete guarantees that skb->len is at least
> -	 * hard_header_len, so we can inspect the dest address without
> -	 * checking skb->len
> -	 */
> +	/* This check is no longer done by usbnet */
> +	if (skb->len < dev->net->hard_header_len)
> +		return 0;
> +
>  	switch (skb->data[0] & 0xf0) {
>  	case 0x40:
>  		proto = htons(ETH_P_IP);
> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index a48bc0f..524a47a 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -492,6 +492,10 @@ EXPORT_SYMBOL_GPL(rndis_unbind);
>   */
>  int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  {
> +	/* This check is no longer done by usbnet */
> +	if (skb->len < dev->net->hard_header_len)
> +		return 0;
> +

Wouldn't it be better to test against ETH_HLEN, since that is a constant
and "obviously correct" in this case?


> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 4671da7..dd10d58 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -542,17 +542,19 @@ static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
>  	}
>  	// else network stack removes extra byte if we forced a short packet
>  
> -	if (skb->len) {
> -		/* all data was already cloned from skb inside the driver */
> -		if (dev->driver_info->flags & FLAG_MULTI_PACKET)
> -			dev_kfree_skb_any(skb);
> -		else
> -			usbnet_skb_return(dev, skb);
> +	/* all data was already cloned from skb inside the driver */
> +	if (dev->driver_info->flags & FLAG_MULTI_PACKET)
> +		goto done;
> +
> +	if (skb->len < ETH_HLEN) {
> +		dev->net->stats.rx_errors++;
> +		dev->net->stats.rx_length_errors++;
> +		netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len);
> +	} else {
> +		usbnet_skb_return(dev, skb);
>  		return;
>  	}
>  
> -	netif_dbg(dev, rx_err, dev->net, "drop\n");
> -	dev->net->stats.rx_errors++;
>  done:
>  	skb_queue_tail(&dev->done, skb);
>  }


At first glance I wondered where the dev_kfree_skb_any(skb) went.  But
then I realized that you leave that to the common rx_cleanup path, using
the dev->done queue.  Is that right?  If so, then I guess it could use a
bit of explaining in the commit message?

If I'm wrong, then I'm still looking for the explanation of where the
dev_kfree_skb_any went :-)



Bjørn