On Tue, Aug 12, 2014 at 10:41 PM, Nick Krause <xerofoify@xxxxxxxxx> wrote: > 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 Sorry about the multiple emails. Here is skb_put for you all to read. Nick 1271 unsigned char *skb_put(struct sk_buff *skb, unsigned int len) 1272 { 1273 unsigned char *tmp = skb_tail_pointer(skb); 1274 SKB_LINEAR_ASSERT(skb); 1275 skb->tail += len; 1276 skb->len += len; 1277 if (unlikely(skb->tail > skb->end)) 1278 skb_over_panic(skb, len, __builtin_return_address(0)); 1279 return tmp; 1280 } 1281 EXPORT_SYMBOL(skb_put); _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
