[PATCH v3] net: core: Prevent from dereferencing null pointer when releasing SKB

2017-04-25 Thread Myungho Jung 
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 <mhju...@gmail.com>
---
Changes in v2:
 - Correct category in subject
Changes in v3:
 - Fix typo in subject

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

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(>users) == 1)) {
smp_rmb();
atomic_set(>users, 0);
-- 
2.7.4

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

2017-04-24 Thread Myungho Jung 
On Mon, Apr 24, 2017 at 09:44:50PM -0400, David Miller wrote:
> From: Myungho Jung <mhju...@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 <mhju...@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 <mhju...@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

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

2017-04-24 Thread Myungho Jung 
On Mon, Apr 24, 2017 at 06:10:32PM -0700, Eric Dumazet wrote:
> On Mon, Apr 24, 2017 at 6:00 PM, Myungho Jung <mhju...@gmail.com> wrote:
> > On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
> >> From: Myungho Jung <mhju...@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 <mhju...@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

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

2017-04-24 Thread Myungho Jung 
On Mon, Apr 24, 2017 at 12:02:35PM -0400, David Miller wrote:
> From: Myungho Jung <mhju...@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 <mhju...@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

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

2017-04-20 Thread Myungho Jung 
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 <mhju...@gmail.com>
---
Changes in v2:
 - Correct category in subject

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

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(>users) == 1)) {
smp_rmb();
atomic_set(>users, 0);
-- 
2.7.4

Re: [PATCH] p54: Prevent from dereferencing null pointer when releasing SKB

2017-04-20 Thread Myungho Jung 
On Thu, Apr 20, 2017 at 04:03:43PM -0700, Greg Rose wrote:
> On Thu, 2017-04-20 at 11:25 -0700, Myungho Jung wrote:
> > 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 <mhju...@gmail.com>
> 
> Hi,
> 
> I think the patch is fine but I'm confused by the subject.  You mention
> p54 driver but the change is in dev.c.  I know the bugzilla references
> the p54 but that's not where the change is.
> 
> Seems odd to me.
> 
> - Greg
> 
> > ---
> >  net/core/dev.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > 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(>users) == 1)) {
> > smp_rmb();
> > atomic_set(>users, 0);
> 
> 
> 

Hi Greg,

Thank you for checking my patch. I missed that I moved change from p54
to net/dev. Do I need to resubmit patch v2 to modify subject?

Thanks,
Myungho

[PATCH] p54: Prevent from dereferencing null pointer when releasing SKB

2017-04-20 Thread Myungho Jung 
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 <mhju...@gmail.com>
---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

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(>users) == 1)) {
smp_rmb();
atomic_set(>users, 0);
-- 
2.7.4

[PATCH] p54: Prevent from dereferencing null pointer when releasing SKB

2017-04-10 Thread Myungho Jung 
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 <mhju...@gmail.com>
---
 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

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(>users) == 1)) {
smp_rmb();
atomic_set(>users, 0);
-- 
2.7.4

Re: [PATCH] p54: add null pointer check before releasing socket buffer

2017-04-10 Thread Myungho Jung 
On Mon, Apr 10, 2017 at 02:12:54PM +0200, Christian Lamparter wrote:
> (Added linux-wireless, since this is a wireless driver)
> 
> On Sunday, April 9, 2017 10:23:20 PM CEST Myungho Jung wrote:
> > Kernel panic is caused by trying to dereference null pointer. Check if
> > the pointer is null before freeing space.
> Do you have the kernel panic somewhere?
> I think you have an even bigger problem: You see, in order to get EEPROM
> readback and rx_stats feedback you need to sent a request to the firmware
> and if the response's req_id cookies don't match, you end up filling up 
> the very limited device address space.
> 
> As for adding if (!skb) checks. I think kfree, kfree_skb, dev_kfree_skb 
> (aka consume_skb) all check for null pointers already. So the logical
> thing to do would be to make dev_kfree_skb_irq (which would also fix
> dev_kfree_skb_any) consistent with kfree, kfree_skb, dev_kfree_skb and
> add the check there.
> 
> > Signed-off-by: Myungho Jung <mhju...@gmail.com>
> > ---
> >  drivers/net/wireless/intersil/p54/txrx.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intersil/p54/txrx.c 
> > b/drivers/net/wireless/intersil/p54/txrx.c
> > index 1af7da0..8956061 100644
> > --- a/drivers/net/wireless/intersil/p54/txrx.c
> > +++ b/drivers/net/wireless/intersil/p54/txrx.c
> > @@ -503,7 +503,9 @@ static void p54_rx_eeprom_readback(struct p54_common 
> > *priv,
> >  
> > priv->eeprom = NULL;
> > tmp = p54_find_and_unlink_skb(priv, hdr->req_id);
> > -   dev_kfree_skb_any(tmp);
> > +   if (unlikely(!tmp))
> > +   dev_kfree_skb_any(tmp);
> > +
> > complete(>eeprom_comp);
> >  }
> >  
> > @@ -597,7 +599,9 @@ static void p54_rx_stats(struct p54_common *priv, 
> > struct sk_buff *skb)
> > }
> >  
> > tmp = p54_find_and_unlink_skb(priv, hdr->req_id);
> > -   dev_kfree_skb_any(tmp);
> > +   if (unlikely(!tmp))
> > +   dev_kfree_skb_any(tmp);
> > +
> > complete(>stat_comp);
> >  }
> >  
> > 
> 
> 

I found that the fix was totally opposite to my thought. Sorry about
confusion. I'm not sure it actually caused kernel panic but guessed from
a bug report [https://bugzilla.kernel.org/show_bug.cgi?id=195289]. And
correct fix will be like this:
if (likely(tmp))
dev_kfree_skb_any(tmp);

But, like you said, I think null pointer should be checked in
dev_kfree_skb_irq although already checking before calling it in many
other places. I'll try another patch. Thank you for your advice.

Thanks,
Myungho

[PATCH] p54: add null pointer check before releasing socket buffer

2017-04-09 Thread Myungho Jung 
Kernel panic is caused by trying to dereference null pointer. Check if
the pointer is null before freeing space.

Signed-off-by: Myungho Jung <mhju...@gmail.com>
---
 drivers/net/wireless/intersil/p54/txrx.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intersil/p54/txrx.c 
b/drivers/net/wireless/intersil/p54/txrx.c
index 1af7da0..8956061 100644
--- a/drivers/net/wireless/intersil/p54/txrx.c
+++ b/drivers/net/wireless/intersil/p54/txrx.c
@@ -503,7 +503,9 @@ static void p54_rx_eeprom_readback(struct p54_common *priv,
 
priv->eeprom = NULL;
tmp = p54_find_and_unlink_skb(priv, hdr->req_id);
-   dev_kfree_skb_any(tmp);
+   if (unlikely(!tmp))
+   dev_kfree_skb_any(tmp);
+
complete(>eeprom_comp);
 }
 
@@ -597,7 +599,9 @@ static void p54_rx_stats(struct p54_common *priv, struct 
sk_buff *skb)
}
 
tmp = p54_find_and_unlink_skb(priv, hdr->req_id);
-   dev_kfree_skb_any(tmp);
+   if (unlikely(!tmp))
+   dev_kfree_skb_any(tmp);
+
complete(>stat_comp);
 }
 
-- 
2.7.4