* [PATCH 1/1] net:sched fix a bug about memery leak
@ 2013-10-24 9:06 Jing Wang
2013-10-24 14:05 ` Sergei Shtylyov
0 siblings, 1 reply; 7+ messages in thread
From: Jing Wang @ 2013-10-24 9:06 UTC (permalink / raw)
To: netdev; +Cc: Jing Wang
From: Jing Wang <windsdaemon@gmail.com>
the code isn't properly release memory
Signed-off-by: Jing Wang <windsdaemon@gmail.com>
---
net/sched/cls_route.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 37da567..118f8d5 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -466,11 +466,11 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
goto reinsert;
}
- err = -ENOBUFS;
+ err = -ENOMEM;
if (head == NULL) {
head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
if (head == NULL)
- goto errout;
+ goto errhead;
tcf_tree_lock(tp);
tp->root = head;
@@ -479,7 +479,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
if (f == NULL)
- goto errout;
+ goto errflt;
err = route4_set_parms(net, tp, base, f, handle, head, tb,
tca[TCA_RATE], 1);
@@ -517,6 +517,9 @@ reinsert:
errout:
kfree(f);
+errflt:
+ kfree(head);
+errhead:
return err;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 1/1] net:sched fix a bug about memery leak
@ 2013-10-24 9:12 Jing Wang
2013-10-24 9:33 ` Eric Dumazet
2013-10-24 14:10 ` Sergei Shtylyov
0 siblings, 2 replies; 7+ messages in thread
From: Jing Wang @ 2013-10-24 9:12 UTC (permalink / raw)
To: davem, jhs, netdev; +Cc: Jing Wang
From: Jing Wang <windsdaemon@gmail.com>
the code isn't properly release memory
Signed-off-by: Jing Wang <windsdaemon@gmail.com>
---
net/sched/cls_route.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 37da567..118f8d5 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -466,11 +466,11 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
goto reinsert;
}
- err = -ENOBUFS;
+ err = -ENOMEM;
if (head == NULL) {
head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
if (head == NULL)
- goto errout;
+ goto errhead;
tcf_tree_lock(tp);
tp->root = head;
@@ -479,7 +479,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
if (f == NULL)
- goto errout;
+ goto errflt;
err = route4_set_parms(net, tp, base, f, handle, head, tb,
tca[TCA_RATE], 1);
@@ -517,6 +517,9 @@ reinsert:
errout:
kfree(f);
+errflt:
+ kfree(head);
+errhead:
return err;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] net:sched fix a bug about memery leak
2013-10-24 9:12 [PATCH 1/1] net:sched fix a bug about memery leak Jing Wang
@ 2013-10-24 9:33 ` Eric Dumazet
2013-10-24 14:07 ` Christoph Paasch
2013-10-24 14:10 ` Sergei Shtylyov
1 sibling, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2013-10-24 9:33 UTC (permalink / raw)
To: Jing Wang; +Cc: davem, jhs, netdev
On Thu, 2013-10-24 at 17:12 +0800, Jing Wang wrote:
> From: Jing Wang <windsdaemon@gmail.com>
>
> the code isn't properly release memory
>
> Signed-off-by: Jing Wang <windsdaemon@gmail.com>
> ---
> net/sched/cls_route.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 37da567..118f8d5 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -466,11 +466,11 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> goto reinsert;
> }
>
> - err = -ENOBUFS;
> + err = -ENOMEM;
> if (head == NULL) {
> head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> if (head == NULL)
> - goto errout;
> + goto errhead;
>
> tcf_tree_lock(tp);
> tp->root = head;
> @@ -479,7 +479,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
>
> f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
> if (f == NULL)
> - goto errout;
> + goto errflt;
>
> err = route4_set_parms(net, tp, base, f, handle, head, tb,
> tca[TCA_RATE], 1);
> @@ -517,6 +517,9 @@ reinsert:
>
> errout:
> kfree(f);
> +errflt:
> + kfree(head);
> +errhead:
> return err;
> }
>
I don't think this patch is needed or correct.
tp->root is the head, you cannot free it like that.
It will be freed properly in route4_destroy()
Please elaborate, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] net:sched fix a bug about memery leak
2013-10-24 9:06 Jing Wang
@ 2013-10-24 14:05 ` Sergei Shtylyov
0 siblings, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2013-10-24 14:05 UTC (permalink / raw)
To: Jing Wang, netdev
Hello.
On 10/24/2013 01:06 PM, Jing Wang wrote:
> From: Jing Wang <windsdaemon@gmail.com>
> the code isn't properly release memory
> Signed-off-by: Jing Wang <windsdaemon@gmail.com>
> ---
> net/sched/cls_route.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 37da567..118f8d5 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
[...]
> @@ -517,6 +517,9 @@ reinsert:
>
> errout:
> kfree(f);
> +errflt:
> + kfree(head);
Please indent with tab, not spaces.
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] net:sched fix a bug about memery leak
2013-10-24 9:33 ` Eric Dumazet
@ 2013-10-24 14:07 ` Christoph Paasch
2013-10-24 14:23 ` Christoph Paasch
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Paasch @ 2013-10-24 14:07 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jing Wang, davem, jhs, netdev
On 24/10/13 - 02:33:38, Eric Dumazet wrote:
> On Thu, 2013-10-24 at 17:12 +0800, Jing Wang wrote:
> > From: Jing Wang <windsdaemon@gmail.com>
> >
> > the code isn't properly release memory
> >
> > Signed-off-by: Jing Wang <windsdaemon@gmail.com>
> > ---
> > net/sched/cls_route.c | 9 ++++++---
> > 1 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> > index 37da567..118f8d5 100644
> > --- a/net/sched/cls_route.c
> > +++ b/net/sched/cls_route.c
> > @@ -466,11 +466,11 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> > goto reinsert;
> > }
> >
> > - err = -ENOBUFS;
> > + err = -ENOMEM;
> > if (head == NULL) {
> > head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> > if (head == NULL)
> > - goto errout;
> > + goto errhead;
> >
> > tcf_tree_lock(tp);
> > tp->root = head;
> > @@ -479,7 +479,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> >
> > f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
> > if (f == NULL)
> > - goto errout;
> > + goto errflt;
> >
> > err = route4_set_parms(net, tp, base, f, handle, head, tb,
> > tca[TCA_RATE], 1);
> > @@ -517,6 +517,9 @@ reinsert:
> >
> > errout:
> > kfree(f);
> > +errflt:
> > + kfree(head);
> > +errhead:
> > return err;
> > }
> >
>
> I don't think this patch is needed or correct.
>
> tp->root is the head, you cannot free it like that.
>
> It will be freed properly in route4_destroy()
>
> Please elaborate, thanks.
I think there is something else wrong in route4_change:
----
>From 1409402bf964bef79667755a5d0d5e0c2bd663f3 Mon Sep 17 00:00:00 2001
From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Thu, 24 Oct 2013 15:33:28 +0200
Subject: [PATCH net] net: sched: Don't free f before it is allocated in
route4_change
f is set to *arg in route4_change at the beginning, which points to a
route4_filter in the hash-table (gotten through route4_get, called by
tc_ctl_filter). If the alloc of head fails, we should not goto errout,
because this will free f and thus freed memory will be referenced by
the hash-table.
Only later the pointer f will change to an allocated route4_filter.
This patch returns err if the allocation of head fails as f has not yet
been allocated inside route4_change.
Seems the code has been like this since Linus's original git-commit.
Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
---
net/sched/cls_route.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 37da567..f17c67f 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -470,7 +470,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
if (head == NULL) {
head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
if (head == NULL)
- goto errout;
+ return err;
tcf_tree_lock(tp);
tp->root = head;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] net:sched fix a bug about memery leak
2013-10-24 9:12 [PATCH 1/1] net:sched fix a bug about memery leak Jing Wang
2013-10-24 9:33 ` Eric Dumazet
@ 2013-10-24 14:10 ` Sergei Shtylyov
1 sibling, 0 replies; 7+ messages in thread
From: Sergei Shtylyov @ 2013-10-24 14:10 UTC (permalink / raw)
To: Jing Wang, davem, jhs, netdev
Hello.
On 10/24/2013 01:12 PM, Jing Wang wrote:
s/mememry/memory/ in the subject.
> From: Jing Wang <windsdaemon@gmail.com>
> the code isn't properly release memory
s/release/releasing/.
> Signed-off-by: Jing Wang <windsdaemon@gmail.com>
WBR, Sergei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] net:sched fix a bug about memery leak
2013-10-24 14:07 ` Christoph Paasch
@ 2013-10-24 14:23 ` Christoph Paasch
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Paasch @ 2013-10-24 14:23 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jing Wang, davem, jhs, netdev
On 24/10/13 - 16:07:09, Christoph Paasch wrote:
> On 24/10/13 - 02:33:38, Eric Dumazet wrote:
> > On Thu, 2013-10-24 at 17:12 +0800, Jing Wang wrote:
> > > From: Jing Wang <windsdaemon@gmail.com>
> > >
> > > the code isn't properly release memory
> > >
> > > Signed-off-by: Jing Wang <windsdaemon@gmail.com>
> > > ---
> > > net/sched/cls_route.c | 9 ++++++---
> > > 1 files changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> > > index 37da567..118f8d5 100644
> > > --- a/net/sched/cls_route.c
> > > +++ b/net/sched/cls_route.c
> > > @@ -466,11 +466,11 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> > > goto reinsert;
> > > }
> > >
> > > - err = -ENOBUFS;
> > > + err = -ENOMEM;
> > > if (head == NULL) {
> > > head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> > > if (head == NULL)
> > > - goto errout;
> > > + goto errhead;
> > >
> > > tcf_tree_lock(tp);
> > > tp->root = head;
> > > @@ -479,7 +479,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> > >
> > > f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
> > > if (f == NULL)
> > > - goto errout;
> > > + goto errflt;
> > >
> > > err = route4_set_parms(net, tp, base, f, handle, head, tb,
> > > tca[TCA_RATE], 1);
> > > @@ -517,6 +517,9 @@ reinsert:
> > >
> > > errout:
> > > kfree(f);
> > > +errflt:
> > > + kfree(head);
> > > +errhead:
> > > return err;
> > > }
> > >
> >
> > I don't think this patch is needed or correct.
> >
> > tp->root is the head, you cannot free it like that.
> >
> > It will be freed properly in route4_destroy()
> >
> > Please elaborate, thanks.
>
> I think there is something else wrong in route4_change:
>
> ----
> From 1409402bf964bef79667755a5d0d5e0c2bd663f3 Mon Sep 17 00:00:00 2001
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> Date: Thu, 24 Oct 2013 15:33:28 +0200
> Subject: [PATCH net] net: sched: Don't free f before it is allocated in
> route4_change
>
> f is set to *arg in route4_change at the beginning, which points to a
> route4_filter in the hash-table (gotten through route4_get, called by
> tc_ctl_filter). If the alloc of head fails, we should not goto errout,
> because this will free f and thus freed memory will be referenced by
> the hash-table.
> Only later the pointer f will change to an allocated route4_filter.
>
> This patch returns err if the allocation of head fails as f has not yet
> been allocated inside route4_change.
>
> Seems the code has been like this since Linus's original git-commit.
>
> Signed-off-by: Christoph Paasch <christoph.paasch@uclouvain.be>
> ---
> net/sched/cls_route.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 37da567..f17c67f 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -470,7 +470,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> if (head == NULL) {
> head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> if (head == NULL)
> - goto errout;
> + return err;
Oh... copy-paste from vim into mutt actually replaced the tabs by spaces...
I will resend. Sorry.
Christoph
>
> tcf_tree_lock(tp);
> tp->root = head;
> --
> 1.8.3.2
>
> --
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-24 14:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-24 9:12 [PATCH 1/1] net:sched fix a bug about memery leak Jing Wang
2013-10-24 9:33 ` Eric Dumazet
2013-10-24 14:07 ` Christoph Paasch
2013-10-24 14:23 ` Christoph Paasch
2013-10-24 14:10 ` Sergei Shtylyov
-- strict thread matches above, loose matches on Subject: below --
2013-10-24 9:06 Jing Wang
2013-10-24 14:05 ` Sergei Shtylyov
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).