Skip to content

Commit

Permalink
openvswitch.h: Align uAPI definition with the kernel.
Browse files Browse the repository at this point in the history
Upstream commit:
    commit 1926407a4ab0e59d5a27bed7b82029b356d80fa0
    Author: Ilya Maximets <[email protected]>
    Date:   Wed Mar 9 23:20:33 2022 +0100

    net: openvswitch: fix uAPI incompatibility with existing user space

    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 << type) converted to (1ULL << type) to avoid integer overflow on
    OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now.

     [1] beb75a4 ("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: openvswitch@beb75a4
    Reported-by: Roi Dayan <[email protected]>
    Signed-off-by: Ilya Maximets <[email protected]>
    Acked-by: Nicolas Dichtel <[email protected]>
    Acked-by: Aaron Conole <[email protected]>
    Link: https://lore.kernel.org/r/[email protected]
    Signed-off-by: Jakub Kicinski <[email protected]>

Not adding OVS_KEY_ATTR_IPV6_EXTHDRS in this commit as this is not
necessary.  Will be added along with the actual userspace
implementation.

This change should help avoiding incompatibility issues in the future.

Acked-by: Aaron Conole <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
igsilya committed Mar 25, 2022
1 parent 9d86459 commit d96d14b
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 10 deletions.
9 changes: 8 additions & 1 deletion datapath/flow_netlink.c
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,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 != 29);
BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 31);

return nla_total_size(4) /* OVS_KEY_ATTR_PRIORITY */
+ nla_total_size(0) /* OVS_KEY_ATTR_TUNNEL */
Expand Down Expand Up @@ -491,6 +491,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 & (1ULL << type)) {
OVS_NLERR(log, "Duplicate key (type %d).", type);
return -EINVAL;
Expand Down
20 changes: 11 additions & 9 deletions datapath/linux/compat/include/linux/openvswitch.h
Original file line number Diff line number Diff line change
Expand Up @@ -388,17 +388,19 @@ enum ovs_key_attr {
OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */
OVS_KEY_ATTR_NSH, /* Nested set of ovs_nsh_key_* */

#ifdef __KERNEL__
/* Only used within kernel data path. */
OVS_KEY_ATTR_TUNNEL_INFO, /* struct ovs_tunnel_info */
#endif

#ifndef __KERNEL__
/* Only used within userspace data path. */
OVS_KEY_ATTR_PACKET_TYPE, /* be32 packet type */
/* 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, /* struct ovs_key_nd_extensions */
#endif

OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info.
* For in-kernel use only.
*/
__OVS_KEY_ATTR_MAX
};

Expand Down
2 changes: 2 additions & 0 deletions lib/odp-execute.c
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@ odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a)
case OVS_KEY_ATTR_CT_ZONE:
case OVS_KEY_ATTR_CT_MARK:
case OVS_KEY_ATTR_CT_LABELS:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
OVS_NOT_REACHED();
Expand Down Expand Up @@ -665,6 +666,7 @@ odp_execute_masked_set_action(struct dp_packet *packet,
case OVS_KEY_ATTR_ICMP:
case OVS_KEY_ATTR_ICMPV6:
case OVS_KEY_ATTR_TCP_FLAGS:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
OVS_NOT_REACHED();
Expand Down
4 changes: 4 additions & 0 deletions lib/odp-util.c
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ ovs_key_attr_to_string(enum ovs_key_attr attr, char *namebuf, size_t bufsize)
case OVS_KEY_ATTR_PACKET_TYPE: return "packet_type";
case OVS_KEY_ATTR_NSH: return "nsh";

case OVS_KEY_ATTR_TUNNEL_INFO: return "<error: kernel-only tunnel_info>";
case __OVS_KEY_ATTR_MAX:
default:
snprintf(namebuf, bufsize, "key%u", (unsigned int) attr);
Expand Down Expand Up @@ -3261,6 +3262,7 @@ odp_mask_is_constant__(enum ovs_key_attr attr, const void *mask, size_t size,
switch (attr) {
case OVS_KEY_ATTR_UNSPEC:
case OVS_KEY_ATTR_ENCAP:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
return false;
Expand Down Expand Up @@ -4412,6 +4414,7 @@ format_odp_key_attr__(const struct nlattr *a, const struct nlattr *ma,
break;
}
case OVS_KEY_ATTR_UNSPEC:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
format_generic_odp_key(a, ds);
Expand Down Expand Up @@ -6616,6 +6619,7 @@ odp_key_to_dp_packet(const struct nlattr *key, size_t key_len,
case OVS_KEY_ATTR_MPLS:
case OVS_KEY_ATTR_PACKET_TYPE:
case OVS_KEY_ATTR_NSH:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
break;
Expand Down
1 change: 1 addition & 0 deletions ofproto/ofproto-dpif-sflow.c
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,7 @@ sflow_read_set_action(const struct nlattr *attr,
case OVS_KEY_ATTR_UNSPEC:
case OVS_KEY_ATTR_PACKET_TYPE:
case OVS_KEY_ATTR_NSH:
case OVS_KEY_ATTR_TUNNEL_INFO:
case __OVS_KEY_ATTR_MAX:
default:
break;
Expand Down

0 comments on commit d96d14b

Please sign in to comment.