On Thu, Jun 13, 2019 at 09:09:47AM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
>
>
> > On 13 Jun 2019, at 10:33, Simon Horman <simon.hor...@netronome.com> wrote:
> >
> > 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.
>
>
> Hi everyone,
>
> Apologies that somehow I seem to have caused a bit of trouble. If need be
No need to be. :-)
Marcelo
