netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gre: fix a possible skb leak
@ 2013-06-24 13:26 Eric Dumazet
  2013-06-24 14:03 ` Veaceslav Falico
  2013-06-25 23:08 ` [PATCH] " David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-06-24 13:26 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Pravin B Shelar, Daniel Borkmann

From: Eric Dumazet <edumazet@google.com>

commit 68c331631143 ("v4 GRE: Add TCP segmentation offload for GRE")
added a possible skb leak, because it frees only the head of segment
list, in case a skb_linearize() call fails.

This patch adds a kfree_skb_list() helper to fix the bug.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: Daniel Borkmann <dborkman@redhat.com>
---
 include/linux/skbuff.h |    1 +
 net/core/skbuff.c      |   20 ++++++++++++--------
 net/ipv4/gre.c         |    2 +-
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c676eae..dec1748 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -627,6 +627,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
 }
 
 extern void kfree_skb(struct sk_buff *skb);
+extern void kfree_skb_list(struct sk_buff *segs);
 extern void skb_tx_error(struct sk_buff *skb);
 extern void consume_skb(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index cfd777b..1c1738c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -483,15 +483,8 @@ EXPORT_SYMBOL(skb_add_rx_frag);
 
 static void skb_drop_list(struct sk_buff **listp)
 {
-	struct sk_buff *list = *listp;
-
+	kfree_skb_list(*listp);
 	*listp = NULL;
-
-	do {
-		struct sk_buff *this = list;
-		list = list->next;
-		kfree_skb(this);
-	} while (list);
 }
 
 static inline void skb_drop_fraglist(struct sk_buff *skb)
@@ -651,6 +644,17 @@ void kfree_skb(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(kfree_skb);
 
+void kfree_skb_list(struct sk_buff *segs)
+{
+	while (segs) {
+		struct sk_buff *next = segs->next;
+
+		kfree_skb(segs);
+		segs = next;
+	}
+}
+EXPORT_SYMBOL(kfree_skb_list);
+
 /**
  *	skb_tx_error - report an sk_buff xmit error
  *	@skb: buffer that triggered an error
diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
index b2e805a..7856d16 100644
--- a/net/ipv4/gre.c
+++ b/net/ipv4/gre.c
@@ -178,7 +178,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
 
 				err = __skb_linearize(skb);
 				if (err) {
-					kfree_skb(segs);
+					kfree_skb_list(segs);
 					segs = ERR_PTR(err);
 					goto out;
 				}

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: gre: fix a possible skb leak
  2013-06-24 13:26 [PATCH] gre: fix a possible skb leak Eric Dumazet
@ 2013-06-24 14:03 ` Veaceslav Falico
  2013-06-24 14:36   ` Eric Dumazet
  2013-06-24 14:38   ` Daniel Borkmann
  2013-06-25 23:08 ` [PATCH] " David Miller
  1 sibling, 2 replies; 6+ messages in thread
From: Veaceslav Falico @ 2013-06-24 14:03 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Pravin B Shelar, Daniel Borkmann

On Mon, Jun 24, 2013 at 06:26:00AM -0700, Eric Dumazet wrote:
>From: Eric Dumazet <edumazet@google.com>
>
>commit 68c331631143 ("v4 GRE: Add TCP segmentation offload for GRE")
>added a possible skb leak, because it frees only the head of segment
>list, in case a skb_linearize() call fails.
>
>This patch adds a kfree_skb_list() helper to fix the bug.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Pravin B Shelar <pshelar@nicira.com>
>Cc: Daniel Borkmann <dborkman@redhat.com>
>
>---
>include/linux/skbuff.h |    1 +
> net/core/skbuff.c      |   20 ++++++++++++--------
> net/ipv4/gre.c         |    2 +-
> 3 files changed, 14 insertions(+), 9 deletions(-)
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>index 9c676eae..dec1748 100644
>--- a/include/linux/skbuff.h
>+++ b/include/linux/skbuff.h
>@@ -627,6 +627,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
> }
>
> extern void kfree_skb(struct sk_buff *skb);
>+extern void kfree_skb_list(struct sk_buff *segs);
> extern void skb_tx_error(struct sk_buff *skb);
> extern void consume_skb(struct sk_buff *skb);
> extern void	       __kfree_skb(struct sk_buff *skb);
>diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>index cfd777b..1c1738c 100644
>--- a/net/core/skbuff.c
>+++ b/net/core/skbuff.c
>@@ -483,15 +483,8 @@ EXPORT_SYMBOL(skb_add_rx_frag);
>
> static void skb_drop_list(struct sk_buff **listp)
> {
>-	struct sk_buff *list = *listp;
>-
>+	kfree_skb_list(*listp);
> 	*listp = NULL;
>-
>-	do {
>-		struct sk_buff *this = list;
>-		list = list->next;
>-		kfree_skb(this);
>-	} while (list);
> }
>
> static inline void skb_drop_fraglist(struct sk_buff *skb)
>@@ -651,6 +644,17 @@ void kfree_skb(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(kfree_skb);
>
>+void kfree_skb_list(struct sk_buff *segs)
>+{
>+	while (segs) {
>+		struct sk_buff *next = segs->next;
>+
>+		kfree_skb(segs);
>+		segs = next;
>+	}
>+}
>+EXPORT_SYMBOL(kfree_skb_list);
>+
> /**
>  *	skb_tx_error - report an sk_buff xmit error
>  *	@skb: buffer that triggered an error
>diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
>index b2e805a..7856d16 100644
>--- a/net/ipv4/gre.c
>+++ b/net/ipv4/gre.c
>@@ -178,7 +178,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
>
> 				err = __skb_linearize(skb);
> 				if (err) {
>-					kfree_skb(segs);
>+					kfree_skb_list(segs);
> 					segs = ERR_PTR(err);
> 					goto out;
> 				}
>

Maybe we can also use it here (build-tested only):

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4850dc1..5776819 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2924,10 +2924,7 @@ perform_csum_check:
  	return segs;
  
  err:
-	while ((skb = segs)) {
-		segs = skb->next;
-		kfree_skb(skb);
-	}
+	kfree_skb_list(segs);
  	return ERR_PTR(err);
  }
  EXPORT_SYMBOL_GPL(skb_segment);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 4bcabf3..27ba045 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -575,11 +575,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
  			return 0;
  		}
  
-		while (frag) {
-			skb = frag->next;
-			kfree_skb(frag);
-			frag = skb;
-		}
+		kfree_skb_list(frag);
  		IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
  		return err;
  

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: gre: fix a possible skb leak
  2013-06-24 14:03 ` Veaceslav Falico
@ 2013-06-24 14:36   ` Eric Dumazet
  2013-06-24 14:39     ` Veaceslav Falico
  2013-06-24 14:38   ` Daniel Borkmann
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-06-24 14:36 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: David Miller, netdev, Pravin B Shelar, Daniel Borkmann

On Mon, 2013-06-24 at 16:03 +0200, Veaceslav Falico wrote:

> Maybe we can also use it here (build-tested only):

Sure, but it will be done later on net-next

This patch is a stable candidate, so I made a somewhat minimal change

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: gre: fix a possible skb leak
  2013-06-24 14:03 ` Veaceslav Falico
  2013-06-24 14:36   ` Eric Dumazet
@ 2013-06-24 14:38   ` Daniel Borkmann
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2013-06-24 14:38 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: Eric Dumazet, David Miller, netdev, Pravin B Shelar

On 06/24/2013 04:03 PM, Veaceslav Falico wrote:
> On Mon, Jun 24, 2013 at 06:26:00AM -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
...
> Maybe we can also use it here (build-tested only):

Yep, this clean-up looks good I think. But then -net needs to be merged into
-net-next first (also for the gre_offload patch).

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4850dc1..5776819 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2924,10 +2924,7 @@ perform_csum_check:
>       return segs;
>
>   err:
> -    while ((skb = segs)) {
> -        segs = skb->next;
> -        kfree_skb(skb);
> -    }
> +    kfree_skb_list(segs);
>       return ERR_PTR(err);
>   }
>   EXPORT_SYMBOL_GPL(skb_segment);
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 4bcabf3..27ba045 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -575,11 +575,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
>               return 0;
>           }
>
> -        while (frag) {
> -            skb = frag->next;
> -            kfree_skb(frag);
> -            frag = skb;
> -        }
> +        kfree_skb_list(frag);
>           IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
>           return err;
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: gre: fix a possible skb leak
  2013-06-24 14:36   ` Eric Dumazet
@ 2013-06-24 14:39     ` Veaceslav Falico
  0 siblings, 0 replies; 6+ messages in thread
From: Veaceslav Falico @ 2013-06-24 14:39 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Pravin B Shelar, Daniel Borkmann

On Mon, Jun 24, 2013 at 07:36:00AM -0700, Eric Dumazet wrote:
>On Mon, 2013-06-24 at 16:03 +0200, Veaceslav Falico wrote:
>
>> Maybe we can also use it here (build-tested only):
>
>Sure, but it will be done later on net-next
>
>This patch is a stable candidate, so I made a somewhat minimal change

Makes sense, didn't realize that it's for net, not net-next.

Thank you!

>
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] gre: fix a possible skb leak
  2013-06-24 13:26 [PATCH] gre: fix a possible skb leak Eric Dumazet
  2013-06-24 14:03 ` Veaceslav Falico
@ 2013-06-25 23:08 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-06-25 23:08 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, pshelar, dborkman

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 24 Jun 2013 06:26:00 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> commit 68c331631143 ("v4 GRE: Add TCP segmentation offload for GRE")
> added a possible skb leak, because it frees only the head of segment
> list, in case a skb_linearize() call fails.
> 
> This patch adds a kfree_skb_list() helper to fix the bug.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks Eric.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-06-25 23:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-24 13:26 [PATCH] gre: fix a possible skb leak Eric Dumazet
2013-06-24 14:03 ` Veaceslav Falico
2013-06-24 14:36   ` Eric Dumazet
2013-06-24 14:39     ` Veaceslav Falico
2013-06-24 14:38   ` Daniel Borkmann
2013-06-25 23:08 ` [PATCH] " David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).