There will be a memory leak in the function alloc_ring when the parameter sw_size is nonzero and metadata is NULL. I don't know if that can happen. Here is the code, taken from the latest git tree. static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t elem_size, size_t sw_size, dma_addr_t * phys, void *metadata) { size_t len = nelem * elem_size; void *s = NULL; void *p = dma_alloc_coherent(&pdev->dev, len, phys, GFP_KERNEL); if (!p) return NULL; if (sw_size) { s = kcalloc(nelem, sw_size, GFP_KERNEL); if (!s) { dma_free_coherent(&pdev->dev, len, p, *phys); return NULL; } } if (metadata) *(void **)metadata = s; memset(p, 0, len); return p; }
"I don't know if that can happen" I meant that I don't know if the parameters can have those values. But if those parameter values are given, the memory leak is almost certain.
Reply-To: akpm@linux-foundation.org (switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface). On Sat, 22 Nov 2008 21:11:51 -0800 (PST) bugme-daemon@bugzilla.kernel.org wrote: > http://bugzilla.kernel.org/show_bug.cgi?id=12087 > > Summary: [drivers/net/cxgb3/sge.c:563]: Possible memory leak: s > Product: Drivers > Version: 2.5 > KernelVersion: latest git tree > Platform: All > OS/Version: Linux > Tree: Mainline > Status: NEW > Severity: normal > Priority: P1 > Component: Network > AssignedTo: jgarzik@pobox.com > ReportedBy: danielm77@spray.se > > > There will be a memory leak in the function alloc_ring when the parameter > sw_size is nonzero and metadata is NULL. I don't know if that can happen. > > Here is the code, taken from the latest git tree. > > static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t elem_size, > size_t sw_size, dma_addr_t * phys, void *metadata) > { > size_t len = nelem * elem_size; > void *s = NULL; > void *p = dma_alloc_coherent(&pdev->dev, len, phys, GFP_KERNEL); > > if (!p) > return NULL; > if (sw_size) { > s = kcalloc(nelem, sw_size, GFP_KERNEL); > > if (!s) { > dma_free_coherent(&pdev->dev, len, p, *phys); > return NULL; > } > } > if (metadata) > *(void **)metadata = s; > memset(p, 0, len); > return p; > } > > yeah, that is a bit silly-looking.
Reply-To: rdreier@cisco.com > There will be a memory leak in the function alloc_ring when the parameter > sw_size is nonzero and metadata is NULL. I don't know if that can happen. > > Here is the code, taken from the latest git tree. > > static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t elem_size, > size_t sw_size, dma_addr_t * phys, void *metadata) It might make sense to stick a WARN_ON(sw_size && !metadata) in that function or something like that, but a quick audit of the current code (there are only 4 callers of alloc_ring() in that file -- and it's static so we know those are all of them -- all in t3_sge_alloc_qset()) shows that all callers pass non-NULL metadata if sw_size != 0. - R.
[adding cxgb3 maintainer to cc list] On Sat, 22 Nov 2008 21:35:32 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > (switched to email. Please respond via emailed reply-to-all, not via > the bugzilla web interface). > > On Sat, 22 Nov 2008 21:11:51 -0800 (PST) > bugme-daemon@bugzilla.kernel.org wrote: > > > http://bugzilla.kernel.org/show_bug.cgi?id=12087 > > > > Summary: [drivers/net/cxgb3/sge.c:563]: Possible memory > > leak: s Product: Drivers > > Version: 2.5 > > KernelVersion: latest git tree > > Platform: All > > OS/Version: Linux > > Tree: Mainline > > Status: NEW > > Severity: normal > > Priority: P1 > > Component: Network > > AssignedTo: jgarzik@pobox.com > > ReportedBy: danielm77@spray.se > > > > > > There will be a memory leak in the function alloc_ring when the > > parameter sw_size is nonzero and metadata is NULL. I don't know if > > that can happen. > > > > Here is the code, taken from the latest git tree. > > > > static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t > > elem_size, size_t sw_size, dma_addr_t * phys, void *metadata) > > { > > size_t len = nelem * elem_size; > > void *s = NULL; > > void *p = dma_alloc_coherent(&pdev->dev, len, phys, > > GFP_KERNEL); > > > > if (!p) > > return NULL; > > if (sw_size) { > > s = kcalloc(nelem, sw_size, GFP_KERNEL); > > > > if (!s) { > > dma_free_coherent(&pdev->dev, len, p, > > *phys); return NULL; > > } > > } > > if (metadata) > > *(void **)metadata = s; > > memset(p, 0, len); > > return p; > > } > > > > > > yeah, that is a bit silly-looking. > -- > 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
Reply-To: divy@chelsio.com Jay Cliburn wrote: > [adding cxgb3 maintainer to cc list] Thanks, I'll look at this. Divy > > On Sat, 22 Nov 2008 21:35:32 -0800 > Andrew Morton <akpm@linux-foundation.org> wrote: > >> >> (switched to email. Please respond via emailed reply-to-all, not via >> the bugzilla web interface). >> >> On Sat, 22 Nov 2008 21:11:51 -0800 (PST) >> bugme-daemon@bugzilla.kernel.org wrote: >> >> > http://bugzilla.kernel.org/show_bug.cgi?id=12087 >> > >> > Summary: [drivers/net/cxgb3/sge.c:563]: Possible memory >> > leak: s Product: Drivers >> > Version: 2.5 >> > KernelVersion: latest git tree >> > Platform: All >> > OS/Version: Linux >> > Tree: Mainline >> > Status: NEW >> > Severity: normal >> > Priority: P1 >> > Component: Network >> > AssignedTo: jgarzik@pobox.com >> > ReportedBy: danielm77@spray.se >> > >> > >> > There will be a memory leak in the function alloc_ring when the >> > parameter sw_size is nonzero and metadata is NULL. I don't know if >> > that can happen. >> > >> > Here is the code, taken from the latest git tree. >> > >> > static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t >> > elem_size, size_t sw_size, dma_addr_t * phys, void *metadata) >> > { >> > size_t len = nelem * elem_size; >> > void *s = NULL; >> > void *p = dma_alloc_coherent(&pdev->dev, len, phys, >> > GFP_KERNEL); >> > >> > if (!p) >> > return NULL; >> > if (sw_size) { >> > s = kcalloc(nelem, sw_size, GFP_KERNEL); >> > >> > if (!s) { >> > dma_free_coherent(&pdev->dev, len, p, >> > *phys); return NULL; >> > } >> > } >> > if (metadata) >> > *(void **)metadata = s; >> > memset(p, 0, len); >> > return p; >> > } >> > >> > >> >> yeah, that is a bit silly-looking. >> -- >> 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 >
Reply-To: divy@chelsio.com Andrew Morton wrote: > (switched to email. Please respond via emailed reply-to-all, not via the > bugzilla web interface). > > On Sat, 22 Nov 2008 21:11:51 -0800 (PST) bugme-daemon@bugzilla.kernel.org > wrote: > >> http://bugzilla.kernel.org/show_bug.cgi?id=12087 >> >> Summary: [drivers/net/cxgb3/sge.c:563]: Possible memory leak: s >> Product: Drivers >> Version: 2.5 >> KernelVersion: latest git tree >> Platform: All >> OS/Version: Linux >> Tree: Mainline >> Status: NEW >> Severity: normal >> Priority: P1 >> Component: Network >> AssignedTo: jgarzik@pobox.com >> ReportedBy: danielm77@spray.se >> >> >> There will be a memory leak in the function alloc_ring when the parameter >> sw_size is nonzero and metadata is NULL. I don't know if that can happen. >> >> Here is the code, taken from the latest git tree. >> >> static void *alloc_ring(struct pci_dev *pdev, size_t nelem, size_t >> elem_size, >> size_t sw_size, dma_addr_t * phys, void *metadata) >> { >> size_t len = nelem * elem_size; >> void *s = NULL; >> void *p = dma_alloc_coherent(&pdev->dev, len, phys, GFP_KERNEL); >> >> if (!p) >> return NULL; >> if (sw_size) { >> s = kcalloc(nelem, sw_size, GFP_KERNEL); >> >> if (!s) { >> dma_free_coherent(&pdev->dev, len, p, *phys); >> return NULL; >> } >> } >> if (metadata) >> *(void **)metadata = s; >> memset(p, 0, len); >> return p; >> } >> >> > > yeah, that is a bit silly-looking. The following patch should hopefully take care of this issue: http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=5256554489531f3e177e7308752d8f0681cdd5a6 Cheers, Divy