Messages in this thread | | | From | Hawkins Jiawei <> | Subject | Re: [PATCH] net: sched: fix memory leak in tcindex_set_parms | Date | Sat, 5 Nov 2022 22:11:56 +0800 |
| |
On Fri, 4 Nov 2022 at 10:23, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 4 Nov 2022 00:07:00 +0800 Hawkins Jiawei wrote: > > > 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? > > > > I think we shouldn't put the tcf_exts_destroy(&old_e) > > into this if block, or other RCU readers may derefer the > > freed memory (Please correct me If I am wrong). > > > > So I put the tcf_exts_destroy(&old_e) near the tcindex > > destroy work, after the RCU updateing. > > I'm not sure what this code is trying to do, to be honest. > Your concern that there may be a concurrent reader is valid, > but then again tcindex_filter_result_init() just wipes the > entire structure with a memset() so concurrent readers are > already likely broken? > > Maybe tcindex_filter_result_init() dates back to times when > exts were a list (see commit 22dc13c837c) and calling > tcf_exts_init() wasn't that different than cleaning it up? > In other words this code is trying to destroy old_r, not > reinitialize it? Yes, I also think this code is just trying to destroy the old_r.
In my opinion, the context here is a bit like, this filter's some properties has been changed, so kernel should drop its old filter result and update a new one.
Before kernel finishes RCU updating, concurrent readers should see an empty result(or a valid old result), cleaned by tcindex_filter_result_init().
This won't trigger the memory leak before commit b9a24bb76bf6 ("net_sched: properly handle failure case of tcf_exts_init()"), I think. Because the new filter result still uses the old_r->exts.
Yet after this commit, kernel allocates the new struct tcf_exts for new filter result in tcindex_alloc_perfect_hash(), which triggers the memory leak if kernel cleans the old_r without destroying its newly allocted struct tcf_exts.
As for the patch, I think we'd better free this struct tcf_exts after RCU updating, to make sure that concurrent readers can only see an empty result or a valid old result, before finishing updating (Please correct me if I am wrong). > > > > > > > > err = tcindex_filter_result_init(old_r, cp, net);
| |