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
> got slightly confused with the race you described, but in principle
> there is nothing stopping new actions from implementing the user space
> correctly, right?
FWIW, that is my thinking too.
