[ovs-dev] [PATCH] dpif: Ensure ERSPAN GRE support

[Gregory Rose]

On 6/1/2018 6:15 AM, Eric Garver wrote:
> I'm a bit late, but I have comments below.
>
> I'm also a bit out of touch, so I may be missing some context - if so, I
> apologize.
>
> On Thu, May 31, 2018 at 03:50:31PM -0700, Greg Rose wrote:
>> When verifying the built-in gre kernel module check for ERSPAN support.
>>
>> Reported-by: Guru Shetty <guru at ovn.org>
>> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
>> ---
>>   lib/dpif-netlink-rtnl.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
>> index bec3fce..197cfb6 100644
>> --- a/lib/dpif-netlink-rtnl.c
>> +++ b/lib/dpif-netlink-rtnl.c
>> @@ -45,8 +45,8 @@ VLOG_DEFINE_THIS_MODULE(dpif_netlink_rtnl);
>>   #ifndef IFLA_GRE_MAX
>>   #define IFLA_GRE_MAX 0
>>   #endif
>> -#if IFLA_GRE_MAX < 18
>> -#define IFLA_GRE_COLLECT_METADATA 18
>> +#if IFLA_GRE_MAX < 24
>> +#define IFLA_GRE_ERSPAN_HWID 24
>>   #endif
>>   
>>   #ifndef IFLA_GENEVE_MAX
>> @@ -74,7 +74,7 @@ static const struct nl_policy vxlan_policy[] = {
>>       [IFLA_VXLAN_GPE] = { .type = NL_A_FLAG, .optional = true },
>>   };
>>   static const struct nl_policy gre_policy[] = {
>> -    [IFLA_GRE_COLLECT_METADATA] = { .type = NL_A_FLAG },
> I think we still need to verify _COLLECT_METADATA was passed back.
>
>> +    [IFLA_GRE_ERSPAN_HWID] = { .type = NL_A_U16 },
>>   };
>>   static const struct nl_policy geneve_policy[] = {
>>       [IFLA_GENEVE_COLLECT_METADATA] = { .type = NL_A_FLAG },
>> @@ -207,7 +207,7 @@ dpif_netlink_rtnl_gre_verify(const struct netdev_tunnel_config OVS_UNUSED *tnl,
>>       err = rtnl_policy_parse(kind, reply, gre_policy, gre,
>>                               ARRAY_SIZE(gre_policy));
>>       if (!err) {
>> -        if (!nl_attr_get_flag(gre[IFLA_GRE_COLLECT_METADATA])) {
>> +        if (!nl_attr_get_u16(gre[IFLA_GRE_ERSPAN_HWID])) {
> We need to verify COLLECT_METADATA is actually in use here. We should
> only be checking for ERSPAN if ERSPAN was requested by the user.

I'm not super familiar with the code but the intent is to prevent the 
built-in ip_gre kernel module
from being used if it doesn't support ERSPAN.  If the built-in gre and 
ip_gre kernel modules
are loaded and the kernel doesn't have ERSPAN (4.16 and above) then OVS 
users won't
be able to use ERSPAN features.

- Greg

>
>>               err = EINVAL;
>>           }
>>       }
>> @@ -328,7 +328,7 @@ dpif_netlink_rtnl_create(const struct netdev_tunnel_config *tnl_cfg,
>>       case OVS_VPORT_TYPE_ERSPAN:
>>       case OVS_VPORT_TYPE_IP6ERSPAN:
>>       case OVS_VPORT_TYPE_IP6GRE:
>> -        nl_msg_put_flag(&request, IFLA_GRE_COLLECT_METADATA);
> I think we still need to use COLLECT_METADATA. Don't we depend on using
> lwt?
>
>> +        nl_msg_put_u16(&request, IFLA_GRE_ERSPAN_HWID, 0xdead);
>>           break;
>>       case OVS_VPORT_TYPE_GENEVE:
>>           nl_msg_put_flag(&request, IFLA_GENEVE_COLLECT_METADATA);