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