diff mbox series

[ovs-dev] ofproto-dpif: remove checking setting nd_ext field

Message ID 20200928134947.48269-1-fankaixi.li@bytedance.com
State Deferred
Headers show
Series [ovs-dev] ofproto-dpif: remove checking setting nd_ext field | expand

Commit Message

Kaixi Fan Sept. 28, 2020, 1:49 p.m. UTC 
From: "fankaixi.li" <fankaixi.li@bytedance.com>

In order to support openflow rule which setting nd_ext fields in openflow tables,
we should remove setting nd_ext fields when constructing rule. The ofproto would
translate it into userspace actions when handling upcalls.

When trying to add the following flows to ovs with kernel:
# flow 1
ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x4000000, table=0,
priority=50,icmp6,icmpv6_type=135,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,nd_sll=fa:16:3e:55:ad:df,actions=set_field:136->icmpv6_type,set_field:0->icmpv6_code,set_field:2->nd_options_type,goto_table:1"
# flow 2
ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x12220d57,table=1,priority=80,icmp6,icmpv6_type=136,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,actions= move:eth_src[]->eth_dst[],set_field:00:23:15:d3:22:01->eth_src,move:ipv6_src[]->ipv6_dst[],set_field:2001:db8:0:2:0:0:0:1->ipv6_src,set_field:00:23:15:d3:22:01->nd_tll,set_field:0xe00->nd_reserved,set_field:2->nd_options_type,output:3"

Ovs would emit a error message:
OFPT_ERROR (OF1.3) (xid=0x6): OFPBAC_BAD_SET_ARGUMENT

The output of log file shows a error message:
2020-09-28T12:22:09.453Z|00036|ofproto_dpif|WARN|Rejecting set field action because datapath does not support setting IPv6 ND Extensions fields (your kernel module may be out of date)
2020-09-28T12:22:09.453Z|00037|connmgr|INFO|br0<->unix#14: sending OFPBAC_BAD_SET_ARGUMENT error reply to OFPT_FLOW_MOD message

This patch would fix this error.

Also following Flavio Leitner's advise, I have prepared a testcase for this patch. 
But after running auto-testcases, I found that the test framework ovs support flows with actions to set nd_reserved field. 
The testsuites.log shows table 0 supporting setting nd_reserved and nd_options_type fields, and it shows that ovs was configured at dummy mode. 
I think there are some differences bewteen these two ovs modes. So I decide not commit the testcase about this fix. 

Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
CC: Flavio Leitner <fbl@sysclose.org>
Fixes: d0d571493cf8 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")
---
 ofproto/ofproto-dpif.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Aaron Conole Sept. 28, 2020, 3:39 p.m. UTC | #1 
fankaixi.li@bytedance.com writes:

> From: "fankaixi.li" <fankaixi.li@bytedance.com>
>
> In order to support openflow rule which setting nd_ext fields in openflow tables,
> we should remove setting nd_ext fields when constructing rule. The ofproto would
> translate it into userspace actions when handling upcalls.
>
> When trying to add the following flows to ovs with kernel:
> # flow 1
> ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x4000000, table=0,
> priority=50,icmp6,icmpv6_type=135,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,nd_sll=fa:16:3e:55:ad:df,actions=set_field:136->icmpv6_type,set_field:0->icmpv6_code,set_field:2->nd_options_type,goto_table:1"
> # flow 2
> ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x12220d57,table=1,priority=80,icmp6,icmpv6_type=136,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,actions= move:eth_src[]->eth_dst[],set_field:00:23:15:d3:22:01->eth_src,move:ipv6_src[]->ipv6_dst[],set_field:2001:db8:0:2:0:0:0:1->ipv6_src,set_field:00:23:15:d3:22:01->nd_tll,set_field:0xe00->nd_reserved,set_field:2->nd_options_type,output:3"
>
> Ovs would emit a error message:
> OFPT_ERROR (OF1.3) (xid=0x6): OFPBAC_BAD_SET_ARGUMENT
>
> The output of log file shows a error message:
> 2020-09-28T12:22:09.453Z|00036|ofproto_dpif|WARN|Rejecting set field action because datapath does not support setting IPv6 ND Extensions fields (your kernel module may be out of date)
> 2020-09-28T12:22:09.453Z|00037|connmgr|INFO|br0<->unix#14: sending OFPBAC_BAD_SET_ARGUMENT error reply to OFPT_FLOW_MOD message
>
> This patch would fix this error.
>
> Also following Flavio Leitner's advise, I have prepared a testcase for this patch. 
> But after running auto-testcases, I found that the test framework ovs support flows with actions to set nd_reserved field. 
> The testsuites.log shows table 0 supporting setting nd_reserved and nd_options_type fields, and it shows that ovs was configured at dummy mode. 
> I think there are some differences bewteen these two ovs modes. So I decide not commit the testcase about this fix. 

