diff mbox series

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

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

Commit Message

Kaixi Fan Sept. 11, 2020, 4:05 a.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.

Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
---
 ofproto/ofproto-dpif.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Aaron Conole Sept. 23, 2020, 2:51 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.
>
> Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
> ---

This is effectively trying to revert:

d0d57149 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")

Did I understand it correctly?

>  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;
> -            }
>          }
>      }
Flavio Leitner Sept. 23, 2020, 8:08 p.m. UTC | #2 
On Wed, Sep 23, 2020 at 10:51:51AM -0400, Aaron Conole wrote:
> 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.
> >
> > Signed-off-by: fankaixi.li <fankaixi.li@bytedance.com>
> > ---
> 
> This is effectively trying to revert:
> 
> d0d57149 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported")
> 
> Did I understand it correctly?

The rule is constructed in userspace and depending on the mask
it may or may not be slow path'ed. In the case where it is
pushed to kernel DP, then OVS_KEY_ATTR_ND_EXTENSIONS is only
defined in userspace, leading the kernel to show:

 openvswitch: netlink: Key type X is out of range max Y

I don't see how the patch below prevents that to happen.

Thanks,
fbl

> >  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;
> > -            }
> >          }
> >      }
>
Kaixi Fan Sept. 24, 2020, 9:19 a.m. UTC | #3 
Hi Aaron conole and Flavio Leitner, I am just trying to fix the bug. When I
am trying to put ipv6 nd proxy flows into ovs (with kernel), ovs emits
error message like this:
"Only userspace datapath support OVS_KEY_ATTR_ND_EXTENSIONS in keys."
So the flows could not be inserted into ovs by ovs-ofctl. The flows
contains actions to modify ipv6 nd_reserved and nd_options_type fields, in
order to change a Neighbor Solicitation packet to be a Neighbor
Advertisement packet.
Ovs kernel datapath always upcall this type of packets to userspace. The
userspace datapath would handle these packets and send them back to the
kernel datapath. Also megaflows with "userspace" action would be inserted
into kernel datapath.

于2020年09月23日星期三 22:52 <aconole@redhat.com> 写道:

fankaixi.li@bytedance.com writes: > From: "fankaixi.li" > > 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. > >
Signed-off-by: fankaixi.li > --- This is effectively trying to revert:
d0d57149 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported") Did I
understand it correctly? > 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; > - } > } > }
Flavio Leitner Sept. 24, 2020, 9:31 p.m. UTC | #4 
Hi,

Right, it will be forced to slow path if the action actually changes
the value otherwise just omitted.

The problem is with the matching side where an error will happen
if the data path doesn't support nd extension.

I will continue to review the patch and follow up later, but in
the meantime I think the patch should include the Fixes: tag to
the commit pointed by Aaron. It would be great if it includes a
new test.

Thanks,
fbl

On Thu, Sep 24, 2020 at 09:19:53AM +0000, 范开喜 wrote:
> Hi Aaron conole and Flavio Leitner, I am just trying to fix the bug. When I
> am trying to put ipv6 nd proxy flows into ovs (with kernel), ovs emits
> error message like this:
> "Only userspace datapath support OVS_KEY_ATTR_ND_EXTENSIONS in keys."
> So the flows could not be inserted into ovs by ovs-ofctl. The flows
> contains actions to modify ipv6 nd_reserved and nd_options_type fields, in
> order to change a Neighbor Solicitation packet to be a Neighbor
> Advertisement packet.
> Ovs kernel datapath always upcall this type of packets to userspace. The
> userspace datapath would handle these packets and send them back to the
> kernel datapath. Also megaflows with "userspace" action would be inserted
> into kernel datapath.
> 
> 于2020年09月23日星期三 22:52 <aconole@redhat.com> 写道:
> 
> fankaixi.li@bytedance.com writes: > From: "fankaixi.li" > > 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. > >
> Signed-off-by: fankaixi.li > --- This is effectively trying to revert:
> d0d57149 ("ofproto-dpif: Allow IPv6 ND Extensions only if supported") Did I
> understand it correctly? > 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; > - } > } > }
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;
-            }
         }
     }