Re: [PATCH] net: sched: crash on blocks with goto chain action

On 11/21/2017 08:31 PM, Cong Wang wrote:

On Mon, Nov 20, 2017 at 1:41 PM, Roman Kapl [off-list ref] wrote:

quoted

On 11/20/2017 06:54 PM, Cong Wang wrote:

quoted

On Sun, Nov 19, 2017 at 8:17 AM, Roman Kapl [off-list ref] wrote:

quoted

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?