Please submit the test case so that we can look it over.  Flag it as RFC
and maybe we can figure out how to adjust it so it tests what you're
hoping.

> Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
> CC: Flavio Leitner <fbl@sysclose.org>
> Fixes: d0d571493cf8 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")
> ---
>  ofproto/ofproto-dpif.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 4f0638f23..f4c37f43d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4637,14 +4637,6 @@ check_actions(const struct ofproto_dpif *ofproto,
>                                         "ct original direction tuple");
>                  return OFPERR_NXBAC_CT_DATAPATH_SUPPORT;
>              }
> -        } else if (!support->nd_ext && ofpact->type == OFPACT_SET_FIELD) {
> -            const struct mf_field *dst = ofpact_get_mf_dst(ofpact);
> -
> -            if (dst->id == MFF_ND_RESERVED || dst->id == MFF_ND_OPTIONS_TYPE) {
> -                report_unsupported_act("set field",
> -                                       "setting IPv6 ND Extensions fields");
> -                return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> -            }
>          }
>      }
Simon Horman Oct. 3, 2023, 8:50 a.m. UTC | #2 
On Mon, Sep 28, 2020 at 09:49:47PM +0800, fankaixi.li@bytedance.com wrote:
> From: "fankaixi.li" <fankaixi.li@bytedance.com>
> 
> In order to support openflow rule which setting nd_ext fields in openflow tables,
> we should remove setting nd_ext fields when constructing rule. The ofproto would
> translate it into userspace actions when handling upcalls.
> 
> When trying to add the following flows to ovs with kernel:
> # flow 1
> ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x4000000, table=0,
> priority=50,icmp6,icmpv6_type=135,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,nd_sll=fa:16:3e:55:ad:df,actions=set_field:136->icmpv6_type,set_field:0->icmpv6_code,set_field:2->nd_options_type,goto_table:1"
> # flow 2
> ovs-ofctl -Oopenflow13 add-flow br0 "cookie=0x12220d57,table=1,priority=80,icmp6,icmpv6_type=136,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,actions= move:eth_src[]->eth_dst[],set_field:00:23:15:d3:22:01->eth_src,move:ipv6_src[]->ipv6_dst[],set_field:2001:db8:0:2:0:0:0:1->ipv6_src,set_field:00:23:15:d3:22:01->nd_tll,set_field:0xe00->nd_reserved,set_field:2->nd_options_type,output:3"
> 
> Ovs would emit a error message:
> OFPT_ERROR (OF1.3) (xid=0x6): OFPBAC_BAD_SET_ARGUMENT
> 
> The output of log file shows a error message:
> 2020-09-28T12:22:09.453Z|00036|ofproto_dpif|WARN|Rejecting set field action because datapath does not support setting IPv6 ND Extensions fields (your kernel module may be out of date)
> 2020-09-28T12:22:09.453Z|00037|connmgr|INFO|br0<->unix#14: sending OFPBAC_BAD_SET_ARGUMENT error reply to OFPT_FLOW_MOD message
> 
> This patch would fix this error.
> 
> Also following Flavio Leitner's advise, I have prepared a testcase for this patch. 
> But after running auto-testcases, I found that the test framework ovs support flows with actions to set nd_reserved field. 
> The testsuites.log shows table 0 supporting setting nd_reserved and nd_options_type fields, and it shows that ovs was configured at dummy mode. 
> I think there are some differences bewteen these two ovs modes. So I decide not commit the testcase about this fix. 
> 
> Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
> CC: Flavio Leitner <fbl@sysclose.org>
> Fixes: d0d571493cf8 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")

Hi Fankaixi Li,

This patch appears to have gone stale in patchwork, for one reason or
another. If it is still relevant then I think it needs to be revisited,
by being reposted after appropriate preparation.

As such I'm marking this patch as "Deferred" in patchwork.

No action is required unless there is a desire to revisit this patch.
Kaixi Fan Oct. 24, 2023, 2:22 a.m. UTC | #3 
Hi Simon Horman,

