diff mbox series

[ovs-dev,v2,4/7] tc: Add NLA_F_NESTED to nested netlink messages

Message ID 20230425124122.2443376-5-roid@nvidia.com
State Changes Requested
Headers show
Series Add vxlan gbp offload with TC | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Roi Dayan April 25, 2023, 12:41 p.m. UTC 
From: Gavin Li <gavinl@nvidia.com>

Linux kernel netlink module added NLA_F_NESTED flag checking for nested
netlink messages in 5.2. A nested message without the flag set will be
treated as malformated one. The check is optional and is controlled by
message policy. To avoid this, add NLA_F_NESTED explicitly for all
nested netlink messages.

Signed-off-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 lib/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ilya Maximets April 26, 2023, 4:39 p.m. UTC | #1 
On 4/25/23 14:41, Roi Dayan wrote:
> From: Gavin Li <gavinl@nvidia.com>
> 
> Linux kernel netlink module added NLA_F_NESTED flag checking for nested
> netlink messages in 5.2. A nested message without the flag set will be
> treated as malformated one. The check is optional and is controlled by
> message policy. To avoid this, add NLA_F_NESTED explicitly for all
> nested netlink messages.
> 
> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> ---
>  lib/netlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/netlink.c b/lib/netlink.c
> index 6215282d6fbe..f128b63074f9 100644
> --- a/lib/netlink.c
> +++ b/lib/netlink.c
> @@ -519,7 +519,7 @@ size_t
>  nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
>  {
>      size_t offset = msg->size;
> -    nl_msg_put_unspec_uninit(msg, type, 0);
> +    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
>      return offset;
>  }
>  


Just posting a link to the ongoing v1 conversation on this patch here
for visibility:
  https://patchwork.ozlabs.org/project/openvswitch/patch/20230425063238.2366798-5-roid@nvidia.com/

Best regards, Ilya Maximets.
Simon Horman May 10, 2023, 2:10 p.m. UTC | #2 
On Wed, Apr 26, 2023 at 06:39:22PM +0200, Ilya Maximets wrote:
> On 4/25/23 14:41, Roi Dayan wrote:
> > From: Gavin Li <gavinl@nvidia.com>
> > 
> > Linux kernel netlink module added NLA_F_NESTED flag checking for nested
> > netlink messages in 5.2. A nested message without the flag set will be
> > treated as malformated one. The check is optional and is controlled by
> > message policy. To avoid this, add NLA_F_NESTED explicitly for all
> > nested netlink messages.
> > 
> > Signed-off-by: Gavin Li <gavinl@nvidia.com>
> > Reviewed-by: Roi Dayan <roid@nvidia.com>
> > ---
> >  lib/netlink.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/netlink.c b/lib/netlink.c
> > index 6215282d6fbe..f128b63074f9 100644
> > --- a/lib/netlink.c
> > +++ b/lib/netlink.c
> > @@ -519,7 +519,7 @@ size_t
> >  nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
> >  {
> >      size_t offset = msg->size;
> > -    nl_msg_put_unspec_uninit(msg, type, 0);
> > +    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
> >      return offset;
> >  }
> >  
> 
> 
> Just posting a link to the ongoing v1 conversation on this patch here
> for visibility:
>   https://patchwork.ozlabs.org/project/openvswitch/patch/20230425063238.2366798-5-roid@nvidia.com/

It seems to me that the agreement in the thread at the link above was to
create a function, nl_msg_start_nested_with_flag(), that would only set the
flag when appropriate. Which I think was the TC use-case.

Did the conversation move on from there?
Or was it an oversight not to do that in v2?
Or am I completely off track?
Ilya Maximets May 10, 2023, 2:28 p.m. UTC | #3 
On 5/10/23 16:10, Simon Horman wrote:
> On Wed, Apr 26, 2023 at 06:39:22PM +0200, Ilya Maximets wrote:
>> On 4/25/23 14:41, Roi Dayan wrote:
>>> From: Gavin Li <gavinl@nvidia.com>
>>>
>>> Linux kernel netlink module added NLA_F_NESTED flag checking for nested
>>> netlink messages in 5.2. A nested message without the flag set will be
>>> treated as malformated one. The check is optional and is controlled by
>>> message policy. To avoid this, add NLA_F_NESTED explicitly for all
>>> nested netlink messages.
>>>
>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>>> ---
>>>  lib/netlink.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netlink.c b/lib/netlink.c
>>> index 6215282d6fbe..f128b63074f9 100644
>>> --- a/lib/netlink.c
>>> +++ b/lib/netlink.c
>>> @@ -519,7 +519,7 @@ size_t
>>>  nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
>>>  {
>>>      size_t offset = msg->size;
>>> -    nl_msg_put_unspec_uninit(msg, type, 0);
>>> +    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
>>>      return offset;
>>>  }
>>>  
>>
>>
>> Just posting a link to the ongoing v1 conversation on this patch here
>> for visibility:
>>   https://patchwork.ozlabs.org/project/openvswitch/patch/20230425063238.2366798-5-roid@nvidia.com/
> 
> It seems to me that the agreement in the thread at the link above was to
> create a function, nl_msg_start_nested_with_flag(), that would only set the
> flag when appropriate. Which I think was the TC use-case.
> 
> Did the conversation move on from there?
> Or was it an oversight not to do that in v2?
> Or am I completely off track?

v2 was posted before the conversation on v1 converged.
I think we have an agreement, but it might make sense to review (at least
visually) some other aspects of the patch set before posting v3.

Best regards, Ilya Maximets.
Simon Horman May 10, 2023, 2:42 p.m. UTC | #4 
On Wed, May 10, 2023 at 04:28:06PM +0200, Ilya Maximets wrote:
> On 5/10/23 16:10, Simon Horman wrote:
> > On Wed, Apr 26, 2023 at 06:39:22PM +0200, Ilya Maximets wrote:
> >> On 4/25/23 14:41, Roi Dayan wrote:
> >>> From: Gavin Li <gavinl@nvidia.com>
> >>>
> >>> Linux kernel netlink module added NLA_F_NESTED flag checking for nested
> >>> netlink messages in 5.2. A nested message without the flag set will be
> >>> treated as malformated one. The check is optional and is controlled by
> >>> message policy. To avoid this, add NLA_F_NESTED explicitly for all
> >>> nested netlink messages.
> >>>
> >>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
> >>> Reviewed-by: Roi Dayan <roid@nvidia.com>
> >>> ---
> >>>  lib/netlink.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/netlink.c b/lib/netlink.c
> >>> index 6215282d6fbe..f128b63074f9 100644
> >>> --- a/lib/netlink.c
> >>> +++ b/lib/netlink.c
> >>> @@ -519,7 +519,7 @@ size_t
> >>>  nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
> >>>  {
> >>>      size_t offset = msg->size;
> >>> -    nl_msg_put_unspec_uninit(msg, type, 0);
> >>> +    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
> >>>      return offset;
> >>>  }
> >>>  
> >>
> >>
> >> Just posting a link to the ongoing v1 conversation on this patch here
> >> for visibility:
> >>   https://patchwork.ozlabs.org/project/openvswitch/patch/20230425063238.2366798-5-roid@nvidia.com/
> > 
> > It seems to me that the agreement in the thread at the link above was to
> > create a function, nl_msg_start_nested_with_flag(), that would only set the
> > flag when appropriate. Which I think was the TC use-case.
> > 
> > Did the conversation move on from there?
> > Or was it an oversight not to do that in v2?
> > Or am I completely off track?
> 
> v2 was posted before the conversation on v1 converged.
> I think we have an agreement, but it might make sense to review (at least
> visually) some other aspects of the patch set before posting v3.

Understood. I will try to take a look over v2 with this in mind.
Roi Dayan May 11, 2023, 12:43 p.m. UTC | #5 
On 10/05/2023 17:10, Simon Horman wrote:
> On Wed, Apr 26, 2023 at 06:39:22PM +0200, Ilya Maximets wrote:
>> On 4/25/23 14:41, Roi Dayan wrote:
>>> From: Gavin Li <gavinl@nvidia.com>
>>>
>>> Linux kernel netlink module added NLA_F_NESTED flag checking for nested
>>> netlink messages in 5.2. A nested message without the flag set will be
>>> treated as malformated one. The check is optional and is controlled by
>>> message policy. To avoid this, add NLA_F_NESTED explicitly for all
>>> nested netlink messages.
>>>
>>> Signed-off-by: Gavin Li <gavinl@nvidia.com>
>>> Reviewed-by: Roi Dayan <roid@nvidia.com>
>>> ---
>>>  lib/netlink.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/netlink.c b/lib/netlink.c
>>> index 6215282d6fbe..f128b63074f9 100644
>>> --- a/lib/netlink.c
>>> +++ b/lib/netlink.c
>>> @@ -519,7 +519,7 @@ size_t
>>>  nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
>>>  {
>>>      size_t offset = msg->size;
>>> -    nl_msg_put_unspec_uninit(msg, type, 0);
>>> +    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
>>>      return offset;
>>>  }
>>>  
>>
>>
>> Just posting a link to the ongoing v1 conversation on this patch here
>> for visibility:
>>   https://patchwork.ozlabs.org/project/openvswitch/patch/20230425063238.2366798-5-roid@nvidia.com/
> 
> It seems to me that the agreement in the thread at the link above was to
> create a function, nl_msg_start_nested_with_flag(), that would only set the
> flag when appropriate. Which I think was the TC use-case.
> 
> Did the conversation move on from there?
> Or was it an oversight not to do that in v2?
> Or am I completely off track?

Hi Simon,

Correct. we will add nl_msg_start_nested_with_flag() and use it
currently only for TC vxlan gbp.

Thanks,
Roi
diff mbox series

Patch

diff --git a/lib/netlink.c b/lib/netlink.c
index 6215282d6fbe..f128b63074f9 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -519,7 +519,7 @@  size_t
 nl_msg_start_nested(struct ofpbuf *msg, uint16_t type)
 {
     size_t offset = msg->size;
-    nl_msg_put_unspec_uninit(msg, type, 0);
+    nl_msg_put_unspec_uninit(msg, type | NLA_F_NESTED, 0);
     return offset;
 }