| Message ID | 1388776399-27657-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri, 3 Jan 2014 11:13:19 -0800 > Fix it by moving allocation to ->init(). > > Cc: Thomas Graf <tgraf@suug.ch> > Cc: David S. Miller <davem@davemloft.net> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> I don't understand how the memory leak can happen, please explain it in your commit message. Also: > { > + struct cls_cgroup_head *head; > + head = kzalloc(sizeof(*head), GFP_KERNEL); Please add an empty line between local variable declarations and code. Thanks. -- 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
On Fri, Jan 3, 2014 at 6:02 PM, David Miller <davem@davemloft.net> wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Fri, 3 Jan 2014 11:13:19 -0800 > >> Fix it by moving allocation to ->init(). >> >> Cc: Thomas Graf <tgraf@suug.ch> >> Cc: David S. Miller <davem@davemloft.net> >> Cc: Jamal Hadi Salim <jhs@mojatatu.com> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > I don't understand how the memory leak can happen, please explain > it in your commit message. > The leak happens when ->change() fails after the allocation inside cls_cgroup_change(), its caller only does cleanup when itself creates one. So, the callee should do cleanup on error path by itself. But I may miss something. Since it is not urgent at all, I will explain this in changelog and resend it for net-next. > Also: > >> { >> + struct cls_cgroup_head *head; >> + head = kzalloc(sizeof(*head), GFP_KERNEL); > > Please add an empty line between local variable declarations > and code. > OK. -- 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
On 01/06/14 at 03:23pm, Cong Wang wrote: > On Fri, Jan 3, 2014 at 6:02 PM, David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > > Date: Fri, 3 Jan 2014 11:13:19 -0800 > > > >> Fix it by moving allocation to ->init(). > >> > >> Cc: Thomas Graf <tgraf@suug.ch> > >> Cc: David S. Miller <davem@davemloft.net> > >> Cc: Jamal Hadi Salim <jhs@mojatatu.com> > >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > > I don't understand how the memory leak can happen, please explain > > it in your commit message. > > > > The leak happens when ->change() fails after the allocation > inside cls_cgroup_change(), its caller only does cleanup > when itself creates one. So, the callee should do cleanup > on error path by itself. But I may miss something. > > Since it is not urgent at all, I will explain this in changelog > and resend it for net-next. I have no problem with the intent of the change but I want to note that the behavior was introduced intentionally to be in line with behaviour of other classifiers that use chaining. It's not a leak, the reference is kept and freed when the chain itself is deleted. -- 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/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c index 16006c9..f0d1e81 100644 --- a/net/sched/cls_cgroup.c +++ b/net/sched/cls_cgroup.c @@ -169,6 +169,11 @@ static void cls_cgroup_put(struct tcf_proto *tp, unsigned long f) static int cls_cgroup_init(struct tcf_proto *tp) { + struct cls_cgroup_head *head; + head = kzalloc(sizeof(*head), GFP_KERNEL); + if (head == NULL) + return -ENOBUFS; + tp->root = head; return 0; } @@ -195,21 +200,9 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb, if (!tca[TCA_OPTIONS]) return -EINVAL; - if (head == NULL) { - if (!handle) - return -EINVAL; - - head = kzalloc(sizeof(*head), GFP_KERNEL); - if (head == NULL) - return -ENOBUFS; - + if (head->handle == 0) head->handle = handle; - tcf_tree_lock(tp); - tp->root = head; - tcf_tree_unlock(tp); - } - if (handle != head->handle) return -ENOENT;
Fix it by moving allocation to ->init(). Cc: Thomas Graf <tgraf@suug.ch> Cc: David S. Miller <davem@davemloft.net> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/sched/cls_cgroup.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)