Message ID | 200912171810.45575.roger.oksanen@cs.helsinki.fi |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 17 Dec 2009, Roger Oksanen wrote: > e100: Fix broken cbs accounting due to missing memset. > > Alan Stern noticed that e100 caused slab corruption. > commit 98468efddb101f8a29af974101c17ba513b07be1 changed > the allocation of cbs to use dma pools that don't return zeroed memory, > especially the cb->status field used to track which cb to clean, causing > (the visible) double freeing of skbs and a wrong free cbs count. > > Now the cbs are explicitly zeroed at allocation time. > > Reported-by: Alan Stern <stern@rowland.harvard.edu> > Tested-by: Alan Stern <stern@rowland.harvard.edu> > Signed-off-by: Roger Oksanen <roger.oksanen@cs.helsinki.fi> Change looks reasonable, ACK. should we also consider a followon patch to zero memory allocated with pci_pools? Seems useful. Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
when using DMAPOOL_DEBUG, do we really want to over-write memset(retval, POOL_POISON_ALLOCATED, pool->size); set in dma_pool_alloc. On Thu, Dec 17, 2009 at 9:21 AM, Brandeburg, Jesse <jesse.brandeburg@intel.com> wrote: > > On Thu, 17 Dec 2009, Roger Oksanen wrote: >> e100: Fix broken cbs accounting due to missing memset. >> >> Alan Stern noticed that e100 caused slab corruption. >> commit 98468efddb101f8a29af974101c17ba513b07be1 changed >> the allocation of cbs to use dma pools that don't return zeroed memory, >> especially the cb->status field used to track which cb to clean, causing >> (the visible) double freeing of skbs and a wrong free cbs count. >> >> Now the cbs are explicitly zeroed at allocation time. >> >> Reported-by: Alan Stern <stern@rowland.harvard.edu> >> Tested-by: Alan Stern <stern@rowland.harvard.edu> >> Signed-off-by: Roger Oksanen <roger.oksanen@cs.helsinki.fi> > > Change looks reasonable, ACK. > > should we also consider a followon patch to zero memory allocated with > pci_pools? Seems useful. > > Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com> Date: Thu, 17 Dec 2009 09:21:49 -0800 (Pacific Standard Time) > > On Thu, 17 Dec 2009, Roger Oksanen wrote: >> e100: Fix broken cbs accounting due to missing memset. >> >> Alan Stern noticed that e100 caused slab corruption. >> commit 98468efddb101f8a29af974101c17ba513b07be1 changed >> the allocation of cbs to use dma pools that don't return zeroed memory, >> especially the cb->status field used to track which cb to clean, causing >> (the visible) double freeing of skbs and a wrong free cbs count. >> >> Now the cbs are explicitly zeroed at allocation time. >> >> Reported-by: Alan Stern <stern@rowland.harvard.edu> >> Tested-by: Alan Stern <stern@rowland.harvard.edu> >> Signed-off-by: Roger Oksanen <roger.oksanen@cs.helsinki.fi> > > Change looks reasonable, ACK. Applied, thanks everyone. I'll try to requeue the e100 stuff together into -stable again now that this is resolved. > should we also consider a followon patch to zero memory allocated with > pci_pools? Seems useful. Nah, it's more fun debugging subtle bugs like this one. :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/e100.c b/drivers/net/e100.c index d269a68..0c53c92 100644 --- a/drivers/net/e100.c +++ b/drivers/net/e100.c @@ -1817,6 +1817,7 @@ static int e100_alloc_cbs(struct nic *nic) &nic->cbs_dma_addr); if (!nic->cbs) return -ENOMEM; + memset(nic->cbs, 0, count * sizeof(struct cb)); for (cb = nic->cbs, i = 0; i < count; cb++, i++) { cb->next = (i + 1 < count) ? cb + 1 : nic->cbs; @@ -1825,7 +1826,6 @@ static int e100_alloc_cbs(struct nic *nic) cb->dma_addr = nic->cbs_dma_addr + i * sizeof(struct cb); cb->link = cpu_to_le32(nic->cbs_dma_addr + ((i+1) % count) * sizeof(struct cb)); - cb->skb = NULL; } nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = nic->cbs;