Messages in this thread Patch in this message | | | From | Hawkins Jiawei <> | Subject | Re: [PATCH] net: sched: fix memory leak in tcindex_set_parms | Date | Tue, 8 Nov 2022 00:00:36 +0800 |
| |
On Mon, 7 Nov 2022 at 01:49, Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Sun, Nov 06, 2022 at 10:55:31PM +0800, Hawkins Jiawei wrote: > > Hi Cong, > > > > > > > > > > > diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c > > > index 1c9eeb98d826..00a6c04a4b42 100644 > > > --- a/net/sched/cls_tcindex.c > > > +++ b/net/sched/cls_tcindex.c > > > @@ -479,6 +479,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > > > } > > > > > > if (old_r && old_r != r) { > > > + tcf_exts_destroy(&old_r->exts); > > > err = tcindex_filter_result_init(old_r, cp, net); > > > if (err < 0) { > > > kfree(f); > > > > As for the position of the tcf_exts_destroy(), should we > > call it after the RCU updating, after > > `rcu_assign_pointer(tp->root, cp)` ? > > > > Or the concurrent RCU readers may derefer this freed memory > > (Please correct me If I am wrong). > > I don't think so, because we already have tcf_exts_change() in multiple > places within tcindex_set_parms(). Even if this is really a problem,
Do you mean that, if this is a problem, then these tcf_exts_change() should have already triggered the Use-after-Free?(Please correct me if I get wrong)
But it seems that these tcf_exts_change() don't destory the old_r, so it doesn't face the above concurrent problems.
I find there are two tcf_exts_chang() in tcindex_set_parms(). One is
oldp = p; r->res = cr; tcf_exts_change(&r->exts, &e);
rcu_assign_pointer(tp->root, cp);
the other is
f->result.res = r->res; tcf_exts_change(&f->result.exts, &r->exts);
fp = cp->h + (handle % cp->hash); for (nfp = rtnl_dereference(*fp); nfp; fp = &nfp->next, nfp = rtnl_dereference(*fp)) ; /* nothing */
rcu_assign_pointer(*fp, f);
*r->exts* or *f->result.exts*, both are newly allocated in `tcindex_set_params()`, so the concurrent RCU readers won't read them before RCU updating.
> moving it after rcu_assign_pointer() does not help, you need to wait for > a grace period.
Yes, you are right. So if this is really a problem, I wonder if we can add the synchronize_rcu() before freeing the old->exts, like:
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c index 1c9eeb98d826..57d900c664cf 100644 --- a/net/sched/cls_tcindex.c +++ b/net/sched/cls_tcindex.c @@ -338,6 +338,7 @@ 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; + struct tcf_exts old_e = {}; err = tcf_exts_init(&e, net, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE); if (err < 0) @@ -479,6 +480,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, } if (old_r && old_r != r) { + old_e = old_r->exts; err = tcindex_filter_result_init(old_r, cp, net); if (err < 0) { kfree(f); @@ -510,6 +512,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, tcf_exts_destroy(&new_filter_result.exts); } + synchronize_rcu(); + tcf_exts_destroy(&old_e); + if (oldp) tcf_queue_work(&oldp->rwork, tcindex_partial_destroy_work); return 0; > > Thanks.
| |