Message-ID: <81250230-a845-b03f-6600-b716c02f5137@nvidia.com>
Date:   Wed, 9 Mar 2022 09:49:59 +0200
From:   Roi Dayan <roid@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Ilya Maximets <i.maximets@....org>
Cc:     Johannes Berg <johannes@...solutions.net>, dev@...nvswitch.org,
        Toms Atteka <cpp.code.lv@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, davem@...emloft.net,
        David Ahern <dsahern@...il.com>, Jiri Pirko <jiri@...nulli.us>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        "pshelar@....org" <pshelar@....org>
Subject: Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6
 extension header support



On 2022-03-08 10:16 PM, Jakub Kicinski wrote:
> On Tue, 8 Mar 2022 20:33:12 +0100 Ilya Maximets wrote:
>> On 3/8/22 17:17, Jakub Kicinski wrote:
>>> On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote:
>>>> Yes, that is something that I had in mind too.  The only thing that makes
>>>> me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here.  Even though it
>>>> doesn't make a lot of difference, I'd better keep the kernel-only attributes
>>>> at the end of the enumeration.  Is there a better way to handle kernel-only
>>>> attribute?
>>>
>>> My thought was to leave the kernel/userspace only types "behind" to
>>> avoid perpetuating the same constructs.
>>>
>>> Johannes's point about userspace to userspace messages makes the whole
>>> thing a little less of an aberration.
>>>
>>> Is there a reason for the types to be hidden under __KERNEL__?
>>> Or someone did that in a misguided attempt to save space in attr arrays
>>> when parsing?
>>
>> My impression is that OVS_KEY_ATTR_TUNNEL_INFO was hidden from the
>> user space just because it's not supposed to ever be used by the
>> user space application.  Pravin or Jesse should know for sure.
> 
> Hard to make any assumptions about what user space that takes
> the liberty of re-defining kernel uAPI types will or will not
> do ;) The only way to be safe would be to actively reject the
> attr ID on the kernel side. Unless user space uses the same
> exact name for something else IMHO hiding the value doesn't
> afford us any extra protection.
> 
>>>> I agree with that.
>>>
>>> Should we add the user space types to the kernel header and remove the
>>> ifdef __KERNEL__ around TUNNEL_INFO, then?
>>
>> I don't think we need to actually define them, but we may list them
>> in the comment.  I'm OK with either option though.
>>
>> For the removal of ifdef __KERNEL__, that might be a good thing to do.
>> I'm just not sure what are the best practices here.
>> We'll need to make some code changes in user space to avoid warnings
>> about not all the enum members being used in 'switch'es.  But that's
>> not a problem.
> 
> Presumably only as the headers are updated? IOW we would not break
> the build for older sources?
> 
>> If you think that having a flat enum without 'ifdef's is a viable
>> option from a kernel's point of view, I'm all for it.
>>
>> Maybe something like this (only checked that this compiles; 29 and
>> 30 are correct numbers of these userspace attributes):
> 
> It's a bit of an uncharted territory, hard to say what's right.
> It may be a little easier to code up the rejection if we have
> the types defined (which I think we need to do in
> __parse_flow_nlattrs()? seems OvS does its own nla parsing?)
> 
> Johannes, any preference?
> 
>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>> index 9d1710f20505..86bc951be5bc 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -351,11 +351,19 @@ enum ovs_key_attr {
>>   	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4,   /* struct ovs_key_ct_tuple_ipv4 */
>>   	OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6,   /* struct ovs_key_ct_tuple_ipv6 */
>>   	OVS_KEY_ATTR_NSH,       /* Nested set of ovs_nsh_key_* */
>> -	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>   
>> -#ifdef __KERNEL__
>> -	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
>> -#endif
>> +	/* User space decided to squat on types 29 and 30.  They are listed
>> +	 * below, but should not be sent to the kernel:
>> +	 *
>> +	 * OVS_KEY_ATTR_PACKET_TYPE,   be32 packet type
>> +	 * OVS_KEY_ATTR_ND_EXTENSIONS, IPv6 Neighbor Discovery extensions
>> +	 *
>> +	 * 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_TUNNEL_INFO = 31,  /* struct ip_tunnel_info.
>> +					   For in-kernel use only. */
>> +	OVS_KEY_ATTR_IPV6_EXTHDRS,  /* struct ovs_key_ipv6_exthdr */
>>   	__OVS_KEY_ATTR_MAX
>>   };

so why not again just flat enum without ifdefs and without values
commented out? even if we leave values in comments like above it doesn't
mean the userspace won't use them by mistake and send to the kernel.
but the kernel will probably ignore as not being used and at least
there won't be a conflict again even if by mistake.. and it's easiest
to read.

>>   
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index 8b4124820f7d..315064bada3e 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -346,7 +346,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 != 30);
>> +	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
>>   
>>   	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
>>   		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
>> ---
>>
>> Thoughts?
>>
>> The same change can be ported to the user-space header, but with
>> types actually defined and not part of the comment.  It may look
>> like this: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpastebin.com%2Fk8UWEZtR&amp;data=04%7C01%7Croid%40nvidia.com%7C3f34544168c14459f44608da01408358%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637823673860054963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=HYfuir9d21MFFxPibJz%2FppfxsylDMLz0CZIOcSCLQQw%3D&amp;reserved=0  (without IPV6_EXTHDRS yet).
>> For the future, we'll try to find a way to define them in a separate
>> enum or will define them dynamically based on the policy dumped from
>> the currently running kernel. In any case no new userspace-only types
>> should be defined in that enum.
> 

Powered by blists - more mailing lists