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.
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.
Thanks,
Roi
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
