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

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

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.horman@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