[ovs-dev,V2,09/11] compat: Use nla_parse deprecated functions
diff mbox series

Message ID 1582738882-15842-10-git-send-email-gvrose8192@gmail.com
State Superseded
Headers show
Series
  • Add Linux kernel datapath support up to 5.5
Related show

Commit Message

Gregory Rose Feb. 26, 2020, 5:41 p.m. UTC
Changes for in kernel generated netlink attribute parsing functions
require our out of tree driver to use the deprecated forms of those
functions.  Otherwise the message parsing will return -EINVAL because
NLA_F_NESTED is not set in the nla_type field.

Signed-off-by: Greg Rose <gvrose8192@gmail.com>
---
 acinclude.m4                                | 3 +++
 datapath/linux/compat/include/net/netlink.h | 5 +++++
 2 files changed, 8 insertions(+)

Comments

Yi-Hung Wei Feb. 28, 2020, 12:39 a.m. UTC | #1
On Wed, Feb 26, 2020 at 9:42 AM Greg Rose <gvrose8192@gmail.com> wrote:
>
> Changes for in kernel generated netlink attribute parsing functions
> require our out of tree driver to use the deprecated forms of those
> functions.  Otherwise the message parsing will return -EINVAL because
> NLA_F_NESTED is not set in the nla_type field.
>
> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
> ---
>  acinclude.m4                                | 3 +++
>  datapath/linux/compat/include/net/netlink.h | 5 +++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index a55c905..43d1576 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -1072,6 +1072,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>    OVS_FIND_FIELD_IFELSE([$KSRC/include/net/genetlink.h], [genl_ops],
>                          [policy],
>                          [OVS_DEFINE([HAVE_GENL_OPS_POLICY])])
> +  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h],
> +                  [nla_parse_deprecated_strict],
> +                  [OVS_DEFINE([HAVE_NLA_PARSE_DEPRECATED_STRICT])])
>
>    if cmp -s datapath/linux/kcompat.h.new \
>              datapath/linux/kcompat.h >/dev/null 2>&1; then
> diff --git a/datapath/linux/compat/include/net/netlink.h b/datapath/linux/compat/include/net/netlink.h
> index 34fc346..7c0d993 100644
> --- a/datapath/linux/compat/include/net/netlink.h
> +++ b/datapath/linux/compat/include/net/netlink.h
> @@ -143,6 +143,10 @@ static inline int nla_put_be64(struct sk_buff *skb, int attrtype, __be64 value,
>
>  #endif
>
> +#ifdef HAVE_NLA_PARSE_DEPRECATED_STRICT
> +#define nla_parse_nested nla_parse_nested_deprecated
> +#define nla_parse nla_parse_deprecated_strict

Thanks for this patch.  Looks like this change is related to upstream
patch 8cb081746c031 ("netlink: make validation more configurable for
future strictness").

To be in sync with the upstream kernel, should we use the new upstream
netlink parsing APIs (i.e. nla_parse_nested_deprecated(),
nla_parse_deprecated_strict(), genlmsg_parse_deprecated()), and
backports them to the old APIs if they are not available in the older
kernel?

Thanks,

-Yi-Hung


> +#else
>  #ifndef HAVE_NETLINK_EXT_ACK
>  struct netlink_ext_ack;
>
> @@ -164,6 +168,7 @@ static inline int rpl_nla_parse(struct nlattr **tb, int maxtype,
>  }
>  #define nla_parse rpl_nla_parse
>  #endif
> +#endif /* HAVE_NLA_PARSE_DEPRECATED_STRICT */
>
>  #ifndef HAVE_NLA_NEST_START_NOFLAG
>  static inline struct nlattr *rpl_nla_nest_start_noflag(struct sk_buff *skb,
> --
Gregory Rose Feb. 28, 2020, 5:21 p.m. UTC | #2
On 2/27/2020 4:39 PM, Yi-Hung Wei wrote:
> On Wed, Feb 26, 2020 at 9:42 AM Greg Rose <gvrose8192@gmail.com> wrote:
>> Changes for in kernel generated netlink attribute parsing functions
>> require our out of tree driver to use the deprecated forms of those
>> functions.  Otherwise the message parsing will return -EINVAL because
>> NLA_F_NESTED is not set in the nla_type field.
>>
>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>> ---
>>   acinclude.m4                                | 3 +++
>>   datapath/linux/compat/include/net/netlink.h | 5 +++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/acinclude.m4 b/acinclude.m4
>> index a55c905..43d1576 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -1072,6 +1072,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>>     OVS_FIND_FIELD_IFELSE([$KSRC/include/net/genetlink.h], [genl_ops],
>>                           [policy],
>>                           [OVS_DEFINE([HAVE_GENL_OPS_POLICY])])
>> +  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h],
>> +                  [nla_parse_deprecated_strict],
>> +                  [OVS_DEFINE([HAVE_NLA_PARSE_DEPRECATED_STRICT])])
>>
>>     if cmp -s datapath/linux/kcompat.h.new \
>>               datapath/linux/kcompat.h >/dev/null 2>&1; then
>> diff --git a/datapath/linux/compat/include/net/netlink.h b/datapath/linux/compat/include/net/netlink.h
>> index 34fc346..7c0d993 100644
>> --- a/datapath/linux/compat/include/net/netlink.h
>> +++ b/datapath/linux/compat/include/net/netlink.h
>> @@ -143,6 +143,10 @@ static inline int nla_put_be64(struct sk_buff *skb, int attrtype, __be64 value,
>>
>>   #endif
>>
>> +#ifdef HAVE_NLA_PARSE_DEPRECATED_STRICT
>> +#define nla_parse_nested nla_parse_nested_deprecated
>> +#define nla_parse nla_parse_deprecated_strict
> Thanks for this patch.  Looks like this change is related to upstream
> patch 8cb081746c031 ("netlink: make validation more configurable for
> future strictness").
>
> To be in sync with the upstream kernel, should we use the new upstream
> netlink parsing APIs (i.e. nla_parse_nested_deprecated(),
> nla_parse_deprecated_strict(), genlmsg_parse_deprecated()), and
> backports them to the old APIs if they are not available in the older
> kernel?

Hi Yi-Hung,

I'm not sure that provides much value since we never intend (SFAIK) to
start using the NLA_F_NESTED nla_type in our out of tree kernel code.
I think this should be sufficient but I'm CC'ing David Ahern to get his
opinion since he suggested that I use the deprecated functions for
our out of tree driver.

David,

any suggestion?

Thanks,

- Greg

>
> Thanks,
>
> -Yi-Hung
>
>
>> +#else
>>   #ifndef HAVE_NETLINK_EXT_ACK
>>   struct netlink_ext_ack;
>>
>> @@ -164,6 +168,7 @@ static inline int rpl_nla_parse(struct nlattr **tb, int maxtype,
>>   }
>>   #define nla_parse rpl_nla_parse
>>   #endif
>> +#endif /* HAVE_NLA_PARSE_DEPRECATED_STRICT */
>>
>>   #ifndef HAVE_NLA_NEST_START_NOFLAG
>>   static inline struct nlattr *rpl_nla_nest_start_noflag(struct sk_buff *skb,
>> --
David Ahern Feb. 28, 2020, 5:42 p.m. UTC | #3
On 2/28/20 10:21 AM, Gregory Rose wrote:
> 
> On 2/27/2020 4:39 PM, Yi-Hung Wei wrote:
>> On Wed, Feb 26, 2020 at 9:42 AM Greg Rose <gvrose8192@gmail.com> wrote:
>>> Changes for in kernel generated netlink attribute parsing functions
>>> require our out of tree driver to use the deprecated forms of those
>>> functions.  Otherwise the message parsing will return -EINVAL because
>>> NLA_F_NESTED is not set in the nla_type field.
>>>
>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>> ---
>>>   acinclude.m4                                | 3 +++
>>>   datapath/linux/compat/include/net/netlink.h | 5 +++++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/acinclude.m4 b/acinclude.m4
>>> index a55c905..43d1576 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -1072,6 +1072,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>>>     OVS_FIND_FIELD_IFELSE([$KSRC/include/net/genetlink.h], [genl_ops],
>>>                           [policy],
>>>                           [OVS_DEFINE([HAVE_GENL_OPS_POLICY])])
>>> +  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h],
>>> +                  [nla_parse_deprecated_strict],
>>> +                  [OVS_DEFINE([HAVE_NLA_PARSE_DEPRECATED_STRICT])])
>>>
>>>     if cmp -s datapath/linux/kcompat.h.new \
>>>               datapath/linux/kcompat.h >/dev/null 2>&1; then
>>> diff --git a/datapath/linux/compat/include/net/netlink.h
>>> b/datapath/linux/compat/include/net/netlink.h
>>> index 34fc346..7c0d993 100644
>>> --- a/datapath/linux/compat/include/net/netlink.h
>>> +++ b/datapath/linux/compat/include/net/netlink.h
>>> @@ -143,6 +143,10 @@ static inline int nla_put_be64(struct sk_buff
>>> *skb, int attrtype, __be64 value,
>>>
>>>   #endif
>>>
>>> +#ifdef HAVE_NLA_PARSE_DEPRECATED_STRICT
>>> +#define nla_parse_nested nla_parse_nested_deprecated
>>> +#define nla_parse nla_parse_deprecated_strict
>> Thanks for this patch.  Looks like this change is related to upstream
>> patch 8cb081746c031 ("netlink: make validation more configurable for
>> future strictness").
>>
>> To be in sync with the upstream kernel, should we use the new upstream
>> netlink parsing APIs (i.e. nla_parse_nested_deprecated(),
>> nla_parse_deprecated_strict(), genlmsg_parse_deprecated()), and
>> backports them to the old APIs if they are not available in the older
>> kernel?
> 
> Hi Yi-Hung,
> 
> I'm not sure that provides much value since we never intend (SFAIK) to
> start using the NLA_F_NESTED nla_type in our out of tree kernel code.
> I think this should be sufficient but I'm CC'ing David Ahern to get his
> opinion since he suggested that I use the deprecated functions for
> our out of tree driver.
> 
> David,
> 
> any suggestion?
> 

The intent is that all new code uses the strict parsing functions. Older
code has to use the deprecated ones to maintain backwards compatibility.

The new parsing functions are very strict and expect all attributes to
be properly validated (e.g., a u32 is 4 bytes, a u8 is 1 byte), with
proper flags (e.g., the NESTED one) and all bytes accounted for.
Gregory Rose Feb. 28, 2020, 5:46 p.m. UTC | #4
On 2/28/2020 9:42 AM, David Ahern wrote:
> On 2/28/20 10:21 AM, Gregory Rose wrote:
>> On 2/27/2020 4:39 PM, Yi-Hung Wei wrote:
>>> On Wed, Feb 26, 2020 at 9:42 AM Greg Rose <gvrose8192@gmail.com> wrote:
>>>> Changes for in kernel generated netlink attribute parsing functions
>>>> require our out of tree driver to use the deprecated forms of those
>>>> functions.  Otherwise the message parsing will return -EINVAL because
>>>> NLA_F_NESTED is not set in the nla_type field.
>>>>
>>>> Signed-off-by: Greg Rose <gvrose8192@gmail.com>
>>>> ---
>>>>    acinclude.m4                                | 3 +++
>>>>    datapath/linux/compat/include/net/netlink.h | 5 +++++
>>>>    2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/acinclude.m4 b/acinclude.m4
>>>> index a55c905..43d1576 100644
>>>> --- a/acinclude.m4
>>>> +++ b/acinclude.m4
>>>> @@ -1072,6 +1072,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>>>>      OVS_FIND_FIELD_IFELSE([$KSRC/include/net/genetlink.h], [genl_ops],
>>>>                            [policy],
>>>>                            [OVS_DEFINE([HAVE_GENL_OPS_POLICY])])
>>>> +  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h],
>>>> +                  [nla_parse_deprecated_strict],
>>>> +                  [OVS_DEFINE([HAVE_NLA_PARSE_DEPRECATED_STRICT])])
>>>>
>>>>      if cmp -s datapath/linux/kcompat.h.new \
>>>>                datapath/linux/kcompat.h >/dev/null 2>&1; then
>>>> diff --git a/datapath/linux/compat/include/net/netlink.h
>>>> b/datapath/linux/compat/include/net/netlink.h
>>>> index 34fc346..7c0d993 100644
>>>> --- a/datapath/linux/compat/include/net/netlink.h
>>>> +++ b/datapath/linux/compat/include/net/netlink.h
>>>> @@ -143,6 +143,10 @@ static inline int nla_put_be64(struct sk_buff
>>>> *skb, int attrtype, __be64 value,
>>>>
>>>>    #endif
>>>>
>>>> +#ifdef HAVE_NLA_PARSE_DEPRECATED_STRICT
>>>> +#define nla_parse_nested nla_parse_nested_deprecated
>>>> +#define nla_parse nla_parse_deprecated_strict
>>> Thanks for this patch.  Looks like this change is related to upstream
>>> patch 8cb081746c031 ("netlink: make validation more configurable for
>>> future strictness").
>>>
>>> To be in sync with the upstream kernel, should we use the new upstream
>>> netlink parsing APIs (i.e. nla_parse_nested_deprecated(),
>>> nla_parse_deprecated_strict(), genlmsg_parse_deprecated()), and
>>> backports them to the old APIs if they are not available in the older
>>> kernel?
>> Hi Yi-Hung,
>>
>> I'm not sure that provides much value since we never intend (SFAIK) to
>> start using the NLA_F_NESTED nla_type in our out of tree kernel code.
>> I think this should be sufficient but I'm CC'ing David Ahern to get his
>> opinion since he suggested that I use the deprecated functions for
>> our out of tree driver.
>>
>> David,
>>
>> any suggestion?
>>
> The intent is that all new code uses the strict parsing functions. Older
> code has to use the deprecated ones to maintain backwards compatibility.
>
> The new parsing functions are very strict and expect all attributes to
> be properly validated (e.g., a u32 is 4 bytes, a u8 is 1 byte), with
> proper flags (e.g., the NESTED one) and all bytes accounted for.

Thanks for the clarification David.

Yi-Hung,

let me take this offline with you and we can discuss the correct solution.

- Greg

Patch
diff mbox series

diff --git a/acinclude.m4 b/acinclude.m4
index a55c905..43d1576 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -1072,6 +1072,9 @@  AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_FIND_FIELD_IFELSE([$KSRC/include/net/genetlink.h], [genl_ops],
                         [policy],
                         [OVS_DEFINE([HAVE_GENL_OPS_POLICY])])
+  OVS_GREP_IFELSE([$KSRC/include/net/netlink.h],
+                  [nla_parse_deprecated_strict],
+                  [OVS_DEFINE([HAVE_NLA_PARSE_DEPRECATED_STRICT])])
 
   if cmp -s datapath/linux/kcompat.h.new \
             datapath/linux/kcompat.h >/dev/null 2>&1; then
diff --git a/datapath/linux/compat/include/net/netlink.h b/datapath/linux/compat/include/net/netlink.h
index 34fc346..7c0d993 100644
--- a/datapath/linux/compat/include/net/netlink.h
+++ b/datapath/linux/compat/include/net/netlink.h
@@ -143,6 +143,10 @@  static inline int nla_put_be64(struct sk_buff *skb, int attrtype, __be64 value,
 
 #endif
 
+#ifdef HAVE_NLA_PARSE_DEPRECATED_STRICT
+#define nla_parse_nested nla_parse_nested_deprecated
+#define nla_parse nla_parse_deprecated_strict
+#else
 #ifndef HAVE_NETLINK_EXT_ACK
 struct netlink_ext_ack;
 
@@ -164,6 +168,7 @@  static inline int rpl_nla_parse(struct nlattr **tb, int maxtype,
 }
 #define nla_parse rpl_nla_parse
 #endif
+#endif /* HAVE_NLA_PARSE_DEPRECATED_STRICT */
 
 #ifndef HAVE_NLA_NEST_START_NOFLAG
 static inline struct nlattr *rpl_nla_nest_start_noflag(struct sk_buff *skb,