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
| ||
|
Date: Thu, 20 Feb 2014 12:03:39 +0300 From: Dan Carpenter <dan.carpenter@...cle.com> To: "Zhao, Gang" <gamerh2o@...il.com> Cc: One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>, devel@...verdev.osuosl.org, mark.einon@...il.com, linux-kernel@...r.kernel.org Subject: Re: [PATCH] et131x: fix allocation failures On Thu, Feb 20, 2014 at 11:03:45AM +0800, Zhao, Gang wrote: > On Wed, 2014-02-19 at 19:43:15 +0800, One Thousand Gnomes wrote: > > On Wed, 19 Feb 2014 09:14:19 +0800 > > "Zhao\, Gang" <gamerh2o@...il.com> wrote: > > > >> Alan, thanks for resending this patch. But it seems you overlooked > >> something we discussed earlier. > >> > >> On Mon, 2014-02-17 at 22:13:08 +0800, Alan wrote: > >> > We should check the ring allocations don't fail. > >> > If we get a fail we need to clean up properly. The allocator assumes the > >> > deallocator will be used on failure, but it isn't. Make sure the > >> > right deallocator is always called and add a missing check against > >> > fbr allocation failure. > >> > > >> > [v2]: Correct check logic > >> > > >> > Signed-off-by: Alan Cox <alan@...ux.intel.com> > >> > --- > >> > drivers/staging/et131x/et131x.c | 9 +++++++-- > >> > 1 file changed, 7 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c > >> > index 6413500..cc600df 100644 > >> > --- a/drivers/staging/et131x/et131x.c > >> > +++ b/drivers/staging/et131x/et131x.c > >> > @@ -2124,7 +2124,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter) > >> > > >> > /* Alloc memory for the lookup table */ > >> > rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL); > >> > + if (rx_ring->fbr[0] == NULL) > >> > + return -ENOMEM; > >> > rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL); > >> > + if (rx_ring->fbr[1] == NULL) > >> > + return -ENOMEM; > >> > >> Shouldn't rx_ring->fbr[0] be freed when allocation of rx_ring->fbr[1] > >> fails ? Or we will leak memory here. > > > > No.. the tx_dma_memory_free and rx_dma_memory_free functions are > > designed to handle incomplete set up. They are now called on incomplete > > setup and will clean up all the resources. > > > > Yes, you are right. By calling {tx, rx}_dma_memory_free the memory will > be freed. > > But I think a comment is needed here, to make this more clear ? Without > proper comment the above code looks a little strange to let one think > it's right. :) No. We don't need a comment. If people start adding kfree() calls all over the place without thinking then we are already screwed and no comment is going to help us. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists