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

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

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?

Thanks,
Marcelo