Message-ID: <ddbde7ffec27783a65a63e89962bf9db7565f0e0.camel@redhat.com>
Date:   Tue, 16 Oct 2018 19:38:41 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net/sched: properly init chain in case of multiple
 control actions

On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote:
> On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcaratti@...hat.com> wrote:
> > 
> > On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote:
> > > Why not just validate the fallback action in each action init()?
> > > For example, checking tcfg_paction in tcf_gact_init().
> > > 
> > > I don't see the need of making it generic.
...
> > A (legal?) trick  is to let tcf_action store the fallback action when it
> > contains a 'goto chain' command, I just posted a proposal for gact. If you
> > think it's ok, I will test and post the same for act_police.
> 
> Do we really need to support TC_ACT_GOTO_CHAIN for
> gact->tcfg_paction etc.? I mean, is it useful in practice or is it just for
> completeness?
> 
> IF we don't need to support it, we can just make it invalid without needing
> to initialize it in ->init() at all.
> 
> If we do, however, we really need to move it into each ->init(), because
> we have to lock each action if we are modifying an existing one. With
> your patch, tcf_action_goto_chain_init() is still called without the per-action
> lock.
> 
> What's more, if we support two different actions in gact, that is, tcfg_paction
> and tcf_action, how could you still only have one a->goto_chain pointer?
> There should be two pointers for each of them. :)

whatever fixes the NULL dereference is OK for me.
I thought that the proposal made with

https://www.mail-archive.com/netdev@vger.kernel.org/msg251933.html

(i.e., letting init() copy tcfg_paction to tcf_action in case it contained
'goto chain x') was smart enough to preserve the current behavior, and
also let 'goto chain' work in case it was configured  *only* for the
fallback action.
When the action is modified, the change to tcfg_paction is done with the
same spinlock as tcf_action, so I didn't notice anything worse than the
current locking layout. 

(well, after some more thinking I looked again at that patch and yes, it
lacked the most important thing:)

--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -88,6 +88,9 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
                p_parm = nla_data(tb[TCA_GACT_PROB]);
                if (p_parm->ptype >= MAX_RAND)
                        return -EINVAL;
+               if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN) &&
+                   TC_ACT_EXT_CMP(parm->action, TC_ACT_GOTO_CHAIN))
+                       return -EINVAL;
        }
 #endif

That said, 'goto chain' never worked for police and gact since the first
introduction of 'goto chain', so we are not breaking any userspace program.
And I don't necessarily need 'goto chain' in police and gact fallback
actions; nobody complained in 1 year, so we can just add these two lines
in tcf_gact_init() and something similar in tcf_police_init():


                if (p_parm->ptype >= MAX_RAND)
                        return -EINVAL;
+               if (TC_ACT_EXT_CMP(p_parm->paction, TC_ACT_GOTO_CHAIN))
+                       return -EINVAL;


(and maybe also help users with a proper extack). Just let me know which
approach you prefer, I will test and send patches.
thanks!

-- 
davide

Powered by blists - more mailing lists