Thanks for the review.
On Thu, Jan 25, 2018 at 9:32 AM, Pravin Shelar <pshe...@ovn.org> wrote:
> On Wed, Jan 24, 2018 at 11:06 AM, William Tu <u9012...@gmail.com> wrote:
>> The patch adds support for openvswitch to configure erspan
>> v1 and v2. The OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS attr is added
>> to uapi as a binary blob to support all ERSPAN v1 and v2's
>> fields. Note that Previous commit "openvswitch: Add erspan tunnel
>> support." was reverted since it does not design properly.
>>
>> Signed-off-by: William Tu <u9012...@gmail.com>
>> ---
>> include/uapi/linux/openvswitch.h | 2 +-
>> net/openvswitch/flow_netlink.c | 90
>> +++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 90 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/openvswitch.h
>> b/include/uapi/linux/openvswitch.h
>> index dcfab5e3b55c..158c2e45c0a5 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -273,7 +273,6 @@ enum {
>>
>> #define OVS_VXLAN_EXT_MAX (__OVS_VXLAN_EXT_MAX - 1)
>>
>> -
>> /* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
>> */
>> enum {
>> @@ -363,6 +362,7 @@ 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, /* struct erspan_metadata */
>> __OVS_TUNNEL_KEY_ATTR_MAX
>> };
>>
> Since this is uapi, we need to define the struct erspan_metadata in a
> UAPI header file.
Should I define "struct erspan_metadata" in include/uapi/linux/openvswitch.h?
Or I'm planning to create a new file in uapi "include/uapi/linux/erspan.h",
define "struct erspan_metadata" there, and remove its duplicate at
include/net/erspan.h.
>
> Also lets move field 'version' to the begining of the struct for easy
> expansion later.
> struct erspan_metadata {
> int version;
> union {
> __be32 index; /* Version 1 (type II)*/
> struct erspan_md2 md2; /* Version 2 (type III) */
> } u;
> };
>
Sure, will do it.
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index f143908b651d..9d00c24b2836 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -49,6 +49,7 @@
>> #include <net/mpls.h>
>> #include <net/vxlan.h>
>> #include <net/tun_proto.h>
>> +#include <net/erspan.h>
>>
>> #include "flow_netlink.h"
>>
>> @@ -329,7 +330,8 @@ size_t ovs_tun_key_attr_size(void)
>> + nla_total_size(0) /* OVS_TUNNEL_KEY_ATTR_CSUM */
>> + nla_total_size(0) /* OVS_TUNNEL_KEY_ATTR_OAM */
>> + nla_total_size(256) /* OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS */
>> - /* OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS is mutually exclusive with
>> + /* OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS and
>> + * OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS is mutually exclusive with
>> * OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and covered by it.
>> */
>> + nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_SRC */
>> @@ -400,6 +402,7 @@ 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 = OVS_ATTR_VARIABLE },
>> };
>>
>> static const struct ovs_len_tbl
>> @@ -631,6 +634,33 @@ static int vxlan_tun_opt_from_nlattr(const struct
>> nlattr *attr,
>> return 0;
>> }
>>
>> +static int erspan_tun_opt_from_nlattr(const struct nlattr *a,
>> + struct sw_flow_match *match, bool
>> is_mask,
>> + bool log)
>> +{
>> + unsigned long opt_key_offset;
>> +
>> + BUILD_BUG_ON(sizeof(struct erspan_metadata) >
>> + sizeof(match->key->tun_opts));
>> +
>> + if (nla_len(a) > sizeof(match->key->tun_opts)) {
>> + OVS_NLERR(log, "ERSPAN option length err (len %d, max %zu).",
>> + nla_len(a), sizeof(match->key->tun_opts));
>> + return -EINVAL;
>> + }
>> +
>> + if (!is_mask)
>> + SW_FLOW_KEY_PUT(match, tun_opts_len,
>> + sizeof(struct erspan_metadata), false);
>> + else
>> + SW_FLOW_KEY_PUT(match, tun_opts_len, 0xff, true);
>> +
>> + opt_key_offset = TUN_METADATA_OFFSET(nla_len(a));
>> + SW_FLOW_KEY_MEMCPY_OFFSET(match, opt_key_offset, nla_data(a),
>> + nla_len(a), is_mask);
>> + return 0;
>> +}
>> +
> ...
>> @@ -2461,6 +2509,41 @@ static int validate_geneve_opts(struct sw_flow_key
>> *key)
>> return 0;
>> }
>>
>> +static int validate_erspan_opts(struct sw_flow_key *key, bool log)
>> +{
> I do not see any need to validate ERSPAN key values, except the total
> len, which should be less than 255.
>
OK, the total len has been checked at erspan_tun_opt_from_nlattr().
I will remove this extra validation.
Thanks
William
