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

[Nick Krause]

On Tue, Aug 12, 2014 at 10:41 PM, Nick Krause <xerofoify at gmail.com> wrote:
> 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
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);