Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

On Fri, Jan 12, 2018 at 10:39 AM, Pravin Shelar  wrote:
> On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc  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

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Pravin Shelar]

On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc  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.

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

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

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

Hi Jiri,
Thanks a lot for the comments.

On Wed, Jan 10, 2018 at 2:02 PM, Jiri Benc  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

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

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

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

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

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

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

[PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

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 
Cc: Pravin B Shelar 
---
 include/uapi/linux/openvswitch.h |  13 +++-
 net/openvswitch/flow_netlink.c   | 129 ---
 2 files changed, 132 insertions(+), 10 deletions(-)

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(, 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),
+