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

[Nick Krause]

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