netdev - Re: [Patch net v2 2/2] net_sched: fix all the madness of tc filter chain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170910143300.GB1860@nanopsycho>
Date:   Sun, 10 Sep 2017 16:33:00 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [Patch net v2 2/2] net_sched: fix all the madness of tc filter
 chain

Fri, Sep 08, 2017 at 06:37:55PM CEST, xiyou.wangcong@...il.com wrote:
>On Thu, Sep 7, 2017 at 1:52 PM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Thu, Sep 07, 2017 at 07:45:49PM CEST, xiyou.wangcong@...il.com wrote:
>>>Yes it is for chain 0, because block holds a reference to chain 0 during
>>>creation. Non-0 chains are created with refcnt==1 too but paired with
>>>tcf_chain_put() rather than tcf_block_put(). This is what makes chain 0
>>>not special w.r.t. refcnt.
>>
>> So you need to tcf_chain_put only chain 0 here, right? The rest of the
>> chains get destroyed by the previous list_for_each_entry iteration when
>> flush happens and actions destroy happens what decrements recnt to 0
>> there.
>
>
>This is correct. And it should be only chain 0 after flush.
>
>>
>> What do I miss, who would still hold reference for non-0 chains when all
>> tps and all goto_chain actions are gone?
>
>No one. This is totally correct and is exactly what this patch intends to do.
>
>Look, this is why we never need an object with refcnt==0 to exist. ;)

So, I understand that correctly, good. But this is a problem.

When you do:
       list_for_each_entry(chain, &block->chain_list, list)
                tcf_chain_flush(chain);

The reference may get dropped for chains to 0 (for those that does not
have a goto_chain action holding a ref), and therefore they get freed
within the loop. That is problematic when you do the traversing of the
list. You may use list_for_each_entry_safe, but there is another issue:

As a part of tcf_chain_flush destruction, act goto_chain destruction may
get scheduled by call_rcu. That may be the last reference held for the
chain. So you race between this loop and rcu callback.

Consider following example:

chain0  - has only one rule with goto_chain 22 action
chain22 - no rule (refcnt 1 because of the action mentioned above)

         CPU0                            CPU1

    tcf_chain_flush(0)
               -> call_rcu(free_tcf)
                                          free_tcf
                                             ->tcf_chain_put(22)
                                                 ->tcf_chain_destroy(22)
                                                     ->kfree(22)	
    tcf_chain_flush(22)...use-after-free


So what I suggest in order to prevent this is to change your code to
something like:

	/* To avoid race between getting reference in the next loop and
	 * rcu callbacks from deleleted actions freeing the chain.
	 */
	rcu_barrier();

	list_for_each_entry(chain, &block->chain_list, list)
		if (chain->index) /* we already hold ref to chain 0 */
			tcf_chain_hold(chain);

	list_for_each_entry(chain, &block->chain_list, list)
		tcf_chain_flush(chain);
	
	/* Wait for rcu callbacks from deleleted actions that were
	 * sheduled as a result of tcf_chain_flush in the previous loop.
	 * This is not absolutelly necessary, as the chain may live after
	 * the tcf_chain_put is called in the next iteration and would
	 * get freed on tcf_chain_put call from rcu callback later on.
	 */
	rcu_barrier();

	/* Now we are sure that we are the only one holding a reference
	 * to all chains, drop it and let them go.
	 */
	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
		tcf_chain_put(chain);
	kfree(block);

Does this make sense?

Thanks!

Jiri