Checks for Null value in function *get_skub

PATCH In review Nicholas Krause 3 participants 9 messages Jun 16, 2014

Subsystems (2)

InfiniBand CXGB4 iWARP RNIC driver (IW_CXGB4)

lore

Share & tools

lore

Nicholas Krause

author

2014-06-16 15:25 UTC #0 lore raw

PATCH v4 1/1 Checks for Null value in function *get_skub

Signed-off-by Nicholas Krause <[email protected]>

---
 drivers/infiniband/hw/cxgb4/cm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +2 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
  */
 static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
 {
 if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                skb_trim(skb, 0);
                skb_get(skb);
                skb_reset_transport_header(skb);
 	} else {
+		if (skb)
+			kfree_skb (skb);
 		skb = alloc_skb(len, gfp);
 	}
 	t4_set_arp_err_handler(skb, NULL, NULL);

Signature · 1 line

1.9.1

Steve Wise

2014-06-16 15:30 UTC #1 lore raw

Quoted 13 lines · Nicholas Krause

Signed-off-by Nicholas Krause <[email protected]>

---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +0 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {

drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status) · line 352

Quoted 10 lines

drivers/infiniband/hw/cxgb4/cm.c +2 -0

  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
  	} else {
+		if (skb)
+			kfree_skb (skb);
  		skb = alloc_skb(len, gfp);
  	}
  	t4_set_arp_err_handler(skb, NULL, NULL);

Can you change the comment? This patch is now fixing a potential skb leak. Also, kfree_sb() will ignore NULL ptrs, so we could just always call it. But I'd add a comment like /* NULL is ignored */.

Thanks,

Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks,
Levente Kurusa.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Nick Krause

author

2014-06-16 15:37 UTC #3 lore raw

In reply to Steve Wise

Signed-off-by Nicholas Krause <[email protected]>

---
drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-----

 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +1 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
  */
 static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
 {
 if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                skb_trim(skb, 0);
                skb_get(skb);
                skb_reset_transport_header(skb);
        } else {
+               if (skb)

/* NULL is ignored */.

+                       kfree_skb (skb);
                skb = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);

Quoted 32 lines · Steve Wise

Quoted 15 lines · Nicholas Krause
Signed-off-by Nicholas Krause <[email protected]>
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)
drivers/infiniband/hw/cxgb4/cm.c +0 -0
index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status) · line 352
Quoted 10 lines
drivers/infiniband/hw/cxgb4/cm.c +2 -0
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
        } else {
+               if (skb)
+                       kfree_skb (skb);
                skb = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);
Can you change the comment? This patch is now fixing a potential skb leak. Also, kfree_sb() will ignore NULL ptrs, so we could just always call it. But I'd add a comment like /* NULL is ignored */.

Quoted 3 lines

Thanks,
Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Nick Krause

author

2014-06-16 15:45 UTC #4 lore raw

Can we close this bug or is it still a issue as we can' return NULL from kfree or kfree_skb. Here is the bug ID 44631 I would close it if we are done with the bug otherwise I think there are no other issues with allocating a skb,
Cheers Nick

Quoted 60 lines · Nick Krause

Signed-off-by Nicholas Krause <[email protected]>

---
drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-----

 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +2 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
  */
 static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
 {
 if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                skb_trim(skb, 0);
                skb_get(skb);
                skb_reset_transport_header(skb);
        } else {
+               if (skb)
 /* NULL is ignored */.
+                       kfree_skb (skb);
                skb = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);

Quoted 32 lines · Steve Wise

Quoted 15 lines · Nicholas Krause
Signed-off-by Nicholas Krause <[email protected]>
---
  drivers/infiniband/hw/cxgb4/cm.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)
drivers/infiniband/hw/cxgb4/cm.c +0 -0
index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status) · line 352
Quoted 10 lines
drivers/infiniband/hw/cxgb4/cm.c +2 -0
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
        } else {
+               if (skb)
+                       kfree_skb (skb);
                skb = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);
Can you change the comment? This patch is now fixing a potential skb leak. Also, kfree_sb() will ignore NULL ptrs, so we could just always call it. But I'd add a comment like /* NULL is ignored */.

Quoted 3 lines

