From: Davide Caratti <dcaratti@redhat.com>
To: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
Jamal Hadi Salim <jhs@mojatatu.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jiri Pirko <jiri@resnulli.us>
Cc: Vlad Buslov <vladbu@mellanox.com>, Roman Mashak <mrv@mojatatu.com>
Subject: [PATCH net 0/2] net/sched: cls_u32: fix refcount leak
Date: Wed, 18 Dec 2019 00:00:03 +0100 [thread overview]
Message-ID: <cover.1576623250.git.dcaratti@redhat.com> (raw)
a refcount leak in the error path of u32_change() has been recently
introduced. It can be observed with the following commands:
[root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 97 \
> u32 match ip src 127.0.0.1/32 indev notexist20 flowid 1:1 action drop
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
[root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 98 \
> handle 42:42 u32 divisor 256
Error: cls_u32: Divisor can only be used on a hash table.
We have an error talking to the kernel
[root@f31 ~]# tc filter replace dev eth0 ingress protocol ip prio 99 \
> u32 ht 47:47
Error: cls_u32: Specified hash table not found.
We have an error talking to the kernel
they all legitimately return -EINVAL; however, they leave semi-configured
filters at eth0 tc ingress:
[root@f31 ~]# tc filter show dev eth0 ingress
filter protocol ip pref 97 u32 chain 0
filter protocol ip pref 97 u32 chain 0 fh 800: ht divisor 1
filter protocol ip pref 98 u32 chain 0
filter protocol ip pref 98 u32 chain 0 fh 801: ht divisor 1
filter protocol ip pref 99 u32 chain 0
filter protocol ip pref 99 u32 chain 0 fh 802: ht divisor 1
With older kernels, filters were unconditionally considered empty (and
thus de-refcounted) on the error path of ->change().
After commit 8b64678e0af8 ("net: sched: refactor tp insert/delete for
concurrent execution"), filters were considered empty when the walk()
function didn't set 'walker.stop' to 1.
Finally, with commit 6676d5e416ee ("net: sched: set dedicated tcf_walker
flag when tp is empty"), tc filters are considered empty unless the walker
function is called with a non-NULL handle. This last change doesn't fit
cls_u32 design, because at least the "root hnode" is (almost) always
non-NULL, as it's allocated in u32_init().
- patch 1/2 is a proposal to restore the original kernel behavior, where
no filter was installed in the error path of u32_change().
- patch 2/2 adds tdc selftests that can be ued to verify the correct
behavior of u32 in the error path of ->change().
Davide Caratti (2):
net/sched: cls_u32: fix refcount leak in the error path of
u32_change()
tc-testing: initial tdc selftests for cls_u32
net/sched/cls_u32.c | 25 +++
.../tc-testing/tc-tests/filters/tests.json | 22 --
.../tc-testing/tc-tests/filters/u32.json | 205 ++++++++++++++++++
3 files changed, 230 insertions(+), 22 deletions(-)
create mode 100644 tools/testing/selftests/tc-testing/tc-tests/filters/u32.json
--
2.23.0
next reply other threads:[~2019-12-17 23:00 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-17 23:00 Davide Caratti [this message]
2019-12-17 23:00 ` [PATCH net 1/2] net/sched: cls_u32: fix refcount leak in the error path of u32_change() Davide Caratti
2019-12-18 14:23 ` Jamal Hadi Salim
2019-12-19 13:32 ` Jamal Hadi Salim
2019-12-19 16:15 ` Vlad Buslov
2019-12-19 16:33 ` Jamal Hadi Salim
2019-12-19 16:51 ` Vlad Buslov
2019-12-19 17:01 ` Vlad Buslov
2019-12-20 12:11 ` Jamal Hadi Salim
2019-12-20 12:25 ` Jamal Hadi Salim
2019-12-20 13:29 ` Jamal Hadi Salim
2019-12-20 14:04 ` Vlad Buslov
2019-12-20 14:57 ` Jamal Hadi Salim
2019-12-20 15:20 ` Davide Caratti
2019-12-20 15:23 ` Jamal Hadi Salim
2019-12-20 13:21 ` Davide Caratti
2019-12-20 13:54 ` Jamal Hadi Salim
2019-12-17 23:00 ` [PATCH net 2/2] tc-testing: initial tdc selftests for cls_u32 Davide Caratti
2019-12-20 1:53 ` [PATCH net 0/2] net/sched: cls_u32: fix refcount leak David Miller
2019-12-20 12:14 ` Jamal Hadi Salim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1576623250.git.dcaratti@redhat.com \
--to=dcaratti@redhat.com \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=mrv@mojatatu.com \
--cc=netdev@vger.kernel.org \
--cc=vladbu@mellanox.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.