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

[Hawkins Jiawei <yin31149@gmail.com>]

On Sun, 11 Dec 2022 at 05:29, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> 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'.

But if `r` is `cp->perfect + handle`, this means `cp->perfect` is not
NULL. So `p->perfect` should not be NULL, which means `old_r` should be
`p->perfect + handle`, according to tcindex_lookup(). This is not
correct with the assumption that `old_r` is retrieved from p's h field.

>
> >
> > - 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.

I still think this patch should not introduce any memory leaks.

* If the `old_r` is not NULL, it should have only two source according
to the tcindex_lookup() - `old_r` is retrieved from `p->perfect`; or
`old_r` is retrieved from `p->h`. And if `old_r` is retrieved from `p->h`,
this means `p->perfect` is NULL.


* If the `old_r` is retrieved from `p->perfect`, kernel uses
tcindex_alloc_perfect_hash() to newly allocate the filter results.
And `r` should be `cp->perfect + handle`, which is newly allocated.

So `r != old_r` in this situation, but kernel will clears the `old_r`
at tc_filter_wq workqueue in tcindex_partial_destroy_work(), by
destroying the p->perfect. So here kernel doesn't need
tcindex_filter_result_init() to clear the old filter result, and
there is no memory leak.


* If the `old_r` is retrieved from `p->h`, then `p->perfect` is NULL
discussed above. Considering that `cp->h` is directly copied from
`p->h`, `r` should get the same value as `old_r` from tcindex_lookup().

So `r == old_r`, it will ignore the part that kernel uses
tcindex_filter_result_init() to clear the old filter result. So removing
this part of code should have no effect in this situation.



It seems that whether `old_r` is retrived from `p->h` or `p->perfect`,
it is okay to directly deleting the part that kernel uses
tcindex_filter_result_init() to clear the old filter result, without any
memory leak. But this can fix the memory leak caused by
tcindex_filter_result_init().

As for `old_r` may be left as obsoleted, do you mean `old_r` becomes
unused(set but not used)? I think we can directly removing `old_r`.

>
> Thanks.