On Tue, Mar 13, 2018 at 7:13 PM, Davide Caratti <dcara...@redhat.com> wrote:
> Similarly to what other TC actions do, we can duplicate 'sdata' before
> calling tcf_idr_create(), and avoid calling tcf_idr_cleanup(), so that
> leaks of 'index' don't occur anymore.
Looks like we just need to replace the tcf_idr_cleanup() with
tcf_idr_release()? Which is also simpler.
>
> Signed-off-by: Davide Caratti <dcara...@redhat.com>
> ---
>
> Notes:
> Hello,
>
> I observed this faulty behavior on act_bpf, in case of negative return
> value of tcf_bpf_init_from_ops() and tcf_bpf_init_from_efd(). Then I
> tried on act_simple, that parses its parameter in a similar way, and
> reproduced the same leakage of 'index'.
>
> So, unless you think we should fix this issue in a different way (i.e.
> changing tcf_idr_cleanup() ), I will post a similar fix for act_bpf.
>
> Any feedback is welcome, thank you in advance!
Looks like all other callers of tcf_idr_cleanup() need to be replaced too,
but I don't audit all of them...
[...]
> if (!exists) {
> + defdata = nla_strdup(tb[TCA_DEF_DATA], GFP_KERNEL);
> + if (unlikely(!defdata))
> + return -ENOMEM;
> +
> ret = tcf_idr_create(tn, parm->index, est, a,
> &act_simp_ops, bind, false);
> if (ret)
> return ret;
You leak memory here on failure, BTW.
