On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify@xxxxxxxxx> wrote: > 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 I looked into what Jerry states and he seems to be right. I will paste the code of skb_put for your convenience to check if Jerry and me are right. In addition about the call to write_nic_byte(dev, TPPoll, TPPoll_CQ); is there any good place to put it besides the end of the function seems there isn't. I was wondering if I rewrote this to break out of the loop and keep everything else the same is Ok. Nick _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
