net: sched: crash on blocks with goto chain action
Subsystems (3)
PATCH v2 1/1 net: sched: crash on blocks with goto chain action
tcf_block_put_ext has assumed that all filters (and thus their goto actions) are destroyed in RCU callback and thus can not race with our list iteration. However, that is not true during netns cleanup (see tcf_exts_get_net comment).
Prevent the user after free by holding all chains (except 0, that one is already held). foreach_safe is not enough in this case.
To reproduce, run the following in a netns and then delete the ns:
ip link add dtest type dummy
tc qdisc add dev dtest ingress
tc filter add dev dtest chain 1 parent ffff: handle 1 prio 1 flower action goto chain 2
---
v1 -> v2: Hold all chains instead of just the currently iterated one,
the code should be more clear this way.
--- net/sched/cls_api.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
net/sched/cls_api.c +12 -5
@@ -336,7 +336,8 @@ static void tcf_block_put_final(struct work_struct *work)struct tcf_chain *chain, *tmp;rtnl_lock (); -/* Only chain 0 should be still here. */ + +/* At this point, all the chains should have refcnt == 1. */ list_for_each_entry_safe (chain, tmp, &block->chain_list, list)tcf_chain_put (chain);rtnl_unlock ();
@@ -344,15 +345,21 @@ static void tcf_block_put_final(struct work_struct *work) }/* XXX: Standalone actions are not allowed to jump to any chain, and bound -* actions should be all removed after flushing. However, filters are now -* destroyed in tc filter workqueue with RTNL lock, they can not race here. +* actions should be all removed after flushing. */ void tcf_block_put_ext (struct tcf_block *block,struct Qdisc *q,struct tcf_block_ext_info *ei) { -struct tcf_chain *chain, *tmp; +struct tcf_chain *chain; -list_for_each_entry_safe (chain, tmp, &block->chain_list, list) +/* Hold a refcnt for all chains, except 0, so that they don't disappear +* while we are iterating. +*/ +list_for_each_entry (chain, &block->chain_list, list) +if (chain->index) +tcf_chain_hold (chain); + +list_for_each_entry (chain, &block->chain_list, list)tcf_chain_flush (chain);tcf_block_offload_unbind (block, q, ei);
Signature · 1 line
2.15.0
Quoted 43 lines · Fri, Nov 24, 2017 at 12:27:58PM CET, [email protected] wrote:
tcf_block_put_exthas assumed that all filters (and thus their goto actions) are destroyed in RCU callback and thus can not race with our list iteration. However, that is not true during netns cleanup (seetcf_exts_get_netcomment).Prevent the user after free by holding all chains (except 0, that one is already held).
foreach_safeis not enough in this case.To reproduce, run the following in a netns and then delete the ns:
ip link add dtest type dummy
tc qdisc add dev dtest ingress
tc filter add dev dtest chain 1 parent ffff: handle 1 prio 1 flower action goto chain 2Fixes 822e86d997 ("net_sched: remove tcf_block_put_deferred()")Signed-off-by Roman Kapl <[email protected]>---
v1 -> v2: Hold all chains instead of just the currently iterated one,
the code should be more clear this way.
---net/sched/cls_api.c| 17 ++++++++++++-----1 file changed, 12 insertions(+), 5 deletions(-)net/sched/cls_api.c +3
-3@@ -336,7 +336,8 @@ static void tcf_block_put_final(struct work_struct *work)struct tcf_chain *chain, *tmp;rtnl_lock (); -/* Only chain 0 should be still here. */ + +/* At this point, all the chains should have refcnt == 1. */ list_for_each_entry_safe (chain, tmp, &block->chain_list, list)tcf_chain_put (chain);rtnl_unlock ();@@ -344,15 +345,21 @@ static void tcf_block_put_final(struct work_struct *work) }/* XXX: Standalone actions are not allowed to jump to any chain, and bound -* actions should be all removed after flushing. However, filters are now -* destroyed in tc filter workqueue with RTNL lock, they can not race here. +* actions should be all removed after flushing.
net/sched/cls_api.c static void tcf_block_put_final(struct work_struct *work) · line 356
Quoted 10 lines
net/sched/cls_api.c +3
-2*/ void tcf_block_put_ext (struct tcf_block *block,struct Qdisc *q,struct tcf_block_ext_info *ei) { -struct tcf_chain *chain, *tmp; +struct tcf_chain *chain; -list_for_each_entry_safe (chain, tmp, &block->chain_list, list) +/* Hold a refcnt for all chains, except 0, so that they don't disappear +* while we are iterating.
Would be perhaps nice to mention that the appropriate tcf_chain_put is done in tcf_block_put_final()
Regardless of this:
Quoted 12 lines
net/sched/cls_api.c +6
-0+*/ +list_for_each_entry (chain, &block->chain_list, list) +if (chain->index) +tcf_chain_hold (chain); + +list_for_each_entry (chain, &block->chain_list, list)tcf_chain_flush (chain);tcf_block_offload_unbind (block, q, ei);Signature · 1 line
2.15.0
Quoted 2 lines · Roman Kapl
Fixes 822e86d997 ("net_sched: remove tcf_block_put_deferred()")
You blame a wrong commit here.
Commit 822e86d997 was correct at that time, it is the patchset which includes commit e4b95c41df36befcfd11721 makes it buggy again.