On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote: > On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcara...@redhat.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
