On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@xxxxxxxxxxxxxx> wrote: > On Tue Aug 12 14, Valdis.Kletnieks@xxxxxx wrote: >> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said: >> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@xxxxxxxxx> wrote: >> > > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461. >> > > This entry states that we are not checking the skb allocated in fw_download_code >> > > for NULL and after checking it ,I fixed it to check for the NULL value before >> > > returning false and exiting fw_download_code cleanly. >> >> > I am trying to get this patch merged and after my issues with the >> > kernel community, I can't get this into the mainline. >> >> No, you're having trouble getting this into mainline because you are *STILL* >> persisting in submitting patches that are buggy. >> >> In this case, the problem is you *DON'T* exit the function cleanly. >> >> Note your patch causes an immediate return from inside a do/while loop, which >> *also* contains: >> >> skb_put(skb, i); >> >> So if there's (say) 3 fragments needed, and we fail on the allocation of the >> third one, you just leaked references to the first two fragments, and never >> actually clean up the allocations, so we have references to leaked memory. And >> leaking memory in a case where we're almost certainly very close to OOM isn't >> exactly a good idea. Yes, failing to check the return code is a bug - but so is >> failing to unwind the allocations already made. >> >> It took me all of a minute to spot this issue - the only clue needed was that there >> was a '*_put()' call in the function, which should be a warning flag that reference >> counting needs to be checked. >> >> Greg: Consider this a NACK of this patch. >> >> Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*. >> >> Seriously Nick. *PLEASE* stop posting patches until you've gotten a better handle >> on what code maintenance really entails. >> > > A minor point, but I don't believe skb_put() has anything to do with reference counting, > though the name would make you think so. sk_buff reference counting happens in skb_get() > and the *free_skb() routines from the looks of it. > > Jerry > > _______________________________________________ > Kernelnewbies mailing list > Kernelnewbies@xxxxxxxxxxxxxxxxx > http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies Sorry Guys, I will reread the function and send out a patch that is bug free and actually works. Thanks Greg for at least reading it for now :). Cheers Nick _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
