From: Christoph Paasch <christoph.paasch@uclouvain•be>
To: Eric Dumazet <eric.dumazet@gmail•com>
Cc: Jing Wang <windsdaemon@gmail•com>,
davem@davemloft•net, jhs@mojatatu•com, netdev@vger•kernel.org
Subject: Re: [PATCH 1/1] net:sched fix a bug about memery leak
Date: Thu, 24 Oct 2013 16:23:59 +0200 [thread overview]
Message-ID: <20131024142359.GC19470@cpaasch-mac> (raw)
In-Reply-To: <20131024140709.GA19470@cpaasch-mac>
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
next prev parent reply other threads:[~2013-10-24 14:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131024142359.GC19470@cpaasch-mac \
--to=christoph.paasch@uclouvain$(echo .)be \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=windsdaemon@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox