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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Kevin 'ldir' Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk> writes:

>> 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
> and because act_ctinfo hasn’t yet actually been released anything could happen
> to it, reverted if need be.  I’d like it to be done right, not that I know
> what right is, the perils of inexperience and copy/pasting existing boilerplate
> code.
>
> Looking at other code I think I should have done something like:
>
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index e78b60e47c0f..4695aa76c0dc 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -168,7 +168,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>         if (!nla)
>                 return -EINVAL;
>
> -       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
> +       err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, extack);
>         if (err < 0)
>                 return err;
>
> @@ -182,13 +182,19 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>                 dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
>                 /* need contiguous 6 bit mask */
>                 dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
> -               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
> +               if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
> +                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_DSCP_MASK],
> +                                       "dscp mask must be 6 contiguous bits");
>                         return -EINVAL;
> +               }
>                 dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
>                         nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
>                 /* mask & statemask must not overlap */
> -               if (dscpmask & dscpstatemask)
> +               if (dscpmask & dscpstatemask) {
> +                       NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CTINFO_PARMS_STATEMASK],
> +                                       "dscp statemask must not overlap dscp mask");
>                         return -EINVAL;
> +               }
>         }
>
>         /* done the validation:now to the actual action allocation */
>
> Warning: Not even compile tested!  Am I heading in the right
> direction?

Yup! Sending a follow-up patch with an update like this would be an
excellent way to fix the issue :)

-Toke