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

[Jerry Snitselaar]

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