Message ID | 20230425124122.2443376-5-roid@nvidia.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Add vxlan gbp offload with TC | expand |
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 |
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.
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?
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.
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.
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 --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; }