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
> };
>
> 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://pastebin.com/k8UWEZtR (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.
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&data=04%7C01%7Croid%40nvidia.com%7C3f34544168c14459f44608da01408358%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637823673860054963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=HYfuir9d21MFFxPibJz%2FppfxsylDMLz0CZIOcSCLQQw%3D&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.
>
On 3/9/22 08:49, Roi Dayan wrote:
>
>
> 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?
Yep. OVS doesn't use kernel header directly, so the change will be
needed only when the header in OVS repo is updated. Current/older
sources will be fine.
>>
>>> 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?)
Ack. And, yes, __parse_flow_nlattrs() seems to be the right spot.
OVS has lots of nested attributes with some special treatment in a
few cases and dependency tracking, AFAICT, so it parses attributes
on it's own. Is there a better way?
>>
>> 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.
OK. Seems like we have some votes and a reason (explicit reject) to have
them defined. This will also make current user space and kernel definitions
equal. Let me put together a patch and we'll continue the discussion there.
>
>>> 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&data=04%7C01%7Croid%40nvidia.com%7C3f34544168c14459f44608da01408358%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637823673860054963%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=HYfuir9d21MFFxPibJz%2FppfxsylDMLz0CZIOcSCLQQw%3D&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.
>>
On Wed, 9 Mar 2022 14:43:07 +0100 Ilya Maximets wrote:
> >> 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?)
>
> Ack. And, yes, __parse_flow_nlattrs() seems to be the right spot.
> OVS has lots of nested attributes with some special treatment in a
> few cases and dependency tracking, AFAICT, so it parses attributes
> on it's own. Is there a better way?
Looks like OvS has extra requirements like attributes can't be
duplicated and zeroed out attrs are ignored. I don't think generic
parsers can do that today, although the former seems like a useful
addition.
A problem for another time.
> >> Johannes, any preference?
> >
> > 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.
>
> OK. Seems like we have some votes and a reason (explicit reject) to have
> them defined. This will also make current user space and kernel definitions
> equal. Let me put together a patch and we'll continue the discussion there.
Thanks!