| Message ID | 1515549082-4141-3-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: erspan: add support for openvswitch | expand |
On Tue, 9 Jan 2018 17:51:22 -0800, William Tu wrote: > --- a/include/uapi/linux/openvswitch.h > +++ b/include/uapi/linux/openvswitch.h > @@ -363,7 +373,8 @@ enum ovs_tunnel_key_attr { > OVS_TUNNEL_KEY_ATTR_IPV6_SRC, /* struct in6_addr src IPv6 address. */ > OVS_TUNNEL_KEY_ATTR_IPV6_DST, /* struct in6_addr dst IPv6 address. */ > OVS_TUNNEL_KEY_ATTR_PAD, > - OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, /* be32 ERSPAN index. */ > + OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1, /* be32 ERSPAN v1 index (deprecated). */ You can't do this in an uapi header, the existing name must stay the same forever. Otherwise existing programs would suddenly stop to compile. Jiri
On Tue, 9 Jan 2018 17:51:22 -0800, William Tu wrote: > - [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = sizeof(u32) }, > + [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1] = { .len = sizeof(u32) }, > + [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = OVS_ATTR_NESTED, > + .next = ovs_erspan_opt_lens }, Ouch. It's actually much worse than that, you're redefining the meaning of the field. That's complete no-go. > + case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1: > + OVS_NLERR(log, "ERSPAN attribute %d is deprecated.", > + type); > + return -EINVAL; As is this. > @@ -906,8 +1017,8 @@ static int __ip_tun_to_nlattr(struct sk_buff *skb, > vxlan_opt_to_nlattr(skb, tun_opts, swkey_tun_opts_len)) > return -EMSGSIZE; > else if (output->tun_flags & TUNNEL_ERSPAN_OPT && > - nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, > - ((struct erspan_metadata *)tun_opts)->u.index)) > + erspan_opt_to_nlattr(skb, tun_opts, > + swkey_tun_opts_len)) And this. The existing field must continue to work in the same way as before. It must be accepted and *returned* by the kernel. You may add an additional field but the existing behavior must be 100% preserved, both uABI and uAPI wise. Jiri
On Wed, 10 Jan 2018 22:35:14 +0100, Jiri Benc wrote: > The existing field must continue to work in the same way as before. It must > be accepted and *returned* by the kernel. You may add an additional field > but the existing behavior must be 100% preserved, both uABI and uAPI wise. Another way around this is reverting ceaa001a170e in net.git and designing the uAPI properly in net-next. I think that should be the preferred way, as ceaa001a170e is clearly wrong since you need to redo it after 3 months. Not sure when Linus intends to release 4.15 and how much time you have for this, though. Jiri
Hi Jiri, Thanks a lot for the comments. On Wed, Jan 10, 2018 at 2:02 PM, Jiri Benc <jbenc@redhat.com> wrote: > On Wed, 10 Jan 2018 22:35:14 +0100, Jiri Benc wrote: >> The existing field must continue to work in the same way as before. It must >> be accepted and *returned* by the kernel. You may add an additional field >> but the existing behavior must be 100% preserved, both uABI and uAPI wise. > > Another way around this is reverting ceaa001a170e in net.git and > designing the uAPI properly in net-next. I think that should be the > preferred way, as ceaa001a170e is clearly wrong since you need to redo > it after 3 months. The ceaa001a170e is designed for configuring the ERSPAN v1's fields only, not thinking about the future needs for more fields in ERSPAN v2. This patch tries to use the nested attr to handle both v1 and v2. > > Not sure when Linus intends to release 4.15 and how much time you have > for this, though. > > Jiri I'd also prefer reverting ceaa001a170e since it's more clean but I also hope to have this feature in 4.15. How long does reverting take? Am I only able to submit the new patch after the reverting is merged? Or I can submit revert and this new patch in one series? I have little experience in reverting, can you suggest which way is better? Thanks William
On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote: > I'd also prefer reverting ceaa001a170e since it's more clean but I > also hope to have this feature in 4.15. > How long does reverting take? Am I only able to submit the new patch > after the reverting is merged? Or I can submit revert and this new > patch in one series? I have little experience in reverting, can you > suggest which way is better? Send the revert for net (subject will be "[PATCH net] revert: openvswitch: Add erspan tunnel support."). Don't forget to explain why you're proposing a revert. After it is accepted and applied to net.git, wait until the patch appears in net-next.git. It may take a little while. After that, send the new patch(es) for net-next. Jiri
On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc <jbenc@redhat.com> wrote: > On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote: >> I'd also prefer reverting ceaa001a170e since it's more clean but I >> also hope to have this feature in 4.15. >> How long does reverting take? Am I only able to submit the new patch >> after the reverting is merged? Or I can submit revert and this new >> patch in one series? I have little experience in reverting, can you >> suggest which way is better? > > Send the revert for net (subject will be "[PATCH net] revert: > openvswitch: Add erspan tunnel support."). Don't forget to explain why > you're proposing a revert. > > After it is accepted and applied to net.git, wait until the patch > appears in net-next.git. It may take a little while. After that, send > the new patch(es) for net-next. > I agree, Once we have the V2 interface, this current ERSAN interface unlikely to be used by any one, so it would be nice to get rid of the old interface while we can.
On Fri, Jan 12, 2018 at 10:39 AM, Pravin Shelar <pshelar@ovn.org> wrote: > On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc <jbenc@redhat.com> wrote: >> On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote: >>> I'd also prefer reverting ceaa001a170e since it's more clean but I >>> also hope to have this feature in 4.15. >>> How long does reverting take? Am I only able to submit the new patch >>> after the reverting is merged? Or I can submit revert and this new >>> patch in one series? I have little experience in reverting, can you >>> suggest which way is better? >> >> Send the revert for net (subject will be "[PATCH net] revert: >> openvswitch: Add erspan tunnel support."). Don't forget to explain why >> you're proposing a revert. >> >> After it is accepted and applied to net.git, wait until the patch >> appears in net-next.git. It may take a little while. After that, send >> the new patch(es) for net-next. >> > > I agree, Once we have the V2 interface, this current ERSAN interface > unlikely to be used by any one, so it would be nice to get rid of the > old interface while we can. Thanks Jiri and Pravin. I will send out revert patch request. William
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index 4265d7f9e1f2..77c3424cc4ef 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -273,6 +273,16 @@ enum { #define OVS_VXLAN_EXT_MAX (__OVS_VXLAN_EXT_MAX - 1) +enum { + OVS_ERSPAN_OPT_UNSPEC, + OVS_ERSPAN_OPT_IDX, /* be32 index */ + OVS_ERSPAN_OPT_VER, /* u8 version number */ + OVS_ERSPAN_OPT_DIR, /* u8 direction */ + OVS_ERSPAN_OPT_HWID, /* u8 hardware ID */ + __OVS_ERSPAN_OPT_MAX, +}; + +#define OVS_ERSPAN_OPT_MAX (__OVS_ERSPAN_OPT_MAX - 1) /* OVS_VPORT_ATTR_OPTIONS attributes for tunnels. */ @@ -363,7 +373,8 @@ enum ovs_tunnel_key_attr { OVS_TUNNEL_KEY_ATTR_IPV6_SRC, /* struct in6_addr src IPv6 address. */ OVS_TUNNEL_KEY_ATTR_IPV6_DST, /* struct in6_addr dst IPv6 address. */ OVS_TUNNEL_KEY_ATTR_PAD, - OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, /* be32 ERSPAN index. */ + OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1, /* be32 ERSPAN v1 index (deprecated). */ + OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, /* Nested OVS_ERSPAN_OPT_* */ __OVS_TUNNEL_KEY_ATTR_MAX }; diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index bce1f78b0de5..9c6b210e7893 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -335,7 +335,10 @@ size_t ovs_tun_key_attr_size(void) */ + nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_SRC */ + nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_DST */ - + nla_total_size(4); /* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS */ + + nla_total_size(4); /* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1 */ + /* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS is mutually exclusive with + * OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and covered by it. + */ } static size_t ovs_nsh_key_attr_size(void) @@ -386,6 +389,13 @@ static const struct ovs_len_tbl ovs_vxlan_ext_key_lens[OVS_VXLAN_EXT_MAX + 1] = [OVS_VXLAN_EXT_GBP] = { .len = sizeof(u32) }, }; +static const struct ovs_len_tbl ovs_erspan_opt_lens[OVS_ERSPAN_OPT_MAX + 1] = { + [OVS_ERSPAN_OPT_IDX] = { .len = sizeof(u32) }, + [OVS_ERSPAN_OPT_VER] = { .len = sizeof(u8) }, + [OVS_ERSPAN_OPT_DIR] = { .len = sizeof(u8) }, + [OVS_ERSPAN_OPT_HWID] = { .len = sizeof(u8) }, +}; + static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] = { [OVS_TUNNEL_KEY_ATTR_ID] = { .len = sizeof(u64) }, [OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = { .len = sizeof(u32) }, @@ -402,7 +412,9 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] .next = ovs_vxlan_ext_key_lens }, [OVS_TUNNEL_KEY_ATTR_IPV6_SRC] = { .len = sizeof(struct in6_addr) }, [OVS_TUNNEL_KEY_ATTR_IPV6_DST] = { .len = sizeof(struct in6_addr) }, - [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = sizeof(u32) }, + [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1] = { .len = sizeof(u32) }, + [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = OVS_ATTR_NESTED, + .next = ovs_erspan_opt_lens }, }; static const struct ovs_len_tbl @@ -640,16 +652,78 @@ static int erspan_tun_opt_from_nlattr(const struct nlattr *attr, { unsigned long opt_key_offset; struct erspan_metadata opts; + struct nlattr *a; + u16 hwid, dir; + int rem; BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts)); memset(&opts, 0, sizeof(opts)); - opts.u.index = nla_get_be32(attr); + nla_for_each_nested(a, attr, rem) { + int type = nla_type(a); + + if (type > OVS_ERSPAN_OPT_MAX) { + OVS_NLERR(log, "ERSPAN option %d out of range max %d", + type, OVS_ERSPAN_OPT_MAX); + return -EINVAL; + } + + if (!check_attr_len(nla_len(a), + ovs_erspan_opt_lens[type].len)) { + OVS_NLERR(log, "ERSPAN option %d has unexpected len %d expected %d", + type, nla_len(a), + ovs_erspan_opt_lens[type].len); + return -EINVAL; + } - /* Index has only 20-bit */ - if (ntohl(opts.u.index) & ~INDEX_MASK) { - OVS_NLERR(log, "ERSPAN index number %x too large.", - ntohl(opts.u.index)); + switch (type) { + case OVS_ERSPAN_OPT_IDX: + opts.u.index = nla_get_be32(a); + if (ntohl(opts.u.index) & ~INDEX_MASK) { + OVS_NLERR(log, + "ERSPAN index number %x too large.", + ntohl(opts.u.index)); + return -EINVAL; + } + break; + case OVS_ERSPAN_OPT_VER: + opts.version = nla_get_u8(a); + if (opts.version != 1 && opts.version != 2) { + OVS_NLERR(log, + "ERSPAN version %d not supported.", + opts.version); + return -EINVAL; + } + break; + case OVS_ERSPAN_OPT_DIR: + dir = nla_get_u8(a); + if (dir != 0 && dir != 1) { + OVS_NLERR(log, + "ERSPAN direction %d invalid.", + dir); + return -EINVAL; + } + opts.u.md2.dir = dir; + break; + case OVS_ERSPAN_OPT_HWID: + hwid = nla_get_u8(a); + if (hwid & ~(HWID_MASK >> HWID_OFFSET)) { + OVS_NLERR(log, + "ERSPAN hardware ID %x invalid.", + hwid); + return -EINVAL; + } + set_hwid(&opts.u.md2, hwid); + break; + default: + OVS_NLERR(log, "Unknown ERSPAN opt attribute %d", + type); + return -EINVAL; + } + } + if (rem) { + OVS_NLERR(log, "ERSPAN opt message has %d unknown bytes.", + rem); return -EINVAL; } @@ -768,6 +842,10 @@ static int ip_tun_from_nlattr(const struct nlattr *attr, break; case OVS_TUNNEL_KEY_ATTR_PAD: break; + case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1: + OVS_NLERR(log, "ERSPAN attribute %d is deprecated.", + type); + return -EINVAL; case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS: if (opts_type) { OVS_NLERR(log, "Multiple metadata blocks provided"); @@ -846,6 +924,39 @@ static int vxlan_opt_to_nlattr(struct sk_buff *skb, return 0; } +static int erspan_opt_to_nlattr(struct sk_buff *skb, + const void *tun_opts, int swkey_tun_opts_len) +{ + const struct erspan_metadata *opts = tun_opts; + struct nlattr *nla; + + nla = nla_nest_start(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS); + if (!nla) + return -EMSGSIZE; + + if (nla_put_u8(skb, OVS_ERSPAN_OPT_VER, opts->version) < 0) + return -EMSGSIZE; + + if (opts->version == 1) { + if (nla_put_be32(skb, OVS_ERSPAN_OPT_IDX, opts->u.index) < 0) + return -EMSGSIZE; + + } else if (opts->version == 2) { + if (nla_put_u8(skb, OVS_ERSPAN_OPT_DIR, + opts->u.md2.dir) < 0) + return -EMSGSIZE; + + if (nla_put_u8(skb, OVS_ERSPAN_OPT_HWID, + get_hwid(&opts->u.md2)) < 0) + return -EMSGSIZE; + } else { + return -EINVAL; + } + + nla_nest_end(skb, nla); + return 0; +} + static int __ip_tun_to_nlattr(struct sk_buff *skb, const struct ip_tunnel_key *output, const void *tun_opts, int swkey_tun_opts_len, @@ -906,8 +1017,8 @@ static int __ip_tun_to_nlattr(struct sk_buff *skb, vxlan_opt_to_nlattr(skb, tun_opts, swkey_tun_opts_len)) return -EMSGSIZE; else if (output->tun_flags & TUNNEL_ERSPAN_OPT && - nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, - ((struct erspan_metadata *)tun_opts)->u.index)) + erspan_opt_to_nlattr(skb, tun_opts, + swkey_tun_opts_len)) return -EMSGSIZE; }
The patch adds support for configuring the erspan V2 fields for openvswitch. For compatibility reason, the previously added attribute 'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS' is renamed to 'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1' and deprecated, and the newly added attribute 'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS' will handle both V1 and V2. Signed-off-by: William Tu <u9012063@gmail.com> Cc: Pravin B Shelar <pshelar@ovn.org> --- include/uapi/linux/openvswitch.h | 13 +++- net/openvswitch/flow_netlink.c | 129 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 132 insertions(+), 10 deletions(-)