Re: [PATCH v3] net: sched: fix memory leak in tcindex_set_parms

[Cong Wang]

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.