| Message ID | 20171119161716.5865-1-code@rkapl.cz |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: crash on blocks with goto chain action | expand |
On 11/20/2017 06:54 PM, Cong Wang wrote: > On Sun, Nov 19, 2017 at 8:17 AM, Roman Kapl <code@rkapl.cz> wrote: >> 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 the current list element we are >> iterating over (foreach_safe is not enough). > Hmm... > > Looks like we need to restore the trick we used previously, that is > holding refcnt for all list entries before this list iteration. Was there a reason to hold all list entries in that trick? I thought that holding just the current element will be enough, but maybe not.
On Mon, Nov 20, 2017 at 1:41 PM, Roman Kapl <code@rkapl.cz> wrote: > On 11/20/2017 06:54 PM, Cong Wang wrote: >> >> On Sun, Nov 19, 2017 at 8:17 AM, Roman Kapl <code@rkapl.cz> wrote: >>> >>> 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 the current list element we are >>> iterating over (foreach_safe is not enough). >> >> Hmm... >> >> Looks like we need to restore the trick we used previously, that is >> holding refcnt for all list entries before this list iteration. > > > Was there a reason to hold all list entries in that trick? I thought that > holding just the current element will be enough, but maybe not. > Yes, let me quote Jiri's explanation: " The reason for the hold above was to avoid use after free in this loop. Consider following example: chain1 1 filter with action goto_chain 2 chain2 empty Now in your list_for_each_entry_safe loop, chain1 is flushed, action is removed and chain is put: tcf_action_goto_chain_fini->tcf_chain_put(2) Given the fact chain2 is empty, this put would lead to tcf_chain_destroy(2) Then in another iteration of list_for_each_entry_safe you are using already freed chain. "
On 11/21/2017 08:31 PM, Cong Wang wrote: > On Mon, Nov 20, 2017 at 1:41 PM, Roman Kapl <code@rkapl.cz> wrote: >> On 11/20/2017 06:54 PM, Cong Wang wrote: >>> On Sun, Nov 19, 2017 at 8:17 AM, Roman Kapl <code@rkapl.cz> wrote: >>>> 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 the current list element we are >>>> iterating over (foreach_safe is not enough). >>> Hmm... >>> >>> Looks like we need to restore the trick we used previously, that is >>> holding refcnt for all list entries before this list iteration. >> >> Was there a reason to hold all list entries in that trick? I thought that >> holding just the current element will be enough, but maybe not. >> > Yes, let me quote Jiri's explanation: > > " > The reason for the hold above was to avoid use after free in this loop. > Consider following example: > > chain1 > 1 filter with action goto_chain 2 > chain2 > empty I believe the exact same example is part of the 'how to reproduce' part of commit and the patch helped me get rid of that crash. > > Now in your list_for_each_entry_safe loop, Note that list_for_each_entry_safe was replaced by pure list_for_each_entry in my proposed patch. > chain1 is flushed, action is > removed and chain is put: > tcf_action_goto_chain_fini->tcf_chain_put(2) > > Given the fact chain2 is empty, this put would lead to tcf_chain_destroy(2) > > Then in another iteration of list_for_each_entry_safe you are using > already freed chain. > " No, I believe that the last iteration would simply stop, because at the point you reach second iteration, chain->next == head. But maybe the "hold all chains" approach from 822e86d997 (net_sched: remove tcf_block_put_deferred()) is simpler to understand?
On Tue, Nov 21, 2017 at 12:02 PM, Roman Kapl <code@rkapl.cz> wrote: > > But maybe the "hold all chains" approach from 822e86d997 (net_sched: remove > tcf_block_put_deferred()) is simpler to understand? > Yes, it is much easier to understand for me, probably for others too.
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 7d97f612c9b9..58fed2ea3379 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -344,16 +344,26 @@ 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, *last = NULL; - list_for_each_entry_safe(chain, tmp, &block->chain_list, list) + list_for_each_entry(chain, &block->chain_list, list) { + if (last) + tcf_chain_put(last); + /* Flushing a chain may release any other chain in this block, + * so we have to hold on to the chain across flush to known + * which one comes next. + */ + tcf_chain_hold(chain); tcf_chain_flush(chain); + last = chain; + } + if (last) + tcf_chain_put(last); tcf_block_offload_unbind(block, q, ei);
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 the current list element we are iterating over (foreach_safe is not enough). 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 Signed-off-by: Roman Kapl <code@rkapl.cz> --- The mail was original rejected by vger, this is a re-send to netdev@vger only (with the same message ID). Sorry for any confusion. --- net/sched/cls_api.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)