> 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
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?
Cheers,
Kevin D-B
gpg: 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
signature.asc
Description: Message signed with OpenPGP
