Skip to contentSkip to conversation

Checks for Null value in function *get_skub

PATCH Superseded Nicholas Krause 3 participants 5 messages Jun 16, 2014

Subsystems (2)
InfiniBand CXGB4 iWARP RNIC driver (IW_CXGB4)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Nicholas Krause

author
#0 lore raw

PATCH v3 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 = alloc_skb(len, gfp);
 	}
 	t4_set_arp_err_handler(skb, NULL, NULL);
Signature · 6 lines
1.9.1

--
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
author
#1 lore raw

The bug is open here https://bugzilla.kernel.org/show_bug.cgi?id=44631.
I would recommend closing it now as I seem to have fixed it. Cheers Nick

Quoted 24 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 +2 -1
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 = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);
--
Quoted 2 lines

1.9.1

--
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

#2 lore raw
In reply to Nicholas Krause
Quoted 14 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 = alloc_skb(len, gfp);
  	}
  	t4_set_arp_err_handler(skb, NULL, NULL);

Isn't kfree(NULL) legal?

(i.e. the if statement is useless)

Thanks,
Levente Kurusa

author
#3 lore raw
Quoted 1 line

From what I know yes so I would close the bug at the the link in

my previous email and just void my patch.
Cheers Nick

Quoted 30 lines · Levente Kurusa
Quoted 16 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 = alloc_skb(len, gfp);
        }
        t4_set_arp_err_handler(skb, NULL, NULL);

Isn't kfree(NULL) legal?

Quoted 4 lines

(i.e. the if statement is useless)

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

#4 lore raw
Quoted 3 lines · Nick Krause
Quoted 1 line

From what I know yes so I would close the bug at the the link in

my previous email and just void my patch.
Cheers Nick

We shouldn't be kfree-ing an skb anyway. Should use kfree_skb().

There is still a leak if skb is non-null and the skb is non linear or cloned.

Quoted 29 lines · Levente Kurusa
Quoted 28 lines · Nicholas Krause
Quoted 15 lines
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 = alloc_skb(len, gfp);
         }
         t4_set_arp_err_handler(skb, NULL, NULL);

Isn't kfree(NULL) legal?

Quoted 4 lines
Quoted 4 lines

(i.e. the if statement is useless)

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