Few years ago OVS user space made a strange choice in the commit [1]
to define types only valid for the user space inside the copy of a
kernel uAPI header. '#ifndef __KERNEL__' and another attribute was
added later.
This leads to the inevitable clash between user space and kernel types
when the kernel uAPI is extended. The issue was unveiled with the
addition of a new type for IPv6 extension header in kernel uAPI.
When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the
older user space application, application tries to parse it as
OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as
malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with
every IPv6 packet that goes to the user space, IPv6 support is fully
broken.
Fixing that by bringing these user space attributes to the kernel
uAPI to avoid the clash. Strictly speaking this is not the problem
of the kernel uAPI, but changing it is the only way to avoid breakage
of the older user space applications at this point.
These 2 types are explicitly rejected now since they should not be
passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved
out from the '#ifdef __KERNEL__' as there is no good reason to hide
it from the userspace. And it's also explicitly rejected now, because
it's for in-kernel use only.
Comments with warnings were added to avoid the problem coming back.
[1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
Link: https://lore.kernel.org/netdev/[email protected]
Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
Reported-by: Roi Dayan <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
---
Roi, could you please test this change on your setup?
I didn't run system tests myself yet, setting up environment at the moment.
include/uapi/linux/openvswitch.h | 18 ++++++++++++++----
net/openvswitch/flow_netlink.c | 9 ++++++++-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 9d1710f20505..ce3e1738d427 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -351,11 +351,21 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */
OVS_KEY_ATTR_NSH, /* Nested set of ovs_nsh_key_* */
- OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */
-#ifdef __KERNEL__
- OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */
-#endif
+ /* User space decided to squat on types 29 and 30. They are defined
+ * below, but should not be sent to the kernel.
+ *
+ * WARNING: No new types should be added unless they are defined
+ * for both kernel and user space (no 'ifdef's). It's hard
+ * to keep compatibility otherwise.
+ */
+ OVS_KEY_ATTR_PACKET_TYPE, /* be32 packet type */
+ OVS_KEY_ATTR_ND_EXTENSIONS, /* IPv6 Neighbor Discovery extensions */
+
+ OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info.
+ * For in-kernel use only.
+ */
+ OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */
__OVS_KEY_ATTR_MAX
};
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 8b4124820f7d..c9c49e5cd67f 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -346,7 +346,7 @@ size_t ovs_key_attr_size(void)
/* Whenever adding new OVS_KEY_ FIELDS, we should consider
* updating this function.
*/
- BUILD_BUG_ON(OVS_KEY_ATTR_TUNNEL_INFO != 30);
+ BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */
+ nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */
@@ -482,6 +482,13 @@ static int __parse_flow_nlattrs(const struct nlattr *attr,
return -EINVAL;
}
+ if (type == OVS_KEY_ATTR_PACKET_TYPE ||
+ type == OVS_KEY_ATTR_ND_EXTENSIONS ||
+ type == OVS_KEY_ATTR_TUNNEL_INFO) {
+ OVS_NLERR(log, "Key type %d is not supported", type);
+ return -EINVAL;
+ }
+
if (attrs & (1 << type)) {
OVS_NLERR(log, "Duplicate key (type %d).", type);
return -EINVAL;
--
2.34.1
On 3/9/22 16:56, Ilya Maximets wrote:
> Few years ago OVS user space made a strange choice in the commit [1]
> to define types only valid for the user space inside the copy of a
> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was
> added later.
>
> This leads to the inevitable clash between user space and kernel types
> when the kernel uAPI is extended. The issue was unveiled with the
> addition of a new type for IPv6 extension header in kernel uAPI.
>
> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the
> older user space application, application tries to parse it as
> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as
> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with
> every IPv6 packet that goes to the user space, IPv6 support is fully
> broken.
>
> Fixing that by bringing these user space attributes to the kernel
> uAPI to avoid the clash. Strictly speaking this is not the problem
> of the kernel uAPI, but changing it is the only way to avoid breakage
> of the older user space applications at this point.
>
> These 2 types are explicitly rejected now since they should not be
> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved
> out from the '#ifdef __KERNEL__' as there is no good reason to hide
> it from the userspace. And it's also explicitly rejected now, because
> it's for in-kernel use only.
>
> Comments with warnings were added to avoid the problem coming back.
>
> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
>
> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
> Link: https://lore.kernel.org/netdev/[email protected]
> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
> Reported-by: Roi Dayan <[email protected]>
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
>
> Roi, could you please test this change on your setup?
>
> I didn't run system tests myself yet, setting up environment at the moment.
OK. I set up my own environment and the patch doesn't seem to work.
Investigating.
On 3/9/22 18:22, Ilya Maximets wrote:
> On 3/9/22 16:56, Ilya Maximets wrote:
>> Few years ago OVS user space made a strange choice in the commit [1]
>> to define types only valid for the user space inside the copy of a
>> kernel uAPI header. '#ifndef __KERNEL__' and another attribute was
>> added later.
>>
>> This leads to the inevitable clash between user space and kernel types
>> when the kernel uAPI is extended. The issue was unveiled with the
>> addition of a new type for IPv6 extension header in kernel uAPI.
>>
>> When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the
>> older user space application, application tries to parse it as
>> OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as
>> malformed. Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with
>> every IPv6 packet that goes to the user space, IPv6 support is fully
>> broken.
>>
>> Fixing that by bringing these user space attributes to the kernel
>> uAPI to avoid the clash. Strictly speaking this is not the problem
>> of the kernel uAPI, but changing it is the only way to avoid breakage
>> of the older user space applications at this point.
>>
>> These 2 types are explicitly rejected now since they should not be
>> passed to the kernel. Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved
>> out from the '#ifdef __KERNEL__' as there is no good reason to hide
>> it from the userspace. And it's also explicitly rejected now, because
>> it's for in-kernel use only.
>>
>> Comments with warnings were added to avoid the problem coming back.
>>
>> [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
>>
>> Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension header support")
>> Link: https://lore.kernel.org/netdev/[email protected]
>> Link: https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
>> Reported-by: Roi Dayan <[email protected]>
>> Signed-off-by: Ilya Maximets <[email protected]>
>> ---
>>
>> Roi, could you please test this change on your setup?
>>
>> I didn't run system tests myself yet, setting up environment at the moment.
>
> OK. I set up my own environment and the patch doesn't seem to work.
> Investigating.
I figured that out. The problem is that OVS_KEY_ATTR_IPV6_EXTHDRS == 32.
That causes (1 << type) to overflow during the parsing and enable incorrect
bit in the mask of seen attributes. The following additional change fixes
the problem:
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c9c49e5cd67f..5176f6ccac8e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -489,7 +489,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr,
return -EINVAL;
}
- if (attrs & (1 << type)) {
+ if (attrs & (1ULL << type)) {
OVS_NLERR(log, "Duplicate key (type %d).", type);
return -EINVAL;
}
@@ -502,7 +502,7 @@ static int __parse_flow_nlattrs(const struct nlattr *attr,
}
if (!nz || !is_all_zero(nla_data(nla), nla_len(nla))) {
- attrs |= 1 << type;
+ attrs |= 1ULL << type;
a[type] = nla;
}
}
---
I'll run few more checks to be sure and send v2 with above change applied.
We may also want to have a cosmetic change that converts all (1 << OVS_*)
to (1ULL << OVS_*) even for attributes < 32 as a follow up. Current code
looks a bit inconsistent.
Best regards, Ilya Maximets.