On Tue, Aug 12, 2014 at 10:41 PM, Nick Krause <[email protected]> wrote:
> On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <[email protected]> wrote:
>> On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <[email protected]>
>> wrote:
>>> On Tue Aug 12 14, [email protected] wrote:
>>>> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
>>>> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <[email protected]>
>>>> > 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
>>> [email protected]
>>> 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
[email protected]
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
