netdev - Re: [RFC PATCH] net: clean up rx

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1310175469.26320.9.camel@obelisk.thedillows.org>
Date:	Fri, 08 Jul 2011 21:37:49 -0400
From:	David Dillow <dave@...dillows.org>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH] net: clean up rx_copybreak handling

On Sat, 2011-07-09 at 01:27 +0200, Michał Mirosław wrote:
> diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c

> @@ -1675,25 +1674,14 @@ typhoon_rx(struct typhoon *tp, struct basic_ring *rxRing, volatile __le32 * read
>  
>                 pkt_len = le16_to_cpu(rx->frameLen);
>  
> -               if(pkt_len < rx_copybreak &&
> -                  (new_skb = dev_alloc_skb(pkt_len + 2)) != NULL) {
> -                       skb_reserve(new_skb, 2);
> -                       pci_dma_sync_single_for_cpu(tp->pdev, dma_addr,
> -                                                   PKT_BUF_SZ,
> -                                                   PCI_DMA_FROMDEVICE);
> -                       skb_copy_to_linear_data(new_skb, skb->data, pkt_len);
> -                       pci_dma_sync_single_for_device(tp->pdev, dma_addr,
> -                                                      PKT_BUF_SZ,
> -                                                      PCI_DMA_FROMDEVICE);
> -                       skb_put(new_skb, pkt_len);
> +               new_skb = dev_skb_finish_rx_dma(&rxb->skb,
> +                       pkt_len, rx_copybreak,
> +                       &tp->pdev->dev, dma_addr, PKT_BUF_SZ);

Needs a few more tabs in front of the arguments. It looks like
		new_skb = dev_skb_finish_rx_dma(&rxb->skb, pkt_len,
						rx_copybreak, &tp->pdev->dev,
						dma_addr, PKT_BUF_SZ);

would fit the style around it better.

> +               if (!rxb->skb)
>                         typhoon_recycle_rx_skb(tp, idx);

I think you either meant
		if (rxb->skb)
			typhoon_recycle_rx_skb(tp, idx);

or to swap typhoon_{recycle,alloc}_rx_skb(). As it stands, you'll reload
the NIC with a NULL skb pointer, and it will DMA to the old location
when it eventually uses this descriptor.

> -               } else {
> -                       new_skb = skb;
> -                       skb_put(new_skb, pkt_len);
> -                       pci_unmap_single(tp->pdev, dma_addr, PKT_BUF_SZ,
> -                                      PCI_DMA_FROMDEVICE);
> +               else
>                         typhoon_alloc_rx_skb(tp, idx);


--
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