Messages in this thread Patch in this message |  | | Date | Sat, 3 Dec 2022 12:19:12 -0800 | From | Cong Wang <> | Subject | Re: [PATCH v3] net: sched: fix memory leak in tcindex_set_parms |
| |
On Tue, Nov 29, 2022 at 10:52:49AM +0800, Hawkins Jiawei wrote: > Kernel uses tcindex_change() to change an existing > filter properties. During the process of changing, > kernel uses tcindex_alloc_perfect_hash() to newly > allocate filter results, uses tcindex_filter_result_init() > to clear the old filter result. > > Yet the problem is that, kernel clears the old > filter result, without destroying its tcf_exts structure, > which triggers the above memory leak. > > Considering that there already extis a tc_filter_wq workqueue > to destroy the old tcindex_data by tcindex_partial_destroy_work() > at the end of tcindex_set_parms(), this patch solves this memory > leak bug by removing this old filter result clearing part, > and delegating it to the tc_filter_wq workqueue.
Hmm?? The tcindex_partial_destroy_work() is to destroy 'oldp' which is different from 'old_r'. I mean, you seem assuming that struct tcindex_filter_result is always from struct tcindex_data, which is not true, check the following tcindex_lookup() which retrieves tcindex_filter_result from struct tcindex_filter.
static struct tcindex_filter_result *tcindex_lookup(struct tcindex_data *p, u16 key) { if (p->perfect) { struct tcindex_filter_result *f = p->perfect + key;
return tcindex_filter_is_set(f) ? f : NULL; } else if (p->h) { struct tcindex_filter __rcu **fp; struct tcindex_filter *f;
fp = &p->h[key % p->hash]; for (f = rcu_dereference_bh_rtnl(*fp); f; fp = &f->next, f = rcu_dereference_bh_rtnl(*fp)) if (f->key == key) return &f->result; }
return NULL; }
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c > index 1c9eeb98d826..3f4e7a6cdd96 100644 > --- a/net/sched/cls_tcindex.c > +++ b/net/sched/cls_tcindex.c > @@ -478,14 +478,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > tcf_bind_filter(tp, &cr, base); > } > > - if (old_r && old_r != r) { > - err = tcindex_filter_result_init(old_r, cp, net); > - if (err < 0) { > - kfree(f); > - goto errout_alloc; > - } > - } > -
Even if your above analysis is correct, 'old_r' becomes unused (set but not used) now, I think you should get some compiler warning.
Thanks.
|  |