Messages in this thread Patch in this message | | | Date | Wed, 2 Nov 2022 20:26:04 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH] net: sched: fix memory leak in tcindex_set_parms |
| |
On Mon, 31 Oct 2022 14:08:35 +0800 Hawkins Jiawei wrote: > Kernel will uses tcindex_change() to change an existing
s/will//
> traffic-control-indices filter properties. During the > process of changing, kernel will clears the old
s/will//
> traffic-control-indices filter result, and updates it > by RCU assigning new traffic-control-indices data. > > Yet the problem is that, kernel will clears the old
s/will//
> traffic-control-indices filter result, without destroying > its tcf_exts structure, which triggers the above > memory leak. > > This patch solves it by using tcf_exts_destroy() to > destroy the tcf_exts structure in old > traffic-control-indices filter result. >
Please provide a Fixes tag to where the problem was introduced (or the initial git commit).
> Link: https://lore.kernel.org/all/0000000000001de5c505ebc9ec59@google.com/ > Reported-by: syzbot+232ebdbd36706c965ebf@syzkaller.appspotmail.com > Tested-by: syzbot+232ebdbd36706c965ebf@syzkaller.appspotmail.com > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > net/sched/cls_tcindex.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c > index 1c9eeb98d826..dc872a794337 100644 > --- a/net/sched/cls_tcindex.c > +++ b/net/sched/cls_tcindex.c > @@ -338,6 +338,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > struct tcf_result cr = {}; > int err, balloc = 0; > struct tcf_exts e; > +#ifdef CONFIG_NET_CLS_ACT > + struct tcf_exts old_e = {}; > +#endif
Why all the ifdefs?
> err = tcf_exts_init(&e, net, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE); > if (err < 0) > @@ -479,6 +482,14 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > } > > if (old_r && old_r != r) { > +#ifdef CONFIG_NET_CLS_ACT > + /* r->exts is not copied from old_r->exts, and > + * the following code will clears the old_r, so > + * we need to destroy it after updating the tp->root, > + * to avoid memory leak bug. > + */ > + old_e = old_r->exts; > +#endif
Can't you localize all the changes to this if block?
Maybe add a function called tcindex_filter_result_reinit() which will act more appropriately?
> err = tcindex_filter_result_init(old_r, cp, net); > if (err < 0) { > kfree(f); > @@ -510,6 +521,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > tcf_exts_destroy(&new_filter_result.exts); > } > > +#ifdef CONFIG_NET_CLS_ACT > + tcf_exts_destroy(&old_e); > +#endif > if (oldp) > tcf_queue_work(&oldp->rwork, tcindex_partial_destroy_work); > return 0;
| |