| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <878uyu6xjm.fsf@nemi.mork.no>
Date: Wed, 18 Sep 2013 15:59:09 +0200
From: Bjørn Mork <bjorn@...k.no>
To: Ming Lei <ming.lei@...onical.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Oliver Neukum <oneukum@...e.de>, netdev@...r.kernel.org,
linux-usb@...r.kernel.org, Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH] USBNET: fix handling padding packet
Ming Lei <ming.lei@...onical.com> writes:
> Commit 638c5115a7949(USBNET: support DMA SG) introduces DMA SG
> if the usb host controller is capable of building packet from
> discontinuous buffers, but missed handling padding packet when
> building DMA SG.
>
> This patch attachs the pre-allocated padding packet at the
> end of the sg list, so padding packet can be sent to device
> if drivers require that.
>
> Reported-by: David Laight <David.Laight@...lab.com>
> Cc: Oliver Neukum <oliver@...kum.org>
> Signed-off-by: Ming Lei <ming.lei@...onical.com>
> ---
> drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++------
> include/linux/usb/usbnet.h | 1 +
> 2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 7b331e6..4a9bed3 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1241,7 +1241,9 @@ static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
> if (num_sgs == 1)
> return 0;
>
> - urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
> + /* reserve one for zero packet */
> + urb->sg = kmalloc((num_sgs + 1) * sizeof(struct scatterlist),
> + GFP_ATOMIC);
> if (!urb->sg)
> return -ENOMEM;
>
> @@ -1305,7 +1307,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
> if (build_dma_sg(skb, urb) < 0)
> goto drop;
> }
> - entry->length = length = urb->transfer_buffer_length;
> + length = urb->transfer_buffer_length;
>
> /* don't assume the hardware handles USB_ZERO_PACKET
> * NOTE: strictly conforming cdc-ether devices should expect
> @@ -1317,15 +1319,18 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
> if (length % dev->maxpacket == 0) {
> if (!(info->flags & FLAG_SEND_ZLP)) {
> if (!(info->flags & FLAG_MULTI_PACKET)) {
> - urb->transfer_buffer_length++;
> - if (skb_tailroom(skb)) {
> + length++;
> + if (skb_tailroom(skb) && !dev->can_dma_sg) {
> skb->data[skb->len] = 0;
> __skb_put(skb, 1);
> - }
> + } else if (dev->can_dma_sg)
> + sg_set_buf(&urb->sg[urb->num_sgs++],
> + dev->padding_pkt, 1);
> }
> } else
> urb->transfer_flags |= URB_ZERO_PACKET;
> }
> + entry->length = urb->transfer_buffer_length = length;
>
> spin_lock_irqsave(&dev->txq.lock, flags);
> retval = usb_autopm_get_interface_async(dev->intf);
> @@ -1509,6 +1514,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>
> usb_kill_urb(dev->interrupt);
> usb_free_urb(dev->interrupt);
> + kfree(dev->padding_pkt);
>
> free_netdev(net);
> }
> @@ -1679,9 +1685,16 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> /* initialize max rx_qlen and tx_qlen */
> usbnet_update_max_qlen(dev);
>
> + if (dev->can_dma_sg && !(info->flags & FLAG_SEND_ZLP) &&
> + !(info->flags & FLAG_MULTI_PACKET)) {
> + dev->padding_pkt = kzalloc(1, GFP_KERNEL);
> + if (!dev->padding_pkt)
> + goto out4;
> + }
> +
> status = register_netdev (net);
> if (status)
> - goto out4;
> + goto out5;
> netif_info(dev, probe, dev->net,
> "register '%s' at usb-%s-%s, %s, %pM\n",
> udev->dev.driver->name,
> @@ -1699,6 +1712,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>
> return 0;
>
> +out5:
> + kfree(dev->padding_pkt);
> out4:
> usb_free_urb(dev->interrupt);
> out3:
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 9cb2fe8..e303eef 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -42,6 +42,7 @@ struct usbnet {
> struct usb_host_endpoint *status;
> unsigned maxpacket;
> struct timer_list delay;
> + const char *padding_pkt;
>
> /* protocol/interface state */
> struct net_device *net;
The code handling the frame padding was already unecessarily complex
IMHO, and this does not improve it...
Are you really sure that the one driver/device using this really need
the padding byte? If you could just make FLAG_SEND_ZLP part of the
condition for enabling can_dma_sg, then all this extra complexity would
be unnecessary. As the comment in front of the padding code says:
"strictly conforming cdc-ether devices should expect the ZLP here"
There shouldn't be any problems requiring this conformance as a
precondition for allowing SG. The requirements are already strict.
I also believe it would be nice to move the initialisation of can_dma_sg
away from the minidriver and into usbnet_probe. It's confusing that
this field is used "uninitialized" (well, defaulting to zero) in all but
one minidriver. It would much nicer if the logic was more like
usbnet_probe:
if (...)
dev->can_dma_sg = 1;
minidriver_bind:
if (dev->can_dma_sg) {
dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
}
usbnet_start_xmit:
if (skb_shinfo(skb)->nr_frags) {
...
}
This would create a natural flow of events, where probing sets the
capability, minidriver enables features based on capability, and xmit
just does what it has to do based on the skb it was given.
Or maybe even better: factor out common parts of usbnet_start_xmit and
create two versions of it - one using SG and one linear. The per packet
testing seems unnecessary expensive and complex given that the choice is
made during device initialization anyway. You could easily replace the
whole can_dma_sg stuff with a simple
if (...)
net->netdev_ops = &usbnet_sg_netdev_ops;
else
net->netdev_ops = &usbnet_netdev_ops;
in usbnet_probe. But I guess the same can be said about the info->flags
testing in usbnet_start_xmit...
Just a few random thougths. I don't really feel strongly about any of
this, except that I would prefer usbnet becoming *more* readable instead
of less...
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists