On Mon, Nov 20, 2017 at 1:41 PM, Roman Kapl <c...@rkapl.cz> wrote: > On 11/20/2017 06:54 PM, Cong Wang wrote: >> >> On Sun, Nov 19, 2017 at 8:17 AM, Roman Kapl <c...@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. "
