netdev - Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly validate the control action

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 19 Feb 2019 17:50:46 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly
 validate the control action

hi Vlad,

On Mon, 2019-02-18 at 16:52 +0000, Vlad Buslov wrote:
> Hi Davide,
> 
> I really like the idea of putting all action validation code in single
> place, instead of spreading it between act API and specific actions init
> code.

sure, the previous validation code was in tcf_action_add_1(), and I 
attempted to fix the problems (well, it's the same problem showing up in 
3 different ways) without adding the same code everywhere.

Sadly, we need to handle correctly the situations where  
tcf_action_check_ctrlact() returns an error, and I'm seeing that we
can't fix every action with the same exact code everywhere (to avoid
leaking IDRs, or memory, or both).

> See my comment regarding minor problem with using
> tcf_action_set_ctrlact() on current net-next below.

> > +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> > +			    struct tcf_chain *goto_chain)
> > +{
> > +	struct tcf_chain *old;
> > +
> > +	old = xchg(&p->goto_chain, goto_chain);
> > +	if (old)
> > +		tcf_chain_put_by_act(old);
> 
> This is blocking in current net-next because tcf_chain_put_by_act()
> obtains block->lock mutex, so you can't call it while holding tcf_lock
> spinlock like you do in following patches. Its not a big problem but
> I guess you will have to just inline this code into all init handlers
> instead of tcf_action_set_ctrlact() function.

Argh! you are right. Thanks a lot for spotting this.

Ok, let's kill tcf_action_set_ctrlact() and swap 'newchain' with
a->goto_chain in each action init(), with a->tcfa_lock taken. I will
then call tcf_chain_put_by_act() in the init() handler, after the
spinlock is released.

> BTW is atomic xchg strictly necessary if you hold tcf_lock while
> re-assigning goto_chain pointer?

No, it's not necessary. The new value of goto_chain is used only when the
new value of tcfa_action is updated: so, if we just do the assignment of
goto_chain and tcfa_action together with the spinlock taken, at least we
are improving the 
current code.

Thanks!
-- 
davide