On 4/25/2023 11:51 PM, Ilya Maximets wrote:
External email: Use caution opening links or attachments
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.
ACK. A new API like nl_msg_start_nested_with_flag() is a good idea and
we will
only use it for vxlan gbp now.
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