Thanks. If it's ready, I will repost again.

From: "Simon Horman"<horms@ovn.org>
Date: Tue, Oct 3, 2023, 16:50
Subject: [External] Re: [ovs-dev] [PATCH] ofproto-dpif: remove checking
setting nd_ext field
To: <fankaixi.li@bytedance.com>
Cc: <dev@openvswitch.org>, <fbl@redhat.com>, "Flavio Leitner"<
fbl@sysclose.org>
On Mon, Sep 28, 2020 at 09:49:47PM +0800, fankaixi.li@bytedance.com wrote:
> From: "fankaixi.li" <fankaixi.li@bytedance.com> > > In order to support
openflow rule which setting nd_ext fields in openflow tables, > we should
remove setting nd_ext fields when constructing rule. The ofproto would >
translate it into userspace actions when handling upcalls. > > When trying
to add the following flows to ovs with kernel: > # flow 1 > ovs-ofctl
-Oopenflow13 add-flow br0 "cookie=0x4000000, table=0, >
priority=50,icmp6,icmpv6_type=135,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,nd_sll=fa:16:3e:55:ad:df,actions=set_field:136->icmpv6_type,set_field:0->icmpv6_code,set_field:2->nd_options_type,goto_table:1"
> # flow 2 > ovs-ofctl -Oopenflow13 add-flow br0
"cookie=0x12220d57,table=1,priority=80,icmp6,icmpv6_type=136,icmpv6_code=0,nd_target=2001:db8:0:2:0:0:0:1,actions=
move:eth_src[]->eth_dst[],set_field:00:23:15:d3:22:01->eth_src,move:ipv6_src[]->ipv6_dst[],set_field:2001:db8:0:2:0:0:0:1->ipv6_src,set_field:00:23:15:d3:22:01->nd_tll,set_field:0xe00->nd_reserved,set_field:2->nd_options_type,output:3"
> > Ovs would emit a error message: > OFPT_ERROR (OF1.3) (xid=0x6):
OFPBAC_BAD_SET_ARGUMENT > > The output of log file shows a error message: >
2020-09-28T12:22:09.453Z|00036|ofproto_dpif|WARN|Rejecting set field action
because datapath does not support setting IPv6 ND Extensions fields (your
kernel module may be out of date) >
2020-09-28T12:22:09.453Z|00037|connmgr|INFO|br0<->unix#14: sending
OFPBAC_BAD_SET_ARGUMENT error reply to OFPT_FLOW_MOD message > > This patch
would fix this error. > > Also following Flavio Leitner's advise, I have
prepared a testcase for this patch. > But after running auto-testcases, I
found that the test framework ovs support flows with actions to set
nd_reserved field. > The testsuites.log shows table 0 supporting setting
nd_reserved and nd_options_type fields, and it shows that ovs was
configured at dummy mode. > I think there are some differences bewteen
these two ovs modes. So I decide not commit the testcase about this fix. >
> Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com> > CC: Flavio
Leitner <fbl@sysclose.org> > Fixes: d0d571493cf8 ("ofproto-dpif: Allow IPv6
ND Extensions only if supported") Hi Fankaixi Li, This patch appears to
have gone stale in patchwork, for one reason or another. If it is still
relevant then I think it needs to be revisited, by being reposted after
appropriate preparation. As such I'm marking this patch as "Deferred" in
patchwork. No action is required unless there is a desire to revisit this
patch.
Simon Horman Oct. 30, 2023, 10:19 a.m. UTC | #4 
On Mon, Oct 23, 2023 at 07:22:08PM -0700, Kaixi Fan wrote:
> Hi Simon Horman,
> 
> Thanks. If it's ready, I will repost again.

Thanks, much appreciated.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4f0638f23..f4c37f43d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4637,14 +4637,6 @@  check_actions(const struct ofproto_dpif *ofproto,
                                        "ct original direction tuple");
                 return OFPERR_NXBAC_CT_DATAPATH_SUPPORT;
             }
-        } else if (!support->nd_ext && ofpact->type == OFPACT_SET_FIELD) {
-            const struct mf_field *dst = ofpact_get_mf_dst(ofpact);
-
-            if (dst->id == MFF_ND_RESERVED || dst->id == MFF_ND_OPTIONS_TYPE) {
-                report_unsupported_act("set field",
-                                       "setting IPv6 ND Extensions fields");
-                return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
-            }
         }
     }