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

[Davide Caratti <dcaratti@redhat.com>]

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