if the kernel fails duplicating 'sdata', creation of a new action fails
with -ENOMEM. However, subsequent attempts to install the same action
using the same value of 'index' will systematically fail with -ENOSPC,
and that value of 'index' will no more be usable by act_simple, until
rmmod / insmod of act_simple.ko is done:
# tc actions add action simple sdata hello index 100
# tc actions list action simple
action order 0: Simple <hello>
index 100 ref 1 bind 0
# tc actions flush action simple
# tc actions add action simple sdata hello index 100
RTNETLINK answers: Cannot allocate memory
We have an error talking to the kernel
# tc actions flush action simple
# tc actions add action simple sdata hello index 100
RTNETLINK answers: No space left on device
We have an error talking to the kernel
# tc actions add action simple sdata hello index 100
RTNETLINK answers: No space left on device
We have an error talking to the kernel
...
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.
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!
net/sched/act_simple.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 425eac11f6da..b80d4a69a848 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -53,15 +53,6 @@ static void tcf_simp_release(struct tc_action *a)
kfree(d->tcfd_defdata);
}
-static int alloc_defdata(struct tcf_defact *d, char *defdata)
-{
- d->tcfd_defdata = kzalloc(SIMP_MAX_DATA, GFP_KERNEL);
- if (unlikely(!d->tcfd_defdata))
- return -ENOMEM;
- strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
- return 0;
-}
-
static void reset_policy(struct tcf_defact *d, char *defdata,
struct tc_defact *p)
{
@@ -110,20 +101,18 @@ static int tcf_simp_init(struct net *net, struct nlattr
*nla,
return -EINVAL;
}
- defdata = nla_data(tb[TCA_DEF_DATA]);
-
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;
d = to_defact(*a);
- ret = alloc_defdata(d, defdata);
- if (ret < 0) {
- tcf_idr_cleanup(*a, est);
- return ret;
- }
+ d->tcfd_defdata = defdata;
d->tcf_action = parm->action;
ret = ACT_P_CREATED;
} else {
@@ -133,6 +122,7 @@ static int tcf_simp_init(struct net *net, struct nlattr
*nla,
if (!ovr)
return -EEXIST;
+ defdata = nla_data(tb[TCA_DEF_DATA]);
reset_policy(d, defdata, parm);
}
--
2.14.3
