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
