Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
> On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> > On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> > ...
> > > +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> > > +			   struct nlattr *est, struct tc_action **a,
> > > +			   int ovr, int bind, bool rtnl_held,
> > > +			   struct tcf_proto *tp,
> > > +			   struct netlink_ext_ack *extack)
> > > +{
> > > +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> > > +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> > > +	struct tcf_ctinfo_params *cp_new;
> > > +	struct tcf_chain *goto_ch = NULL;
> > > +	u32 dscpmask = 0, dscpstatemask;
> > > +	struct tc_ctinfo *actparm;
> > > +	struct tcf_ctinfo *ci;
> > > +	u8 dscpmaskshift;
> > > +	int ret = 0, err;
> > > +
> > > +	if (!nla)
> > > +		return -EINVAL;
> > > +
> > > +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);  
> >                                                                        ^^^^
> > Hi, two things here:
> > Why not use the extack parameter here? Took me a while to notice
> > that the EINVAL was actually hiding the issue below.
> > And also on the other two EINVALs this function returns.
> > 
> > 
> > Seems there was a race when this code went in and the stricter check
> > added by
> > b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> > 8cb081746c03 ("netlink: make validation more configurable for future
> > strictness").
> > 
> > I can't add these actions with current net-next and iproute-next:
> > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > Error: NLA_F_NESTED is missing.
> > We have an error talking to the kernel
> > 
> > This also happens with the current post of act_ct and should also
> > happen with the act_mpls post (thus why Cc'ing John as well).
> > 
> > I'm not sure how we should fix this. In theory the kernel can't get
> > stricter with userspace here, as that breaks user applications as
> > above, so older actions can't use the more stricter parser. Should we
> > have some actions behaving one way, and newer ones in a different way?
> > That seems bad.
> > 
> > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > much sense here. WDYT?
> 
> Surely for new actions we can require strict validation, there is
> no existing user space to speak of..  Perhaps act_ctinfo and act_ct

Other than the inconsistency amongst the actions, agreed.

> got slightly confused with the race you described, but in principle
> there is nothing stopping new actions from implementing the user space
> correctly, right?

AFAICT we need to patch iproute2 outside of the action code to cope
with it. Something like:

--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -214,7 +214,7 @@ done0:
                         tail = addattr_nest(n, MAX_MSG, ++prio);
                         addattr_l(n, MAX_MSG, TCA_ACT_KIND, k, strlen(k) + 1);

-                       ret = a->parse_aopt(a, &argc, &argv, TCA_ACT_OPTIONS,
+                       ret = a->parse_aopt(a, &argc, &argv, TCA_ACT_OPTIONS | NLA_F_NESTED,

This wouldn't break the older actions, yes, but then again, to expect
a different parsing behavior from different actions.. seems weird. :)

  Marcelo