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?
