[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify at gmail.com> wrote:
> On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev at snitselaar.org> wrote:
>> On Tue Aug 12 14, Valdis.Kletnieks at vt.edu 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 at gmail.com> 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 at kernelnewbies.org
>> 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