On 4/25/23 15:21, Roi Dayan wrote: > > > On 25/04/2023 16:14, Roi Dayan wrote: >> >> >> On 25/04/2023 15:53, Ilya Maximets wrote: >>> On 4/25/23 14:31, Roi Dayan wrote: >>>> >>>> >>>> On 25/04/2023 15:29, Ilya Maximets wrote: >>>>> On 4/25/23 08:32, Roi Dayan via dev wrote: >>>>>> From: Gavin Li <[email protected]> >>>>>> >>>>>> Linux kernel netlink module added NLA_F_NESTED flag checking for nested >>>>>> netlink messages in 5.2. A nested message without the flag set will be >>>>>> treated as malformated one. The check is optional and is controlled by >>>>>> message policy. To avoid this, add NLA_F_NESTED explicitly for all >>>>>> nested netlink messages. >>>>>> >>>>>> Signed-off-by: Gavin Li <[email protected]> >>>>>> Reviewed-by: Roi Dayan <[email protected]> >>>>>> --- >>>>>> lib/netlink.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/netlink.c b/lib/netlink.c >>>>>> index 6215282d6fbe..f128b63074f9 100644 >>>>>> --- a/lib/netlink.c >>>>>> +++ b/lib/netlink.c >>>>>> @@ -519,7 +519,7 @@ size_t >>>>>> nl_msg_start_nested(struct ofpbuf *msg, uint16_t type) >>>>>> { >>>>>> size_t offset = msg->size; >>>>>> - nl_msg_put_unspec_uninit(msg, type, 0); >>>>>> + nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0); >>>>> >>>>> >>>>> Are you sure this is safe? >>>>> >>>>> IIRC, the NLA_F_NESTED checking is not enforced on legacy interfaces >>>>> like the OVS family. And the OVS kernel module in not using netlink >>>>> macros in various places for iteration or attribute checking, so it >>>>> may misinterpret the extra bit as an unknown attribute. >>>>> >>>>> Best regards, Ilya Maximets. >>>> >>>> we haven't encountered an issue with it. >>>> I think it just being ignored then. >>> >>> I think, the change will at least cause frequent unnecessary flow >>> revalidation. >>> Assuming we have a clone() action and it is an action with nested arguments. >>> With your change, OVS will add NLA_F_NESTED to the actions, then it will >>> pass these actions to the kernel. Kernel will ignore the flag and work >>> fine. >>> However, on the flow dump, kernel will use >>> nla_nest_start_noflag(skb, OVS_ACTION_ATTR_CLONE); >>> In the clone_action_to_attr(). And it will return that back to the >>> revalidator. >>> Revalidators do not parse actions, they compare them with memcmp to what OVS >>> is constructing in userspace. See ofpbuf_equal() call in >>> revalidate_ukey__(). >>> This one bit difference will cause a flow mod request to the kernel. And >>> that >>> will happen for every flow that contains nested actions on every >>> revalidation >>> cycle. >>> >>> There are potentially more hidden issues. >>> >>> Or am I missing something? >>> >>> Best regards, Ilya Maximets. >> >> hmm I understand. >> Maybe OVS later needs to move to the newer nla_nest_start() like commented >> on nla_nest_start_noflag() ? >> it will require more testing and maybe further changes.
I'm not sure it that is actually possible. We can probbaly ask kernel to use the flag with a special datapath capability flag, but for as long as we want to preserver backward and forward compatibility we'll have to support both ways of operation, which doesn't really sound great. >> ae0be8de9a53 netlink: make nla_nest_start() add NLA_F_NESTED flag >> >> We encountered that for VXLAN OPT we require to pass this flag and at first >> we >> added it only for vxlan opt but then thought maybe to do it correctly >> for all nested which seems is not straightforward because of what you >> describe. >> Also doing it only for vxlan opt will cause the revaldation but only when >> actually such rules will be used. >> >> So It looks like we will hit this anyway currently. This is only for TC, and we re-construct the TC actions in the userspace after the flow dump, because they have a different format. So, we will compare userspace-created actions with actions created from TC actions, again, in userspace. So, there should not be any issues. >> >> Thanks, >> Roi > > I think it started with this patch which according to the commit > message the strict policy checking was requested by Jakub. > d8f9dfae49ce net: sched: allow flower to match vxlan options Yep, all the attributes after GENEVE have to use the flag. GENEVE is not forced to do so. > > Gavin can fix me or update later if I'm missing something. > > I can try to look for the thread to see what was discussed about > this request and can try to suggest removing the strict checking > so it won't be needed to use the nested flag and we can continue > without it like other options. AFAICT, it's just a policy that all the new interfaces has to have a strict checking. This apparently includes new fields of existing interfaces. This policy doesn't really affect OVS module, because the OVS module doesn't use any common netlink macros (because it's legacy and is doing many unconventional things) and implements all the parsing manually. For the problem you're facing, we may introduce a new function in the OVS userspace, call it nl_msg_start_nested_with_flag() or something like that. Then we can use this function for TC interfaces that require the flag. Or for all the TC interfaces, since we're re-parsing and re-packaging everything that goes in/out of TC anyway, so there should not be that many potential incompatibilities. Or the other way around, we could keep your change and add a _noflag() function and use it everywhere in OVS, except for TC. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
