[v2] net: core: Prevent from dereferencing null pointer when

Message ID 1492732760-25081-1-git-send-email-mhjungk@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Myungho Jung April 20, 2017, 11:59 p.m.
Added NULL check to make __dev_kfree_skb_irq consistent with kfree
family of functions.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289

Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
Changes in v2:
 - Correct category in subject

 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

David Miller April 24, 2017, 4:02 p.m. | #1
From: Myungho Jung <mhjungk@gmail.com>
Date: Thu, 20 Apr 2017 16:59:20 -0700

> Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> family of functions.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> 
> Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> ---
> Changes in v2:
>  - Correct category in subject

This subject line is an incomplete sentence.

This patch prevents dereferenccing a null pointer when "what"?
Myungho Jung April 25, 2017, 1 a.m. | #2
On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
> From: Myungho Jung <mhjungk@gmail.com>
> Date: Thu, 20 Apr 2017 16:59:20 -0700
> 
> > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> > family of functions.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> > 
> > Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> > ---
> > Changes in v2:
> >  - Correct category in subject
> 
> This subject line is an incomplete sentence.
> 
> This patch prevents dereferenccing a null pointer when "what"?

As it was reported on bugzilla, it would happen when plugging p54 usb device
to RPi2. But, i'm not 100% sure that this patch will resolve the issue because
the reporter didn't try my patch yet and I don't have the device to test.

And there might be some other places calling the function without checking
null pointer. The thing is that only the function don't check null among
kfree functions. So, I just hope this patch will prevent potential oops
issues.

Thanks,
Myungho
Eric Dumazet April 25, 2017, 1:10 a.m. | #3
On Mon, Apr 24, 2017 at 6:00 PM, Myungho Jung <mhjungk@gmail.com> wrote:
> On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
>> From: Myungho Jung <mhjungk@gmail.com>
>> Date: Thu, 20 Apr 2017 16:59:20 -0700
>>
>> > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
>> > family of functions.
>> >
>> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
>> >
>> > Signed-off-by: Myungho Jung <mhjungk@gmail.com>
>> > ---
>> > Changes in v2:
>> >  - Correct category in subject
>>
>> This subject line is an incomplete sentence.
>>
>> This patch prevents dereferenccing a null pointer when "what"?
>
> As it was reported on bugzilla, it would happen when plugging p54 usb device
> to RPi2. But, i'm not 100% sure that this patch will resolve the issue because
> the reporter didn't try my patch yet and I don't have the device to test.
>
> And there might be some other places calling the function without checking
> null pointer. The thing is that only the function don't check null among
> kfree functions. So, I just hope this patch will prevent potential oops
> issues.

Okay, but your patch title was : "net: core: Prevent from
dereferencing null pointer when"

"when" is usually followed by other words.
Myungho Jung April 25, 2017, 1:36 a.m. | #4
On Mon, Apr 24, 2017 at 06:10:32PM -0700, Eric Dumazet wrote:
> On Mon, Apr 24, 2017 at 6:00 PM, Myungho Jung <mhjungk@gmail.com> wrote:
> > On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
> >> From: Myungho Jung <mhjungk@gmail.com>
> >> Date: Thu, 20 Apr 2017 16:59:20 -0700
> >>
> >> > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> >> > family of functions.
> >> >
> >> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> >> >
> >> > Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> >> > ---
> >> > Changes in v2:
> >> >  - Correct category in subject
> >>
> >> This subject line is an incomplete sentence.
> >>
> >> This patch prevents dereferenccing a null pointer when "what"?
> >
> > As it was reported on bugzilla, it would happen when plugging p54 usb device
> > to RPi2. But, i'm not 100% sure that this patch will resolve the issue because
> > the reporter didn't try my patch yet and I don't have the device to test.
> >
> > And there might be some other places calling the function without checking
> > null pointer. The thing is that only the function don't check null among
> > kfree functions. So, I just hope this patch will prevent potential oops
> > issues.
> 
> Okay, but your patch title was : "net: core: Prevent from
> dereferencing null pointer when"
> 
> "when" is usually followed by other words.

Oh, sorry that was typo. I'll remove "when" from subject.

Thanks,
Myungho
David Miller April 25, 2017, 1:44 a.m. | #5
From: Myungho Jung <mhjungk@gmail.com>
Date: Mon, 24 Apr 2017 18:00:52 -0700

> On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
>> From: Myungho Jung <mhjungk@gmail.com>
>> Date: Thu, 20 Apr 2017 16:59:20 -0700
>> 
>> > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
>> > family of functions.
>> > 
>> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
>> > 
>> > Signed-off-by: Myungho Jung <mhjungk@gmail.com>
>> > ---
>> > Changes in v2:
>> >  - Correct category in subject
>> 
>> This subject line is an incomplete sentence.
>> 
>> This patch prevents dereferenccing a null pointer when "what"?
> 
> As it was reported on bugzilla, it would happen when plugging p54 usb device
> to RPi2. But, i'm not 100% sure that this patch will resolve the issue because
> the reporter didn't try my patch yet and I don't have the device to test.
> 
> And there might be some other places calling the function without checking
> null pointer. The thing is that only the function don't check null among
> kfree functions. So, I just hope this patch will prevent potential oops
> issues.

It doesn't check for a NULL pointer because it is almost exclusively
used in the interrupt paths where we know we have a non-NULL skb.
Myungho Jung April 25, 2017, 2:39 a.m. | #6
On Mon, Apr 24, 2017 at 09:44:50PM -0400, David Miller wrote:
> From: Myungho Jung <mhjungk@gmail.com>
> Date: Mon, 24 Apr 2017 18:00:52 -0700
> 
> > On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
> >> From: Myungho Jung <mhjungk@gmail.com>
> >> Date: Thu, 20 Apr 2017 16:59:20 -0700
> >> 
> >> > Added NULL check to make __dev_kfree_skb_irq consistent with kfree
> >> > family of functions.
> >> > 
> >> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
> >> > 
> >> > Signed-off-by: Myungho Jung <mhjungk@gmail.com>
> >> > ---
> >> > Changes in v2:
> >> >  - Correct category in subject
> >> 
> >> This subject line is an incomplete sentence.
> >> 
> >> This patch prevents dereferenccing a null pointer when "what"?
> > 
> > As it was reported on bugzilla, it would happen when plugging p54 usb device
> > to RPi2. But, i'm not 100% sure that this patch will resolve the issue because
> > the reporter didn't try my patch yet and I don't have the device to test.
> > 
> > And there might be some other places calling the function without checking
> > null pointer. The thing is that only the function don't check null among
> > kfree functions. So, I just hope this patch will prevent potential oops
> > issues.
> 
> It doesn't check for a NULL pointer because it is almost exclusively
> used in the interrupt paths where we know we have a non-NULL skb.

Yes, actually null is checked before calling the function in most
cases. That's why my first patch was applied not to net/core but to
p54 driver because I was worried about duplicated checking.

But, Christian suggested this patch to make it consistent with other
kfree functions and consume_skb, and Eric agreed with that.

Thanks,
Myungho

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 7869ae3..22be2a6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2450,6 +2450,9 @@  void __dev_kfree_skb_irq(struct sk_buff *skb, enum skb_free_reason reason)
 {
 	unsigned long flags;
 
+	if (unlikely(!skb))
+		return;
+
 	if (likely(atomic_read(&skb->users) == 1)) {
 		smp_rmb();
 		atomic_set(&skb->users, 0);