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: <1351154458.6537.146.camel@edumazet-glaptop> Date: Thu, 25 Oct 2012 10:40:58 +0200 From: Eric Dumazet <eric.dumazet@...il.com> To: Veaceslav Falico <vfalico@...hat.com> Cc: netdev@...r.kernel.org, davem@...emloft.net, richardcochran@...il.com, tshimizu818@...il.com, andy.cress@...kontron.com, erwan.velu@...iacaerospace.com Subject: Re: [PATCH] pch_gbe: don't come up if we can't allocate buffers On Thu, 2012-10-25 at 09:10 +0200, Veaceslav Falico wrote: > Currently, even if pch_gbe_alloc_tx/rx_buffers (even partially) fail, we > bring the interface up. It mihgt confuse the user (if he requested specific > amount of buffers) and can lead to different bugs. > > Add error handling to these functions and clean after them in case of > failure. It also resolves a possible NULL pointer dereference in > pch_gbe_alloc_tx_buffers(), in case of netdev_alloc_skb() failure. It's ok > to (paritally) fail for pch_gbe_alloc_rx_buffers() in some situation, so > don't clean inside and rather handle skb freeing outside of the function. > > This patch also adds tx_alloc_buff_failed ethtool counter. This should not be needed : > -static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, > +static int pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, > struct pch_gbe_tx_ring *tx_ring) > { > struct pch_gbe_buffer *buffer_info; > @@ -1506,6 +1521,7 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, > unsigned int i; > unsigned int bufsz; > struct pch_gbe_tx_desc *tx_desc; > + int err; > > bufsz = > adapter->hw.mac.max_frame_size + PCH_GBE_DMA_ALIGN + NET_IP_ALIGN; > @@ -1513,12 +1529,26 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter, > for (i = 0; i < tx_ring->count; i++) { > buffer_info = &tx_ring->buffer_info[i]; > skb = netdev_alloc_skb(adapter->netdev, bufsz); > + if (unlikely(!skb)) { > + adapter->stats.tx_alloc_buff_failed++; > + err = -ENOMEM; > + goto freeall; > + } Hmmm These skbs should be allocated using GFP_KERNEL, to lower risk of OOM, using a mere alloc_skb(bufsz, GFP_KERNEL) Only skbs for rx 'need' netdev_alloc_skb() to add a NET_SKB_PAD headroom. skb = alloc_skb(bufsz, GFP_KERNEL); if (!skb) goto fail; skb_reserve(skb, NET_IP_ALIGN); And BTW, we dont really need skbs here, only a bounce buffer so that we can copy frames on it... (kmalloc() instead of alloc_skb()) > skb_reserve(skb, PCH_GBE_DMA_ALIGN); PCH_GBE_DMA_ALIGN is zero ... > buffer_info->skb = skb; > tx_desc = PCH_GBE_TX_DESC(*tx_ring, i); > tx_desc->gbec_status = (DSC_INIT16); > } > - return; > + return 0; > + > +freeall: > + for (i--; i >= 0; i--) { > + dev_kfree_skb_any(buffer_info->skb); > + buffer_info->skb = NULL; > + tx_desc = PCH_GBE_TX_DESC(*tx_ring, i); > + tx_desc->gbec_status = 0; > + } > + return err; > } > Also I would call pch_gbe_alloc_tx_buffers() _after_ pch_gbe_alloc_rx_buffers() (Since rx allocations probably are using GFP_ATOMIC allocations) -- 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