Quoted 3 lines
Thanks,
Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Steve Wise

2014-06-16 17:18 UTC #5 lore raw

Quoted 6 lines · Nick Krause

Can we close this bug or is it still a issue as we can' return NULL from kfree or kfree_skb. Here is the bug ID 44631 I would close it if we are done with the bug otherwise I think there are no other issues with allocating a skb,

In what database is this bug ID 44631?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Levente Kurusa

2014-06-16 15:45 UTC #6 lore raw

In reply to Nick Krause

Quoted 15 lines · Nick Krause

Signed-off-by Nicholas Krause <[email protected]>

---
drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-----

  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +0 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {

drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status)

Quoted 10 lines

drivers/infiniband/hw/cxgb4/cm.c +2 -0

                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
         } else {
+               if (skb)
 /* NULL is ignored */.
+                       kfree_skb (skb);
                 skb = alloc_skb(len, gfp);
         }
         t4_set_arp_err_handler(skb, NULL, NULL);

This patch is severely whitespace damaged. Can you fix your MUA or try to use git-send-email?

The changelog as well is of low quality. Maybe something like this:

infiniband: cxgb4: fix memory skb memory leak

... would be better.

Also, this fails the build, due to the extra dot character after the comment. :-(

Thanks,
Levente Kurusa.

Nick Krause

author

2014-06-16 16:32 UTC #7 lore raw

Hey Guys ,
It states tot run cleanfile or cleanpatch script but it doesn't work how do I run these scripts.
Thanks Nick

Quoted 37 lines · Levente Kurusa

Quoted 17 lines · Nick Krause
Signed-off-by Nicholas Krause <[email protected]>
---
drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)
drivers/infiniband/hw/cxgb4/cm.c +0 -0
index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {
drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status)
Quoted 10 lines
drivers/infiniband/hw/cxgb4/cm.c +2 -0
                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
         } else {
+               if (skb)
 /* NULL is ignored */.
+                       kfree_skb (skb);
                 skb = alloc_skb(len, gfp);
         }
         t4_set_arp_err_handler(skb, NULL, NULL);
This patch is severely whitespace damaged. Can you fix your MUA or try to use git-send-email?
The changelog as well is of low quality. Maybe something like this:
infiniband: cxgb4: fix memory skb memory leak

Quoted 7 lines

... would be better.
Also, this fails the build, due to the extra dot character after the comment. :-(
Thanks,
Levente Kurusa.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Steve Wise

2014-06-16 16:37 UTC #8 lore raw

In reply to Levente Kurusa

Quoted 31 lines · Levente Kurusa

Quoted 16 lines · Nick Krause

Signed-off-by Nicholas Krause <[email protected]>

---
drivers/infiniband/hw/cxgb4/
cm.c | 8 +++-----

  1 file changed, 3 insertions(+), 5 deletions(-)

drivers/infiniband/hw/cxgb4/cm.c +0 -0

index f9477e2..2d56983 100644
@@ -340,15 +340,13 @@ static int status2errno(int status)
   */
  static struct sk_buff *get_skb(struct sk_buff *skb, int len, gfp_t 
 gfp)
  {
  if (skb && !skb_is_nonlinear(skb) && !skb_cloned(skb)) {

drivers/infiniband/hw/cxgb4/cm.c static int status2errno(int status)

Quoted 10 lines

drivers/infiniband/hw/cxgb4/cm.c +2 -0

                 skb_trim(skb, 0);
                 skb_get(skb);
                 skb_reset_transport_header(skb);
         } else {
+               if (skb)
 /* NULL is ignored */.
+                       kfree_skb (skb);
                 skb = alloc_skb(len, gfp);
         }
         t4_set_arp_err_handler(skb, NULL, NULL);

This patch is severely whitespace damaged. Can you fix your MUA or try to use git-send-email?

Quoted 9 lines

The changelog as well is of low quality. Maybe something like this:
infiniband: cxgb4: fix memory skb memory leak
... would be better.
Also, this fails the build, due to the extra dot character after the comment. :-(

Agreed. Start over with this patch, do what we asked, and make sure it passes checkpatch...

Quoted 2 lines

Thanks,
Levente Kurusa.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html