Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 10 Dec 2022 13:29:22 -0800
From:   Cong Wang <xiyou.wangcong@gmail.com>
To:     Hawkins Jiawei <yin31149@gmail.com>
Cc:     18801353760@163.com, cong.wang@bytedance.com, davem@davemloft.net,
        dvyukov@google.com, edumazet@google.com, jhs@mojatatu.com,
        jiri@resnulli.us, kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, pabeni@redhat.com,
        syzbot+232ebdbd36706c965ebf@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH v3] net: sched: fix memory leak in tcindex_set_parms
Message-ID: <Y5T6Mrb7cs6o/BqS@pop-os.localdomain>
References: <Y4uvQA2xxtJXltSM@pop-os.localdomain>
 <20221205151956.28422-1-yin31149@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221205151956.28422-1-yin31149@gmail.com>
Precedence: bulk

On Mon, Dec 05, 2022 at 11:19:56PM +0800, Hawkins Jiawei wrote:
> To be more specific, the simplified logic about original
> tcindex_set_parms() is as below:
> 
> static int
> tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> 		  u32 handle, struct tcindex_data *p,
> 		  struct tcindex_filter_result *r, struct nlattr **tb,
> 		  struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
> {
> 	...
> 	if (p->perfect) {
> 		int i;
> 
> 		if (tcindex_alloc_perfect_hash(net, cp) < 0)
> 			goto errout;
> 		cp->alloc_hash = cp->hash;
> 		for (i = 0; i < min(cp->hash, p->hash); i++)
> 			cp->perfect[i].res = p->perfect[i].res;
> 		balloc = 1;
> 	}
> 	cp->h = p->h;
> 
> 	...
> 
> 	if (cp->perfect)
> 		r = cp->perfect + handle;

We can reach here if p->perfect is non-NULL.

> 	else
> 		r = tcindex_lookup(cp, handle) ? : &new_filter_result;
> 
> 	if (old_r && old_r != r) {
> 		err = tcindex_filter_result_init(old_r, cp, net);
> 		if (err < 0) {
> 			kfree(f);
> 			goto errout_alloc;
> 		}
> 	}
> 	...
> }
> 
> - cp's h field is directly copied from p's h field
> 
> - if `old_r` is retrieved from struct tcindex_filter, in other word,
> is retrieved from p's h field. Then the `r` should get the same value
> from `tcindex_loopup(cp, handle)`.

See above, 'r' can be 'cp->perfect + handle' which is newly allocated,
hence different from 'old_r'.

> 
> - so `old_r == r` is true, code will never uses tcindex_filter_result_init()
> to clear the old_r in such case.

Not always.

> 
> So I think this patch still can fix this memory leak caused by 
> tcindex_filter_result_init(), But maybe I need to improve my
> commit message.
> 

I think your patch may introduce other memory leaks and 'old_r' may
be left as obsoleted too.

Thanks.