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: <20110709085444.GA5522@rere.qmqm.pl>
Date:	Sat, 9 Jul 2011 10:54:44 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	David Dillow <dave@...dillows.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH] net: clean up rx_copybreak handling

On Fri, Jul 08, 2011 at 09:37:49PM -0400, David Dillow wrote:
> 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);

All fixed.

What about the patch's general idea?

Best Regards,
Michał Mirosław
--
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