diff mbox series

[ovs-dev] ovn: Add a new action 'nd_na_router' to handle NS requests for router IPs

Message ID 20180507183605.11612-1-nusiddiq@redhat.com
State Superseded
Headers show
Series [ovs-dev] ovn: Add a new action 'nd_na_router' to handle NS requests for router IPs | expand

Commit Message

Numan Siddique May 7, 2018, 6:36 p.m. UTC
From: Numan Siddique <nusiddiq@redhat.com>

Presently when a VM's IPv6 stack sends a Neighbor Solicitation request for its
router IP, (mostly when the ND cache entry for the router is in STALE state)
ovn-controller responds with a Neighbor Adv packet (using the action nd_na).
But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because of which, the
VM deletes the default route. The default route gets added again when the next
RA is received (but would again gets deleted if its sends NS request). And this
results in disruption of IPv6 traffic.

This patch addresses this issue by adding a new action 'nd_na_router' which is
same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO flags. ovn-northd
uses this action. A new action is added instead of modifying the existing 'nd_na'
action. This is because
  - We cannot set the RSO flags in the "nd_na { ..actions .. }"
  - It would be ugly to have something like nd_na { router_flags, ...actions .. }

(Please note: There are 3 'Line length is >79-characters' warnings in ovn.at which
I couldn't resolve.)

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
CC: Justin Pettit <jpettit@ovn.org>
CC: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 include/ovn/actions.h     |  7 +++++++
 ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
 ovn/lib/actions.c         | 22 ++++++++++++++++++++++
 ovn/northd/ovn-northd.c   |  2 +-
 ovn/utilities/ovn-trace.c |  1 +
 tests/ovn.at              |  5 +++++
 6 files changed, 51 insertions(+), 9 deletions(-)

Comments

Mark Michelson May 7, 2018, 7:15 p.m. UTC | #1
Hi Numan, thanks for the fix.

Out of curiosity, why did you add a new action instead of adding a new 
logical field (something like nd.rso)?

On 05/07/2018 02:36 PM, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@redhat.com>
> 
> Presently when a VM's IPv6 stack sends a Neighbor Solicitation request for its
> router IP, (mostly when the ND cache entry for the router is in STALE state)
> ovn-controller responds with a Neighbor Adv packet (using the action nd_na).
> But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because of which, the
> VM deletes the default route. The default route gets added again when the next
> RA is received (but would again gets deleted if its sends NS request). And this
> results in disruption of IPv6 traffic.
> 
> This patch addresses this issue by adding a new action 'nd_na_router' which is
> same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO flags. ovn-northd
> uses this action. A new action is added instead of modifying the existing 'nd_na'
> action. This is because
>    - We cannot set the RSO flags in the "nd_na { ..actions .. }"
>    - It would be ugly to have something like nd_na { router_flags, ...actions .. }
> 
> (Please note: There are 3 'Line length is >79-characters' warnings in ovn.at which
> I couldn't resolve.)
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
> CC: Justin Pettit <jpettit@ovn.org>
> CC: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---
>   include/ovn/actions.h     |  7 +++++++
>   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
>   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
>   ovn/northd/ovn-northd.c   |  2 +-
>   ovn/utilities/ovn-trace.c |  1 +
>   tests/ovn.at              |  5 +++++
>   6 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index fb8f51509..638465193 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -68,6 +68,7 @@ struct ovn_extend_table;
>       OVNACT(ICMP6,             ovnact_nest)            \
>       OVNACT(TCP_RESET,         ovnact_nest)            \
>       OVNACT(ND_NA,             ovnact_nest)            \
> +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
>       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
>       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
>       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
> @@ -444,6 +445,12 @@ enum action_opcode {
>        * The actions, in OpenFlow 1.3 format, follow the action_header.
>        */
>       ACTION_OPCODE_TCP_RESET,
> +
> +    /* "nd_na_router { ...actions... }" with rso flag 'ND_RSO_ROUTER' set.
> +        *
> +        * The actions, in OpenFlow 1.3 format, follow the action_header.
> +        */
> +    ACTION_OPCODE_ND_NA_ROUTER,
>   };
>   
>   /* Header. */
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 6e6aa1caa..305f20649 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -81,7 +81,8 @@ static void send_garp_run(struct controller_ctx *ctx,
>                             struct sset *active_tunnels);
>   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
>                                    const struct match *md,
> -                                 struct ofpbuf *userdata);
> +                                 struct ofpbuf *userdata,
> +                                 bool is_router);
>   static void reload_metadata(struct ofpbuf *ofpacts,
>                               const struct match *md);
>   static void pinctrl_handle_put_nd_ra_opts(
> @@ -1154,7 +1155,11 @@ process_packet_in(const struct ofp_header *msg, struct controller_ctx *ctx)
>           break;
>   
>       case ACTION_OPCODE_ND_NA:
> -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
> +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata, false);
> +        break;
> +
> +    case ACTION_OPCODE_ND_NA_ROUTER:
> +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata, true);
>           break;
>   
>       case ACTION_OPCODE_PUT_ND:
> @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts, const struct match *md)
>   
>   static void
>   pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match *md,
> -                     struct ofpbuf *userdata)
> +                     struct ofpbuf *userdata, bool is_router)
>   {
>       /* This action only works for IPv6 ND packets, and the switch should only
>        * send us ND packets this way, but check here just to be sure. */
> @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match *md,
>       struct dp_packet packet;
>       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>   
> -    /* xxx These flags are not exactly correct.  Look at section 7.2.4
> -     * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
> -     * xxx router's interfaces and ND_RSO_SOLICITED only if it was
> -     * xxx requested. */
> +    /* These flags are not exactly correct.  Look at section 7.2.4
> +     * of RFC 4861. */
> +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
> +    if (is_router) {
> +        rso_flags |= ND_RSO_ROUTER;
> +    }
>       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
>                     &ip_flow->nd_target, &ip_flow->ipv6_src,
> -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
> +                  htonl(rso_flags));
>   
>       /* Reload previous packet metadata and set actions from userdata. */
>       set_actions_and_enqueue_msg(&packet, md, userdata);
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index a6945812d..0669cc1c9 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
>       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
>   }
>   
> +static void
> +parse_ND_NA_ROUTER(struct action_context *ctx)
> +{
> +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
> +}
> +
>   static void
>   parse_ND_NS(struct action_context *ctx)
>   {
> @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest *nest, struct ds *s)
>       format_nested_action(nest, "nd_na", s);
>   }
>   
> +static void
> +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct ds *s)
> +{
> +    format_nested_action(nest, "nd_na_router", s);
> +}
> +
>   static void
>   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
>   {
> @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
>       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, ofpacts);
>   }
>   
> +static void
> +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
> +             const struct ovnact_encode_params *ep,
> +             struct ofpbuf *ofpacts)
> +{
> +    encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
> +}
> +
>   static void
>   encode_ND_NS(const struct ovnact_nest *on,
>                const struct ovnact_encode_params *ep,
> @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
>           parse_TCP_RESET(ctx);
>       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
>           parse_ND_NA(ctx);
> +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
> +        parse_ND_NA_ROUTER(ctx);
>       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
>           parse_ND_NS(ctx);
>       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ce472a536..b157cd1eb 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>               ds_clear(&actions);
>               ds_put_format(&actions,
>                             "put_nd(inport, ip6.src, nd.sll); "
> -                          "nd_na { "
> +                          "nd_na_router { "
>                             "eth.src = %s; "
>                             "ip6.src = %s; "
>                             "nd.target = %s; "
> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> index 9c19b5b9a..1fd48f22e 100644
> --- a/ovn/utilities/ovn-trace.c
> +++ b/ovn/utilities/ovn-trace.c
> @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>               break;
>   
>           case OVNACT_ND_NA:
> +        case OVNACT_ND_NA_ROUTER:
>               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, table_id, pipeline,
>                             super);
>               break;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4a5316510..2c0ae9877 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inpor
>       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>       encodes as controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>       has prereqs nd_ns
> +# nd_na_router
> +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out inport. */ output; };
> +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
> +    encodes as controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> +    has prereqs nd_ns
>   
>   # get_nd
>   get_nd(outport, ip6.dst);
>
Miguel Angel Ajo May 8, 2018, 7:50 a.m. UTC | #2
Thank you Numan!

It took me a bit to find what the "RSO"  flag was, because they are R, S
and O, may be we can change that in the commit, or reference the
RFC/section  (RFC4861 page 23).

     R              Router flag.  When set, the R-bit indicates that
                     the sender is a router.  The R-bit is used by
                     Neighbor Unreachability Detection to detect a
                     router that changes to a host.

      S              Solicited flag.  When set, the S-bit indicates that
                     the advertisement was sent in response to a
                     Neighbor Solicitation from the Destination address.
                     The S-bit is used as a reachability confirmation
                     for Neighbor Unreachability Detection.  It MUST NOT
                     be set in multicast advertisements or in
                     unsolicited unicast advertisements.

      O              Override flag.  When set, the O-bit indicates that
                     the advertisement should override an existing cache
                     entry and update the cached link-layer address.
                     When it is not set the advertisement will not
                     update a cached link-layer address though it will
                     update an existing Neighbor Cache entry for which
                     no link-layer address is known.  It SHOULD NOT be
                     set in solicited advertisements for anycast
                     addresses and in solicited proxy advertisements.
                     It SHOULD be set in other solicited advertisements
                     and in unsolicited advertisements.




And same question Mark did.

Thanks for handling this, good work :)

On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels@redhat.com> wrote:

> Hi Numan, thanks for the fix.
>
> Out of curiosity, why did you add a new action instead of adding a new
> logical field (something like nd.rso)?
>
> On 05/07/2018 02:36 PM, nusiddiq@redhat.com wrote:
> > From: Numan Siddique <nusiddiq@redhat.com>
> >
> > Presently when a VM's IPv6 stack sends a Neighbor Solicitation request
> for its
> > router IP, (mostly when the ND cache entry for the router is in STALE
> state)
> > ovn-controller responds with a Neighbor Adv packet (using the action
> nd_na).
> > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because of which,
> the
> > VM deletes the default route. The default route gets added again when
> the next
> > RA is received (but would again gets deleted if its sends NS request).
> And this
> > results in disruption of IPv6 traffic.
> >
> > This patch addresses this issue by adding a new action 'nd_na_router'
> which is
> > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO flags.
> ovn-northd
> > uses this action. A new action is added instead of modifying the
> existing 'nd_na'
> > action. This is because
> >    - We cannot set the RSO flags in the "nd_na { ..actions .. }"
> >    - It would be ugly to have something like nd_na { router_flags,
> ...actions .. }
> >
> > (Please note: There are 3 'Line length is >79-characters' warnings in
> ovn.at which
> > I couldn't resolve.)
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
> > CC: Justin Pettit <jpettit@ovn.org>
> > CC: Mark Michelson <mmichels@redhat.com>
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
> >   include/ovn/actions.h     |  7 +++++++
> >   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
> >   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
> >   ovn/northd/ovn-northd.c   |  2 +-
> >   ovn/utilities/ovn-trace.c |  1 +
> >   tests/ovn.at              |  5 +++++
> >   6 files changed, 51 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index fb8f51509..638465193 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -68,6 +68,7 @@ struct ovn_extend_table;
> >       OVNACT(ICMP6,             ovnact_nest)            \
> >       OVNACT(TCP_RESET,         ovnact_nest)            \
> >       OVNACT(ND_NA,             ovnact_nest)            \
> > +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
> >       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
> >       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
> >       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
> > @@ -444,6 +445,12 @@ enum action_opcode {
> >        * The actions, in OpenFlow 1.3 format, follow the action_header.
> >        */
> >       ACTION_OPCODE_TCP_RESET,
> > +
> > +    /* "nd_na_router { ...actions... }" with rso flag 'ND_RSO_ROUTER'
> set.
> > +        *
> > +        * The actions, in OpenFlow 1.3 format, follow the action_header.
> > +        */
> > +    ACTION_OPCODE_ND_NA_ROUTER,
> >   };
> >
> >   /* Header. */
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 6e6aa1caa..305f20649 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -81,7 +81,8 @@ static void send_garp_run(struct controller_ctx *ctx,
> >                             struct sset *active_tunnels);
> >   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
> >                                    const struct match *md,
> > -                                 struct ofpbuf *userdata);
> > +                                 struct ofpbuf *userdata,
> > +                                 bool is_router);
> >   static void reload_metadata(struct ofpbuf *ofpacts,
> >                               const struct match *md);
> >   static void pinctrl_handle_put_nd_ra_opts(
> > @@ -1154,7 +1155,11 @@ process_packet_in(const struct ofp_header *msg,
> struct controller_ctx *ctx)
> >           break;
> >
> >       case ACTION_OPCODE_ND_NA:
> > -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
> false);
> > +        break;
> > +
> > +    case ACTION_OPCODE_ND_NA_ROUTER:
> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
> true);
> >           break;
> >
> >       case ACTION_OPCODE_PUT_ND:
> > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts, const
> struct match *md)
> >
> >   static void
> >   pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match
> *md,
> > -                     struct ofpbuf *userdata)
> > +                     struct ofpbuf *userdata, bool is_router)
> >   {
> >       /* This action only works for IPv6 ND packets, and the switch
> should only
> >        * send us ND packets this way, but check here just to be sure. */
> > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct flow *ip_flow,
> const struct match *md,
> >       struct dp_packet packet;
> >       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> >
> > -    /* xxx These flags are not exactly correct.  Look at section 7.2.4
> > -     * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
> > -     * xxx router's interfaces and ND_RSO_SOLICITED only if it was
> > -     * xxx requested. */
> > +    /* These flags are not exactly correct.  Look at section 7.2.4
> > +     * of RFC 4861. */
> > +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
> > +    if (is_router) {
> > +        rso_flags |= ND_RSO_ROUTER;
> > +    }
> >       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
> >                     &ip_flow->nd_target, &ip_flow->ipv6_src,
> > -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
> > +                  htonl(rso_flags));
> >
> >       /* Reload previous packet metadata and set actions from userdata.
> */
> >       set_actions_and_enqueue_msg(&packet, md, userdata);
> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> > index a6945812d..0669cc1c9 100644
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
> >       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
> >   }
> >
> > +static void
> > +parse_ND_NA_ROUTER(struct action_context *ctx)
> > +{
> > +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
> > +}
> > +
> >   static void
> >   parse_ND_NS(struct action_context *ctx)
> >   {
> > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest *nest,
> struct ds *s)
> >       format_nested_action(nest, "nd_na", s);
> >   }
> >
> > +static void
> > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct ds *s)
> > +{
> > +    format_nested_action(nest, "nd_na_router", s);
> > +}
> > +
> >   static void
> >   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
> >   {
> > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
> >       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, ofpacts);
> >   }
> >
> > +static void
> > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
> > +             const struct ovnact_encode_params *ep,
> > +             struct ofpbuf *ofpacts)
> > +{
> > +    encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
> > +}
> > +
> >   static void
> >   encode_ND_NS(const struct ovnact_nest *on,
> >                const struct ovnact_encode_params *ep,
> > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
> >           parse_TCP_RESET(ctx);
> >       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
> >           parse_ND_NA(ctx);
> > +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
> > +        parse_ND_NA_ROUTER(ctx);
> >       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
> >           parse_ND_NS(ctx);
> >       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index ce472a536..b157cd1eb 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> >               ds_clear(&actions);
> >               ds_put_format(&actions,
> >                             "put_nd(inport, ip6.src, nd.sll); "
> > -                          "nd_na { "
> > +                          "nd_na_router { "
> >                             "eth.src = %s; "
> >                             "ip6.src = %s; "
> >                             "nd.target = %s; "
> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> > index 9c19b5b9a..1fd48f22e 100644
> > --- a/ovn/utilities/ovn-trace.c
> > +++ b/ovn/utilities/ovn-trace.c
> > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact *ovnacts, size_t
> ovnacts_len,
> >               break;
> >
> >           case OVNACT_ND_NA:
> > +        case OVNACT_ND_NA_ROUTER:
> >               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, table_id,
> pipeline,
> >                             super);
> >               break;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 4a5316510..2c0ae9877 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
> 12:34:56:78:9a:bc; outport = inpor
> >       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
> >       encodes as
> controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> >       has prereqs nd_ns
> > +# nd_na_router
> > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc;
> outport = inport; inport = ""; /* Allow sending out inport. */ output; };
> > +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
> > +    encodes as
> controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
> > +    has prereqs nd_ns
> >
> >   # get_nd
> >   get_nd(outport, ip6.dst);
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Numan Siddique May 8, 2018, 9:36 a.m. UTC | #3
On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo <majopela@redhat.com
> wrote:

> Thank you Numan!
>
> It took me a bit to find what the "RSO"  flag was, because they are R, S
> and O, may be we can change that in the commit, or reference the
> RFC/section  (RFC4861 page 23).
>

Ack. I will update the commit message and send v2.


>
>      R              Router flag.  When set, the R-bit indicates that
>                      the sender is a router.  The R-bit is used by
>                      Neighbor Unreachability Detection to detect a
>                      router that changes to a host.
>
>       S              Solicited flag.  When set, the S-bit indicates that
>                      the advertisement was sent in response to a
>                      Neighbor Solicitation from the Destination address.
>                      The S-bit is used as a reachability confirmation
>                      for Neighbor Unreachability Detection.  It MUST NOT
>                      be set in multicast advertisements or in
>                      unsolicited unicast advertisements.
>
>       O              Override flag.  When set, the O-bit indicates that
>                      the advertisement should override an existing cache
>                      entry and update the cached link-layer address.
>                      When it is not set the advertisement will not
>                      update a cached link-layer address though it will
>                      update an existing Neighbor Cache entry for which
>                      no link-layer address is known.  It SHOULD NOT be
>                      set in solicited advertisements for anycast
>                      addresses and in solicited proxy advertisements.
>                      It SHOULD be set in other solicited advertisements
>                      and in unsolicited advertisements.
>
>
>
>
> And same question Mark did.
>
> Thanks for handling this, good work :)
>
> On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels@redhat.com> wrote:
>
>> Hi Numan, thanks for the fix.
>>
>> Out of curiosity, why did you add a new action instead of adding a new
>> logical field (something like nd.rso)?
>>
>
The logical flow which uses nd_na looks like

  table=11(ls_in_arp_rsp      ), priority=50   , match=(nd_ns && ip6.dst ==
{2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target ==
2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src = fa:16:3e:fe:9e:2e;
ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target = 2002::f816:3eff:fefe:9e2e;
nd.tll = fa:16:3e:fe:9e:2e; outport = inport; flags.loopback = 1; output;
};)


I suppose you are suggesting to have something like - " nd_na { .., nd.rso
= router ..} ". All the inner logical fields are defined in expr symtab
table [1]. But we cannot add nd.rso there as there is no corresponding ovs
field to modify the ICMPv6 flags of a packet.

From the email discussion here [2] I suppose Zak is working to add this
feature. Once we have this support, we can make use of that in OVN. Right
now IPv6 feature is blocked in Openstack + OVN  because of this issue so we
need this fix.

[1] - https://github.com/openvswitch/ovs/blob/master/
ovn/lib/logical-fields.c#L205
[2] -
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046662.html

https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046528.html


Thanks
Numan




>
>> On 05/07/2018 02:36 PM, nusiddiq@redhat.com wrote:
>> > From: Numan Siddique <nusiddiq@redhat.com>
>> >
>> > Presently when a VM's IPv6 stack sends a Neighbor Solicitation request
>> for its
>> > router IP, (mostly when the ND cache entry for the router is in STALE
>> state)
>> > ovn-controller responds with a Neighbor Adv packet (using the action
>> nd_na).
>> > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because of which,
>> the
>> > VM deletes the default route. The default route gets added again when
>> the next
>> > RA is received (but would again gets deleted if its sends NS request).
>> And this
>> > results in disruption of IPv6 traffic.
>> >
>> > This patch addresses this issue by adding a new action 'nd_na_router'
>> which is
>> > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO flags.
>> ovn-northd
>> > uses this action. A new action is added instead of modifying the
>> existing 'nd_na'
>> > action. This is because
>> >    - We cannot set the RSO flags in the "nd_na { ..actions .. }"
>> >    - It would be ugly to have something like nd_na { router_flags,
>> ...actions .. }
>> >
>> > (Please note: There are 3 'Line length is >79-characters' warnings in
>> ovn.at which
>> > I couldn't resolve.)
>> >
>> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
>> > CC: Justin Pettit <jpettit@ovn.org>
>> > CC: Mark Michelson <mmichels@redhat.com>
>> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>> > ---
>> >   include/ovn/actions.h     |  7 +++++++
>> >   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
>> >   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
>> >   ovn/northd/ovn-northd.c   |  2 +-
>> >   ovn/utilities/ovn-trace.c |  1 +
>> >   tests/ovn.at              |  5 +++++
>> >   6 files changed, 51 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> > index fb8f51509..638465193 100644
>> > --- a/include/ovn/actions.h
>> > +++ b/include/ovn/actions.h
>> > @@ -68,6 +68,7 @@ struct ovn_extend_table;
>> >       OVNACT(ICMP6,             ovnact_nest)            \
>> >       OVNACT(TCP_RESET,         ovnact_nest)            \
>> >       OVNACT(ND_NA,             ovnact_nest)            \
>> > +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
>> >       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
>> >       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
>> >       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
>> > @@ -444,6 +445,12 @@ enum action_opcode {
>> >        * The actions, in OpenFlow 1.3 format, follow the action_header.
>> >        */
>> >       ACTION_OPCODE_TCP_RESET,
>> > +
>> > +    /* "nd_na_router { ...actions... }" with rso flag 'ND_RSO_ROUTER'
>> set.
>> > +        *
>> > +        * The actions, in OpenFlow 1.3 format, follow the
>> action_header.
>> > +        */
>> > +    ACTION_OPCODE_ND_NA_ROUTER,
>> >   };
>> >
>> >   /* Header. */
>> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>> > index 6e6aa1caa..305f20649 100644
>> > --- a/ovn/controller/pinctrl.c
>> > +++ b/ovn/controller/pinctrl.c
>> > @@ -81,7 +81,8 @@ static void send_garp_run(struct controller_ctx *ctx,
>> >                             struct sset *active_tunnels);
>> >   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
>> >                                    const struct match *md,
>> > -                                 struct ofpbuf *userdata);
>> > +                                 struct ofpbuf *userdata,
>> > +                                 bool is_router);
>> >   static void reload_metadata(struct ofpbuf *ofpacts,
>> >                               const struct match *md);
>> >   static void pinctrl_handle_put_nd_ra_opts(
>> > @@ -1154,7 +1155,11 @@ process_packet_in(const struct ofp_header *msg,
>> struct controller_ctx *ctx)
>> >           break;
>> >
>> >       case ACTION_OPCODE_ND_NA:
>> > -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
>> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
>> false);
>> > +        break;
>> > +
>> > +    case ACTION_OPCODE_ND_NA_ROUTER:
>> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
>> true);
>> >           break;
>> >
>> >       case ACTION_OPCODE_PUT_ND:
>> > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts, const
>> struct match *md)
>> >
>> >   static void
>> >   pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match
>> *md,
>> > -                     struct ofpbuf *userdata)
>> > +                     struct ofpbuf *userdata, bool is_router)
>> >   {
>> >       /* This action only works for IPv6 ND packets, and the switch
>> should only
>> >        * send us ND packets this way, but check here just to be sure. */
>> > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct flow
>> *ip_flow, const struct match *md,
>> >       struct dp_packet packet;
>> >       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>> >
>> > -    /* xxx These flags are not exactly correct.  Look at section 7.2.4
>> > -     * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
>> > -     * xxx router's interfaces and ND_RSO_SOLICITED only if it was
>> > -     * xxx requested. */
>> > +    /* These flags are not exactly correct.  Look at section 7.2.4
>> > +     * of RFC 4861. */
>> > +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
>> > +    if (is_router) {
>> > +        rso_flags |= ND_RSO_ROUTER;
>> > +    }
>> >       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
>> >                     &ip_flow->nd_target, &ip_flow->ipv6_src,
>> > -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
>> > +                  htonl(rso_flags));
>> >
>> >       /* Reload previous packet metadata and set actions from userdata.
>> */
>> >       set_actions_and_enqueue_msg(&packet, md, userdata);
>> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
>> > index a6945812d..0669cc1c9 100644
>> > --- a/ovn/lib/actions.c
>> > +++ b/ovn/lib/actions.c
>> > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
>> >       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
>> >   }
>> >
>> > +static void
>> > +parse_ND_NA_ROUTER(struct action_context *ctx)
>> > +{
>> > +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
>> > +}
>> > +
>> >   static void
>> >   parse_ND_NS(struct action_context *ctx)
>> >   {
>> > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest *nest,
>> struct ds *s)
>> >       format_nested_action(nest, "nd_na", s);
>> >   }
>> >
>> > +static void
>> > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct ds *s)
>> > +{
>> > +    format_nested_action(nest, "nd_na_router", s);
>> > +}
>> > +
>> >   static void
>> >   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
>> >   {
>> > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
>> >       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, ofpacts);
>> >   }
>> >
>> > +static void
>> > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
>> > +             const struct ovnact_encode_params *ep,
>> > +             struct ofpbuf *ofpacts)
>> > +{
>> > +    encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
>> > +}
>> > +
>> >   static void
>> >   encode_ND_NS(const struct ovnact_nest *on,
>> >                const struct ovnact_encode_params *ep,
>> > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
>> >           parse_TCP_RESET(ctx);
>> >       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
>> >           parse_ND_NA(ctx);
>> > +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
>> > +        parse_ND_NA_ROUTER(ctx);
>> >       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
>> >           parse_ND_NS(ctx);
>> >       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
>> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> > index ce472a536..b157cd1eb 100644
>> > --- a/ovn/northd/ovn-northd.c
>> > +++ b/ovn/northd/ovn-northd.c
>> > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap *datapaths,
>> struct hmap *ports,
>> >               ds_clear(&actions);
>> >               ds_put_format(&actions,
>> >                             "put_nd(inport, ip6.src, nd.sll); "
>> > -                          "nd_na { "
>> > +                          "nd_na_router { "
>> >                             "eth.src = %s; "
>> >                             "ip6.src = %s; "
>> >                             "nd.target = %s; "
>> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
>> > index 9c19b5b9a..1fd48f22e 100644
>> > --- a/ovn/utilities/ovn-trace.c
>> > +++ b/ovn/utilities/ovn-trace.c
>> > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact *ovnacts,
>> size_t ovnacts_len,
>> >               break;
>> >
>> >           case OVNACT_ND_NA:
>> > +        case OVNACT_ND_NA_ROUTER:
>> >               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, table_id,
>> pipeline,
>> >                             super);
>> >               break;
>> > diff --git a/tests/ovn.at b/tests/ovn.at
>> > index 4a5316510..2c0ae9877 100644
>> > --- a/tests/ovn.at
>> > +++ b/tests/ovn.at
>> > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
>> 12:34:56:78:9a:bc; outport = inpor
>> >       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
>> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>> >       encodes as controller(userdata=00.00.00.
>> 03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.
>> 00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.
>> 18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.
>> 04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.
>> 10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>> >       has prereqs nd_ns
>> > +# nd_na_router
>> > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>> 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out
>> inport. */ output; };
>> > +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>> > +    encodes as controller(userdata=00.00.00.
>> 0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.
>> 00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.
>> 18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.
>> 04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.
>> 10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>> > +    has prereqs nd_ns
>> >
>> >   # get_nd
>> >   get_nd(outport, ip6.dst);
>> >
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Miguel Angel Ajo May 8, 2018, 9:39 a.m. UTC | #4
Ack, makes sense to me

On Tue, May 8, 2018 at 11:36 AM, Numan Siddique <nusiddiq@redhat.com> wrote:

>
>
> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo <
> majopela@redhat.com> wrote:
>
>> Thank you Numan!
>>
>> It took me a bit to find what the "RSO"  flag was, because they are R, S
>> and O, may be we can change that in the commit, or reference the
>> RFC/section  (RFC4861 page 23).
>>
>
> Ack. I will update the commit message and send v2.
>
>
>>
>>      R              Router flag.  When set, the R-bit indicates that
>>                      the sender is a router.  The R-bit is used by
>>                      Neighbor Unreachability Detection to detect a
>>                      router that changes to a host.
>>
>>       S              Solicited flag.  When set, the S-bit indicates that
>>                      the advertisement was sent in response to a
>>                      Neighbor Solicitation from the Destination address.
>>                      The S-bit is used as a reachability confirmation
>>                      for Neighbor Unreachability Detection.  It MUST NOT
>>                      be set in multicast advertisements or in
>>                      unsolicited unicast advertisements.
>>
>>       O              Override flag.  When set, the O-bit indicates that
>>                      the advertisement should override an existing cache
>>                      entry and update the cached link-layer address.
>>                      When it is not set the advertisement will not
>>                      update a cached link-layer address though it will
>>                      update an existing Neighbor Cache entry for which
>>                      no link-layer address is known.  It SHOULD NOT be
>>                      set in solicited advertisements for anycast
>>                      addresses and in solicited proxy advertisements.
>>                      It SHOULD be set in other solicited advertisements
>>                      and in unsolicited advertisements.
>>
>>
>>
>>
>> And same question Mark did.
>>
>> Thanks for handling this, good work :)
>>
>> On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels@redhat.com>
>> wrote:
>>
>>> Hi Numan, thanks for the fix.
>>>
>>> Out of curiosity, why did you add a new action instead of adding a new
>>> logical field (something like nd.rso)?
>>>
>>
> The logical flow which uses nd_na looks like
>
>   table=11(ls_in_arp_rsp      ), priority=50   , match=(nd_ns && ip6.dst
> == {2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target ==
> 2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src = fa:16:3e:fe:9e:2e;
> ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target = 2002::f816:3eff:fefe:9e2e;
> nd.tll = fa:16:3e:fe:9e:2e; outport = inport; flags.loopback = 1; output;
> };)
>
>
> I suppose you are suggesting to have something like - " nd_na { .., nd.rso
> = router ..} ". All the inner logical fields are defined in expr symtab
> table [1]. But we cannot add nd.rso there as there is no corresponding ovs
> field to modify the ICMPv6 flags of a packet.
>
> From the email discussion here [2] I suppose Zak is working to add this
> feature. Once we have this support, we can make use of that in OVN. Right
> now IPv6 feature is blocked in Openstack + OVN  because of this issue so we
> need this fix.
>
> [1] - https://github.com/openvswitch/ovs/blob/master/ovn/lib/
> logical-fields.c#L205
> [2] - https://mail.openvswitch.org/pipermail/ovs-discuss/
> 2018-May/046662.html
>        https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> April/046528.html
>
>
> Thanks
> Numan
>
>
>
>
>>
>>> On 05/07/2018 02:36 PM, nusiddiq@redhat.com wrote:
>>> > From: Numan Siddique <nusiddiq@redhat.com>
>>> >
>>> > Presently when a VM's IPv6 stack sends a Neighbor Solicitation request
>>> for its
>>> > router IP, (mostly when the ND cache entry for the router is in STALE
>>> state)
>>> > ovn-controller responds with a Neighbor Adv packet (using the action
>>> nd_na).
>>> > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because of which,
>>> the
>>> > VM deletes the default route. The default route gets added again when
>>> the next
>>> > RA is received (but would again gets deleted if its sends NS request).
>>> And this
>>> > results in disruption of IPv6 traffic.
>>> >
>>> > This patch addresses this issue by adding a new action 'nd_na_router'
>>> which is
>>> > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO flags.
>>> ovn-northd
>>> > uses this action. A new action is added instead of modifying the
>>> existing 'nd_na'
>>> > action. This is because
>>> >    - We cannot set the RSO flags in the "nd_na { ..actions .. }"
>>> >    - It would be ugly to have something like nd_na { router_flags,
>>> ...actions .. }
>>> >
>>> > (Please note: There are 3 'Line length is >79-characters' warnings in
>>> ovn.at which
>>> > I couldn't resolve.)
>>> >
>>> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1567735
>>> > CC: Justin Pettit <jpettit@ovn.org>
>>> > CC: Mark Michelson <mmichels@redhat.com>
>>> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
>>> > ---
>>> >   include/ovn/actions.h     |  7 +++++++
>>> >   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
>>> >   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
>>> >   ovn/northd/ovn-northd.c   |  2 +-
>>> >   ovn/utilities/ovn-trace.c |  1 +
>>> >   tests/ovn.at              |  5 +++++
>>> >   6 files changed, 51 insertions(+), 9 deletions(-)
>>> >
>>> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>> > index fb8f51509..638465193 100644
>>> > --- a/include/ovn/actions.h
>>> > +++ b/include/ovn/actions.h
>>> > @@ -68,6 +68,7 @@ struct ovn_extend_table;
>>> >       OVNACT(ICMP6,             ovnact_nest)            \
>>> >       OVNACT(TCP_RESET,         ovnact_nest)            \
>>> >       OVNACT(ND_NA,             ovnact_nest)            \
>>> > +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
>>> >       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
>>> >       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
>>> >       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
>>> > @@ -444,6 +445,12 @@ enum action_opcode {
>>> >        * The actions, in OpenFlow 1.3 format, follow the action_header.
>>> >        */
>>> >       ACTION_OPCODE_TCP_RESET,
>>> > +
>>> > +    /* "nd_na_router { ...actions... }" with rso flag 'ND_RSO_ROUTER'
>>> set.
>>> > +        *
>>> > +        * The actions, in OpenFlow 1.3 format, follow the
>>> action_header.
>>> > +        */
>>> > +    ACTION_OPCODE_ND_NA_ROUTER,
>>> >   };
>>> >
>>> >   /* Header. */
>>> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>>> > index 6e6aa1caa..305f20649 100644
>>> > --- a/ovn/controller/pinctrl.c
>>> > +++ b/ovn/controller/pinctrl.c
>>> > @@ -81,7 +81,8 @@ static void send_garp_run(struct controller_ctx *ctx,
>>> >                             struct sset *active_tunnels);
>>> >   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
>>> >                                    const struct match *md,
>>> > -                                 struct ofpbuf *userdata);
>>> > +                                 struct ofpbuf *userdata,
>>> > +                                 bool is_router);
>>> >   static void reload_metadata(struct ofpbuf *ofpacts,
>>> >                               const struct match *md);
>>> >   static void pinctrl_handle_put_nd_ra_opts(
>>> > @@ -1154,7 +1155,11 @@ process_packet_in(const struct ofp_header *msg,
>>> struct controller_ctx *ctx)
>>> >           break;
>>> >
>>> >       case ACTION_OPCODE_ND_NA:
>>> > -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
>>> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
>>> false);
>>> > +        break;
>>> > +
>>> > +    case ACTION_OPCODE_ND_NA_ROUTER:
>>> > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata,
>>> true);
>>> >           break;
>>> >
>>> >       case ACTION_OPCODE_PUT_ND:
>>> > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts, const
>>> struct match *md)
>>> >
>>> >   static void
>>> >   pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match
>>> *md,
>>> > -                     struct ofpbuf *userdata)
>>> > +                     struct ofpbuf *userdata, bool is_router)
>>> >   {
>>> >       /* This action only works for IPv6 ND packets, and the switch
>>> should only
>>> >        * send us ND packets this way, but check here just to be sure.
>>> */
>>> > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct flow
>>> *ip_flow, const struct match *md,
>>> >       struct dp_packet packet;
>>> >       dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
>>> >
>>> > -    /* xxx These flags are not exactly correct.  Look at section 7.2.4
>>> > -     * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
>>> > -     * xxx router's interfaces and ND_RSO_SOLICITED only if it was
>>> > -     * xxx requested. */
>>> > +    /* These flags are not exactly correct.  Look at section 7.2.4
>>> > +     * of RFC 4861. */
>>> > +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
>>> > +    if (is_router) {
>>> > +        rso_flags |= ND_RSO_ROUTER;
>>> > +    }
>>> >       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
>>> >                     &ip_flow->nd_target, &ip_flow->ipv6_src,
>>> > -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
>>> > +                  htonl(rso_flags));
>>> >
>>> >       /* Reload previous packet metadata and set actions from
>>> userdata. */
>>> >       set_actions_and_enqueue_msg(&packet, md, userdata);
>>> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
>>> > index a6945812d..0669cc1c9 100644
>>> > --- a/ovn/lib/actions.c
>>> > +++ b/ovn/lib/actions.c
>>> > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
>>> >       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
>>> >   }
>>> >
>>> > +static void
>>> > +parse_ND_NA_ROUTER(struct action_context *ctx)
>>> > +{
>>> > +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
>>> > +}
>>> > +
>>> >   static void
>>> >   parse_ND_NS(struct action_context *ctx)
>>> >   {
>>> > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest *nest,
>>> struct ds *s)
>>> >       format_nested_action(nest, "nd_na", s);
>>> >   }
>>> >
>>> > +static void
>>> > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct ds *s)
>>> > +{
>>> > +    format_nested_action(nest, "nd_na_router", s);
>>> > +}
>>> > +
>>> >   static void
>>> >   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
>>> >   {
>>> > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
>>> >       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, ofpacts);
>>> >   }
>>> >
>>> > +static void
>>> > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
>>> > +             const struct ovnact_encode_params *ep,
>>> > +             struct ofpbuf *ofpacts)
>>> > +{
>>> > +    encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA_ROUTER,
>>> ofpacts);
>>> > +}
>>> > +
>>> >   static void
>>> >   encode_ND_NS(const struct ovnact_nest *on,
>>> >                const struct ovnact_encode_params *ep,
>>> > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
>>> >           parse_TCP_RESET(ctx);
>>> >       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
>>> >           parse_ND_NA(ctx);
>>> > +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
>>> > +        parse_ND_NA_ROUTER(ctx);
>>> >       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
>>> >           parse_ND_NS(ctx);
>>> >       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
>>> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>>> > index ce472a536..b157cd1eb 100644
>>> > --- a/ovn/northd/ovn-northd.c
>>> > +++ b/ovn/northd/ovn-northd.c
>>> > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap *datapaths,
>>> struct hmap *ports,
>>> >               ds_clear(&actions);
>>> >               ds_put_format(&actions,
>>> >                             "put_nd(inport, ip6.src, nd.sll); "
>>> > -                          "nd_na { "
>>> > +                          "nd_na_router { "
>>> >                             "eth.src = %s; "
>>> >                             "ip6.src = %s; "
>>> >                             "nd.target = %s; "
>>> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
>>> > index 9c19b5b9a..1fd48f22e 100644
>>> > --- a/ovn/utilities/ovn-trace.c
>>> > +++ b/ovn/utilities/ovn-trace.c
>>> > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact *ovnacts,
>>> size_t ovnacts_len,
>>> >               break;
>>> >
>>> >           case OVNACT_ND_NA:
>>> > +        case OVNACT_ND_NA_ROUTER:
>>> >               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, table_id,
>>> pipeline,
>>> >                             super);
>>> >               break;
>>> > diff --git a/tests/ovn.at b/tests/ovn.at
>>> > index 4a5316510..2c0ae9877 100644
>>> > --- a/tests/ovn.at
>>> > +++ b/tests/ovn.at
>>> > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
>>> 12:34:56:78:9a:bc; outport = inpor
>>> >       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll =
>>> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>>> >       encodes as controller(userdata=00.00.00.0
>>> 3.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.0
>>> 0.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.1
>>> 8.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.0
>>> 4.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.1
>>> 0.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>>> >       has prereqs nd_ns
>>> > +# nd_na_router
>>> > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>>> 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out
>>> inport. */ output; };
>>> > +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>>> 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>>> > +    encodes as controller(userdata=00.00.00.0
>>> c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.0
>>> 0.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.1
>>> 8.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.0
>>> 4.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.1
>>> 0.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>>> > +    has prereqs nd_ns
>>> >
>>> >   # get_nd
>>> >   get_nd(outport, ip6.dst);
>>> >
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>
Mark Michelson May 8, 2018, 1:26 p.m. UTC | #5
On 05/08/2018 05:36 AM, Numan Siddique wrote:
> 
> 
> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo 
> <majopela@redhat.com <mailto:majopela@redhat.com>> wrote:
> 
>     Thank you Numan!
> 
>     It took me a bit to find what the "RSO"  flag was, because they are
>     R, S and O, may be we can change that in the commit, or reference
>     the RFC/section  (RFC4861 page 23).
> 
> 
> Ack. I will update the commit message and send v2.
> 
> 
>           R              Router flag.  When set, the R-bit indicates that
>                           the sender is a router.  The R-bit is used by
>                           Neighbor Unreachability Detection to detect a
>                           router that changes to a host.
> 
>            S              Solicited flag.  When set, the S-bit indicates that
>                           the advertisement was sent in response to a
>                           Neighbor Solicitation from the Destination address.
>                           The S-bit is used as a reachability confirmation
>                           for Neighbor Unreachability Detection.  It MUST NOT
>                           be set in multicast advertisements or in
>                           unsolicited unicast advertisements.
> 
>            O              Override flag.  When set, the O-bit indicates that
>                           the advertisement should override an existing cache
>                           entry and update the cached link-layer address.
>                           When it is not set the advertisement will not
>                           update a cached link-layer address though it will
>                           update an existing Neighbor Cache entry for which
>                           no link-layer address is known.  It SHOULD NOT be
>                           set in solicited advertisements for anycast
>                           addresses and in solicited proxy advertisements.
>                           It SHOULD be set in other solicited advertisements
>                           and in unsolicited advertisements.
> 
> 
> 
> 
>     And same question Mark did.
> 
>     Thanks for handling this, good work :)
> 
>     On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels@redhat.com
>     <mailto:mmichels@redhat.com>> wrote:
> 
>         Hi Numan, thanks for the fix.
> 
>         Out of curiosity, why did you add a new action instead of adding
>         a new
>         logical field (something like nd.rso)?
> 
> 
> The logical flow which uses nd_na looks like
> 
>    table=11(ls_in_arp_rsp      ), priority=50   , match=(nd_ns && 
> ip6.dst == {2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target 
> == 2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src = 
> fa:16:3e:fe:9e:2e; ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target = 
> 2002::f816:3eff:fefe:9e2e; nd.tll = fa:16:3e:fe:9e:2e; outport = inport; 
> flags.loopback = 1; output; };)
> 
> 
> I suppose you are suggesting to have something like - " nd_na { .., 
> nd.rso = router ..} ". All the inner logical fields are defined in expr 
> symtab table [1]. But we cannot add nd.rso there as there is no 
> corresponding ovs field to modify the ICMPv6 flags of a packet.
> 
>  From the email discussion here [2] I suppose Zak is working to add this 
> feature. Once we have this support, we can make use of that in OVN. 
> Right now IPv6 feature is blocked in Openstack + OVN  because of this 
> issue so we need this fix.

Yes, this is what I had been thinking. It sounds like if the timing had 
been different, you could have waited for Zak's patch first. But given 
the circumstances, I suppose this will work.

> 
> [1] - 
> https://github.com/openvswitch/ovs/blob/master/ovn/lib/logical-fields.c#L205 
> <https://github.com/openvswitch/ovs/blob/master/ovn/lib/logical-fields.c#L205>
> [2] - 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046662.html
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046528.html
> 
> 
> Thanks
> Numan
> 
> 
> 
>         On 05/07/2018 02:36 PM, nusiddiq@redhat.com
>         <mailto:nusiddiq@redhat.com> wrote:
>          > From: Numan Siddique <nusiddiq@redhat.com
>         <mailto:nusiddiq@redhat.com>>
>          >
>          > Presently when a VM's IPv6 stack sends a Neighbor
>         Solicitation request for its
>          > router IP, (mostly when the ND cache entry for the router is
>         in STALE state)
>          > ovn-controller responds with a Neighbor Adv packet (using the
>         action nd_na).
>          > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because
>         of which, the
>          > VM deletes the default route. The default route gets added
>         again when the next
>          > RA is received (but would again gets deleted if its sends NS
>         request). And this
>          > results in disruption of IPv6 traffic.
>          >
>          > This patch addresses this issue by adding a new action
>         'nd_na_router' which is
>          > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO
>         flags. ovn-northd
>          > uses this action. A new action is added instead of modifying
>         the existing 'nd_na'
>          > action. This is because
>          >    - We cannot set the RSO flags in the "nd_na { ..actions .. }"
>          >    - It would be ugly to have something like nd_na {
>         router_flags, ...actions .. }
>          >
>          > (Please note: There are 3 'Line length is >79-characters'
>         warnings in ovn.at <http://ovn.at> which
>          > I couldn't resolve.)
>          >
>          > Reported-at:
>         https://bugzilla.redhat.com/show_bug.cgi?id=1567735
>         <https://bugzilla.redhat.com/show_bug.cgi?id=1567735>
>          > CC: Justin Pettit <jpettit@ovn.org <mailto:jpettit@ovn.org>>
>          > CC: Mark Michelson <mmichels@redhat.com
>         <mailto:mmichels@redhat.com>>
>          > Signed-off-by: Numan Siddique <nusiddiq@redhat.com
>         <mailto:nusiddiq@redhat.com>>
>          > ---
>          >   include/ovn/actions.h     |  7 +++++++
>          >   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
>          >   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
>          >   ovn/northd/ovn-northd.c   |  2 +-
>          >   ovn/utilities/ovn-trace.c |  1 +
>          >   tests/ovn.at <http://ovn.at>              |  5 +++++
>          >   6 files changed, 51 insertions(+), 9 deletions(-)
>          >
>          > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>          > index fb8f51509..638465193 100644
>          > --- a/include/ovn/actions.h
>          > +++ b/include/ovn/actions.h
>          > @@ -68,6 +68,7 @@ struct ovn_extend_table;
>          >       OVNACT(ICMP6,             ovnact_nest)            \
>          >       OVNACT(TCP_RESET,         ovnact_nest)            \
>          >       OVNACT(ND_NA,             ovnact_nest)            \
>          > +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
>          >       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
>          >       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
>          >       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
>          > @@ -444,6 +445,12 @@ enum action_opcode {
>          >        * The actions, in OpenFlow 1.3 format, follow the
>         action_header.
>          >        */
>          >       ACTION_OPCODE_TCP_RESET,
>          > +
>          > +    /* "nd_na_router { ...actions... }" with rso flag
>         'ND_RSO_ROUTER' set.
>          > +        *
>          > +        * The actions, in OpenFlow 1.3 format, follow the
>         action_header.
>          > +        */
>          > +    ACTION_OPCODE_ND_NA_ROUTER,
>          >   };
>          >
>          >   /* Header. */
>          > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
>          > index 6e6aa1caa..305f20649 100644
>          > --- a/ovn/controller/pinctrl.c
>          > +++ b/ovn/controller/pinctrl.c
>          > @@ -81,7 +81,8 @@ static void send_garp_run(struct
>         controller_ctx *ctx,
>          >                             struct sset *active_tunnels);
>          >   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
>          >                                    const struct match *md,
>          > -                                 struct ofpbuf *userdata);
>          > +                                 struct ofpbuf *userdata,
>          > +                                 bool is_router);
>          >   static void reload_metadata(struct ofpbuf *ofpacts,
>          >                               const struct match *md);
>          >   static void pinctrl_handle_put_nd_ra_opts(
>          > @@ -1154,7 +1155,11 @@ process_packet_in(const struct
>         ofp_header *msg, struct controller_ctx *ctx)
>          >           break;
>          >
>          >       case ACTION_OPCODE_ND_NA:
>          > -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>         &userdata);
>          > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>         &userdata, false);
>          > +        break;
>          > +
>          > +    case ACTION_OPCODE_ND_NA_ROUTER:
>          > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>         &userdata, true);
>          >           break;
>          >
>          >       case ACTION_OPCODE_PUT_ND:
>          > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts,
>         const struct match *md)
>          >
>          >   static void
>          >   pinctrl_handle_nd_na(const struct flow *ip_flow, const
>         struct match *md,
>          > -                     struct ofpbuf *userdata)
>          > +                     struct ofpbuf *userdata, bool is_router)
>          >   {
>          >       /* This action only works for IPv6 ND packets, and the
>         switch should only
>          >        * send us ND packets this way, but check here just to
>         be sure. */
>          > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct
>         flow *ip_flow, const struct match *md,
>          >       struct dp_packet packet;
>          >       dp_packet_use_stub(&packet, packet_stub, sizeof
>         packet_stub);
>          >
>          > -    /* xxx These flags are not exactly correct.  Look at
>         section 7.2.4
>          > -     * xxx of RFC 4861.  For example, we need to set
>         ND_RSO_ROUTER for
>          > -     * xxx router's interfaces and ND_RSO_SOLICITED only if
>         it was
>          > -     * xxx requested. */
>          > +    /* These flags are not exactly correct.  Look at section
>         7.2.4
>          > +     * of RFC 4861. */
>          > +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
>          > +    if (is_router) {
>          > +        rso_flags |= ND_RSO_ROUTER;
>          > +    }
>          >       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
>          >                     &ip_flow->nd_target, &ip_flow->ipv6_src,
>          > -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
>          > +                  htonl(rso_flags));
>          >
>          >       /* Reload previous packet metadata and set actions from
>         userdata. */
>          >       set_actions_and_enqueue_msg(&packet, md, userdata);
>          > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
>          > index a6945812d..0669cc1c9 100644
>          > --- a/ovn/lib/actions.c
>          > +++ b/ovn/lib/actions.c
>          > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
>          >       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
>          >   }
>          >
>          > +static void
>          > +parse_ND_NA_ROUTER(struct action_context *ctx)
>          > +{
>          > +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
>          > +}
>          > +
>          >   static void
>          >   parse_ND_NS(struct action_context *ctx)
>          >   {
>          > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest
>         *nest, struct ds *s)
>          >       format_nested_action(nest, "nd_na", s);
>          >   }
>          >
>          > +static void
>          > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct
>         ds *s)
>          > +{
>          > +    format_nested_action(nest, "nd_na_router", s);
>          > +}
>          > +
>          >   static void
>          >   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
>          >   {
>          > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest *on,
>          >       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA,
>         ofpacts);
>          >   }
>          >
>          > +static void
>          > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
>          > +             const struct ovnact_encode_params *ep,
>          > +             struct ofpbuf *ofpacts)
>          > +{
>          > +    encode_nested_actions(on, ep,
>         ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
>          > +}
>          > +
>          >   static void
>          >   encode_ND_NS(const struct ovnact_nest *on,
>          >                const struct ovnact_encode_params *ep,
>          > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
>          >           parse_TCP_RESET(ctx);
>          >       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
>          >           parse_ND_NA(ctx);
>          > +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
>          > +        parse_ND_NA_ROUTER(ctx);
>          >       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
>          >           parse_ND_NS(ctx);
>          >       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
>          > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>          > index ce472a536..b157cd1eb 100644
>          > --- a/ovn/northd/ovn-northd.c
>          > +++ b/ovn/northd/ovn-northd.c
>          > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap
>         *datapaths, struct hmap *ports,
>          >               ds_clear(&actions);
>          >               ds_put_format(&actions,
>          >                             "put_nd(inport, ip6.src, nd.sll); "
>          > -                          "nd_na { "
>          > +                          "nd_na_router { "
>          >                             "eth.src = %s; "
>          >                             "ip6.src = %s; "
>          >                             "nd.target = %s; "
>          > diff --git a/ovn/utilities/ovn-trace.c
>         b/ovn/utilities/ovn-trace.c
>          > index 9c19b5b9a..1fd48f22e 100644
>          > --- a/ovn/utilities/ovn-trace.c
>          > +++ b/ovn/utilities/ovn-trace.c
>          > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact
>         *ovnacts, size_t ovnacts_len,
>          >               break;
>          >
>          >           case OVNACT_ND_NA:
>          > +        case OVNACT_ND_NA_ROUTER:
>          >               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow,
>         table_id, pipeline,
>          >                             super);
>          >               break;
>          > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
>         <http://ovn.at>
>          > index 4a5316510..2c0ae9877 100644
>          > --- a/tests/ovn.at <http://ovn.at>
>          > +++ b/tests/ovn.at <http://ovn.at>
>          > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc;
>         nd.tll = 12:34:56:78:9a:bc; outport = inpor
>          >       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll
>         = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>          >       encodes as
>         controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>          >       has prereqs nd_ns
>          > +# nd_na_router
>          > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>         12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow
>         sending out inport. */ output; };
>          > +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc;
>         nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = "";
>         output; };
>          > +    encodes as
>         controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>          > +    has prereqs nd_ns
>          >
>          >   # get_nd
>          >   get_nd(outport, ip6.dst);
>          >
> 
>         _______________________________________________
>         dev mailing list
>         dev@openvswitch.org <mailto:dev@openvswitch.org>
>         https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>         <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> 
>
Daniel Alvarez Sanchez May 10, 2018, 4:07 p.m. UTC | #6
On Tue, May 8, 2018 at 3:26 PM, Mark Michelson <mmichels@redhat.com> wrote:

> On 05/08/2018 05:36 AM, Numan Siddique wrote:
>
>>
>>
>> On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo <
>> majopela@redhat.com <mailto:majopela@redhat.com>> wrote:
>>
>>     Thank you Numan!
>>
>>     It took me a bit to find what the "RSO"  flag was, because they are
>>     R, S and O, may be we can change that in the commit, or reference
>>     the RFC/section  (RFC4861 page 23).
>>
>>
>> Ack. I will update the commit message and send v2.
>>
>>
>>           R              Router flag.  When set, the R-bit indicates that
>>                           the sender is a router.  The R-bit is used by
>>                           Neighbor Unreachability Detection to detect a
>>                           router that changes to a host.
>>
>>            S              Solicited flag.  When set, the S-bit indicates
>> that
>>                           the advertisement was sent in response to a
>>                           Neighbor Solicitation from the Destination
>> address.
>>                           The S-bit is used as a reachability confirmation
>>                           for Neighbor Unreachability Detection.  It MUST
>> NOT
>>                           be set in multicast advertisements or in
>>                           unsolicited unicast advertisements.
>>
>>            O              Override flag.  When set, the O-bit indicates
>> that
>>                           the advertisement should override an existing
>> cache
>>                           entry and update the cached link-layer address.
>>                           When it is not set the advertisement will not
>>                           update a cached link-layer address though it
>> will
>>                           update an existing Neighbor Cache entry for
>> which
>>                           no link-layer address is known.  It SHOULD NOT
>> be
>>                           set in solicited advertisements for anycast
>>                           addresses and in solicited proxy advertisements.
>>                           It SHOULD be set in other solicited
>> advertisements
>>                           and in unsolicited advertisements.
>>
>>
>>
>>
>>     And same question Mark did.
>>
>>     Thanks for handling this, good work :)
>>
>>     On Mon, May 7, 2018 at 9:16 PM Mark Michelson <mmichels@redhat.com
>>     <mailto:mmichels@redhat.com>> wrote:
>>
>>         Hi Numan, thanks for the fix.
>>
>>         Out of curiosity, why did you add a new action instead of adding
>>         a new
>>         logical field (something like nd.rso)?
>>
>>
>> The logical flow which uses nd_na looks like
>>
>>    table=11(ls_in_arp_rsp      ), priority=50   , match=(nd_ns && ip6.dst
>> == {2002::f816:3eff:fefe:9e2e, ff02::1:fffe:9e2e} && nd.target ==
>> 2002::f816:3eff:fefe:9e2e), action=(nd_na { eth.src = fa:16:3e:fe:9e:2e;
>> ip6.src = 2002::f816:3eff:fefe:9e2e; nd.target = 2002::f816:3eff:fefe:9e2e;
>> nd.tll = fa:16:3e:fe:9e:2e; outport = inport; flags.loopback = 1; output;
>> };)
>>
>>
>> I suppose you are suggesting to have something like - " nd_na { ..,
>> nd.rso = router ..} ". All the inner logical fields are defined in expr
>> symtab table [1]. But we cannot add nd.rso there as there is no
>> corresponding ovs field to modify the ICMPv6 flags of a packet.
>>
>>  From the email discussion here [2] I suppose Zak is working to add this
>> feature. Once we have this support, we can make use of that in OVN. Right
>> now IPv6 feature is blocked in Openstack + OVN  because of this issue so we
>> need this fix.
>>
>
> Yes, this is what I had been thinking. It sounds like if the timing had
> been different, you could have waited for Zak's patch first. But given the
> circumstances, I suppose this will work.
>
+1 to this and then follow up :)

>
>
>> [1] - https://github.com/openvswitch/ovs/blob/master/ovn/lib/
>> logical-fields.c#L205 <https://github.com/openvswitc
>> h/ovs/blob/master/ovn/lib/logical-fields.c#L205>
>> [2] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/
>> 046662.html
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-April/046528.html
>>
>>
>> Thanks
>> Numan
>>
>>
>>
>>         On 05/07/2018 02:36 PM, nusiddiq@redhat.com
>>         <mailto:nusiddiq@redhat.com> wrote:
>>          > From: Numan Siddique <nusiddiq@redhat.com
>>         <mailto:nusiddiq@redhat.com>>
>>          >
>>          > Presently when a VM's IPv6 stack sends a Neighbor
>>         Solicitation request for its
>>          > router IP, (mostly when the ND cache entry for the router is
>>         in STALE state)
>>          > ovn-controller responds with a Neighbor Adv packet (using the
>>         action nd_na).
>>          > But it doesn't set 'ND_RSO_ROUTER' in the RSO flags. Because
>>         of which, the
>>          > VM deletes the default route. The default route gets added
>>         again when the next
>>          > RA is received (but would again gets deleted if its sends NS
>>         request). And this
>>          > results in disruption of IPv6 traffic.
>>          >
>>          > This patch addresses this issue by adding a new action
>>         'nd_na_router' which is
>>          > same as 'nd_na' but it sets the 'ND_RSO_ROUTER' in the RSO
>>         flags. ovn-northd
>>          > uses this action. A new action is added instead of modifying
>>         the existing 'nd_na'
>>          > action. This is because
>>          >    - We cannot set the RSO flags in the "nd_na { ..actions ..
>> }"
>>          >    - It would be ugly to have something like nd_na {
>>         router_flags, ...actions .. }
>>          >
>>          > (Please note: There are 3 'Line length is >79-characters'
>>         warnings in ovn.at <http://ovn.at> which
>>          > I couldn't resolve.)
>>          >
>>          > Reported-at:
>>         https://bugzilla.redhat.com/show_bug.cgi?id=1567735
>>         <https://bugzilla.redhat.com/show_bug.cgi?id=1567735>
>>          > CC: Justin Pettit <jpettit@ovn.org <mailto:jpettit@ovn.org>>
>>          > CC: Mark Michelson <mmichels@redhat.com
>>         <mailto:mmichels@redhat.com>>
>>          > Signed-off-by: Numan Siddique <nusiddiq@redhat.com
>>         <mailto:nusiddiq@redhat.com>>
>>          > ---
>>          >   include/ovn/actions.h     |  7 +++++++
>>          >   ovn/controller/pinctrl.c  | 23 +++++++++++++++--------
>>          >   ovn/lib/actions.c         | 22 ++++++++++++++++++++++
>>          >   ovn/northd/ovn-northd.c   |  2 +-
>>          >   ovn/utilities/ovn-trace.c |  1 +
>>          >   tests/ovn.at <http://ovn.at>              |  5 +++++
>>
>>          >   6 files changed, 51 insertions(+), 9 deletions(-)
>>          >
>>          > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>          > index fb8f51509..638465193 100644
>>          > --- a/include/ovn/actions.h
>>          > +++ b/include/ovn/actions.h
>>          > @@ -68,6 +68,7 @@ struct ovn_extend_table;
>>          >       OVNACT(ICMP6,             ovnact_nest)            \
>>          >       OVNACT(TCP_RESET,         ovnact_nest)            \
>>          >       OVNACT(ND_NA,             ovnact_nest)            \
>>          > +    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
>>          >       OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
>>          >       OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
>>          >       OVNACT(GET_ND,            ovnact_get_mac_bind)    \
>>          > @@ -444,6 +445,12 @@ enum action_opcode {
>>          >        * The actions, in OpenFlow 1.3 format, follow the
>>         action_header.
>>          >        */
>>          >       ACTION_OPCODE_TCP_RESET,
>>          > +
>>          > +    /* "nd_na_router { ...actions... }" with rso flag
>>         'ND_RSO_ROUTER' set.
>>          > +        *
>>          > +        * The actions, in OpenFlow 1.3 format, follow the
>>         action_header.
>>          > +        */
>>          > +    ACTION_OPCODE_ND_NA_ROUTER,
>>          >   };
>>          >
>>          >   /* Header. */
>>          > diff --git a/ovn/controller/pinctrl.c
>> b/ovn/controller/pinctrl.c
>>          > index 6e6aa1caa..305f20649 100644
>>          > --- a/ovn/controller/pinctrl.c
>>          > +++ b/ovn/controller/pinctrl.c
>>          > @@ -81,7 +81,8 @@ static void send_garp_run(struct
>>         controller_ctx *ctx,
>>          >                             struct sset *active_tunnels);
>>          >   static void pinctrl_handle_nd_na(const struct flow *ip_flow,
>>          >                                    const struct match *md,
>>          > -                                 struct ofpbuf *userdata);
>>          > +                                 struct ofpbuf *userdata,
>>          > +                                 bool is_router);
>>          >   static void reload_metadata(struct ofpbuf *ofpacts,
>>          >                               const struct match *md);
>>          >   static void pinctrl_handle_put_nd_ra_opts(
>>          > @@ -1154,7 +1155,11 @@ process_packet_in(const struct
>>         ofp_header *msg, struct controller_ctx *ctx)
>>          >           break;
>>          >
>>          >       case ACTION_OPCODE_ND_NA:
>>          > -        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>>         &userdata);
>>          > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>>         &userdata, false);
>>          > +        break;
>>          > +
>>          > +    case ACTION_OPCODE_ND_NA_ROUTER:
>>          > +        pinctrl_handle_nd_na(&headers, &pin.flow_metadata,
>>         &userdata, true);
>>          >           break;
>>          >
>>          >       case ACTION_OPCODE_PUT_ND:
>>          > @@ -2308,7 +2313,7 @@ reload_metadata(struct ofpbuf *ofpacts,
>>         const struct match *md)
>>          >
>>          >   static void
>>          >   pinctrl_handle_nd_na(const struct flow *ip_flow, const
>>         struct match *md,
>>          > -                     struct ofpbuf *userdata)
>>          > +                     struct ofpbuf *userdata, bool is_router)
>>          >   {
>>          >       /* This action only works for IPv6 ND packets, and the
>>         switch should only
>>          >        * send us ND packets this way, but check here just to
>>         be sure. */
>>          > @@ -2322,13 +2327,15 @@ pinctrl_handle_nd_na(const struct
>>         flow *ip_flow, const struct match *md,
>>          >       struct dp_packet packet;
>>          >       dp_packet_use_stub(&packet, packet_stub, sizeof
>>         packet_stub);
>>          >
>>          > -    /* xxx These flags are not exactly correct.  Look at
>>         section 7.2.4
>>          > -     * xxx of RFC 4861.  For example, we need to set
>>         ND_RSO_ROUTER for
>>          > -     * xxx router's interfaces and ND_RSO_SOLICITED only if
>>         it was
>>          > -     * xxx requested. */
>>          > +    /* These flags are not exactly correct.  Look at section
>>         7.2.4
>>          > +     * of RFC 4861. */
>>          > +    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
>>          > +    if (is_router) {
>>          > +        rso_flags |= ND_RSO_ROUTER;
>>          > +    }
>>          >       compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
>>          >                     &ip_flow->nd_target, &ip_flow->ipv6_src,
>>          > -                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
>>          > +                  htonl(rso_flags));
>>          >
>>          >       /* Reload previous packet metadata and set actions from
>>         userdata. */
>>          >       set_actions_and_enqueue_msg(&packet, md, userdata);
>>          > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
>>          > index a6945812d..0669cc1c9 100644
>>          > --- a/ovn/lib/actions.c
>>          > +++ b/ovn/lib/actions.c
>>          > @@ -1155,6 +1155,12 @@ parse_ND_NA(struct action_context *ctx)
>>          >       parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
>>          >   }
>>          >
>>          > +static void
>>          > +parse_ND_NA_ROUTER(struct action_context *ctx)
>>          > +{
>>          > +    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
>>          > +}
>>          > +
>>          >   static void
>>          >   parse_ND_NS(struct action_context *ctx)
>>          >   {
>>          > @@ -1206,6 +1212,12 @@ format_ND_NA(const struct ovnact_nest
>>         *nest, struct ds *s)
>>          >       format_nested_action(nest, "nd_na", s);
>>          >   }
>>          >
>>          > +static void
>>          > +format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct
>>         ds *s)
>>          > +{
>>          > +    format_nested_action(nest, "nd_na_router", s);
>>          > +}
>>          > +
>>          >   static void
>>          >   format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
>>          >   {
>>          > @@ -1282,6 +1294,14 @@ encode_ND_NA(const struct ovnact_nest
>> *on,
>>          >       encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA,
>>         ofpacts);
>>          >   }
>>          >
>>          > +static void
>>          > +encode_ND_NA_ROUTER(const struct ovnact_nest *on,
>>          > +             const struct ovnact_encode_params *ep,
>>          > +             struct ofpbuf *ofpacts)
>>          > +{
>>          > +    encode_nested_actions(on, ep,
>>         ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
>>          > +}
>>          > +
>>          >   static void
>>          >   encode_ND_NS(const struct ovnact_nest *on,
>>          >                const struct ovnact_encode_params *ep,
>>          > @@ -2305,6 +2325,8 @@ parse_action(struct action_context *ctx)
>>          >           parse_TCP_RESET(ctx);
>>          >       } else if (lexer_match_id(ctx->lexer, "nd_na")) {
>>          >           parse_ND_NA(ctx);
>>          > +    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
>>          > +        parse_ND_NA_ROUTER(ctx);
>>          >       } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
>>          >           parse_ND_NS(ctx);
>>          >       } else if (lexer_match_id(ctx->lexer, "get_arp")) {
>>          > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>>          > index ce472a536..b157cd1eb 100644
>>          > --- a/ovn/northd/ovn-northd.c
>>          > +++ b/ovn/northd/ovn-northd.c
>>          > @@ -5104,7 +5104,7 @@ build_lrouter_flows(struct hmap
>>         *datapaths, struct hmap *ports,
>>          >               ds_clear(&actions);
>>          >               ds_put_format(&actions,
>>          >                             "put_nd(inport, ip6.src, nd.sll); "
>>          > -                          "nd_na { "
>>          > +                          "nd_na_router { "
>>          >                             "eth.src = %s; "
>>          >                             "ip6.src = %s; "
>>          >                             "nd.target = %s; "
>>          > diff --git a/ovn/utilities/ovn-trace.c
>>         b/ovn/utilities/ovn-trace.c
>>          > index 9c19b5b9a..1fd48f22e 100644
>>          > --- a/ovn/utilities/ovn-trace.c
>>          > +++ b/ovn/utilities/ovn-trace.c
>>          > @@ -1927,6 +1927,7 @@ trace_actions(const struct ovnact
>>         *ovnacts, size_t ovnacts_len,
>>          >               break;
>>          >
>>          >           case OVNACT_ND_NA:
>>          > +        case OVNACT_ND_NA_ROUTER:
>>          >               execute_nd_na(ovnact_get_ND_NA(a), dp, uflow,
>>         table_id, pipeline,
>>          >                             super);
>>          >               break;
>>          > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
>>         <http://ovn.at>
>>          > index 4a5316510..2c0ae9877 100644
>>          > --- a/tests/ovn.at <http://ovn.at>
>>          > +++ b/tests/ovn.at <http://ovn.at>
>>          > @@ -1033,6 +1033,11 @@ nd_na { eth.src = 12:34:56:78:9a:bc;
>>         nd.tll = 12:34:56:78:9a:bc; outport = inpor
>>          >       formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll
>>         = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
>>          >       encodes as
>>         controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.0
>> 0.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.3
>> 4.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.0
>> 0.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.0
>> 0.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>>          >       has prereqs nd_ns
>>          > +# nd_na_router
>>          > +nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll =
>>         12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow
>>         sending out inport. */ output; };
>>          > +    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc;
>>         nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = "";
>>         output; };
>>          > +    encodes as
>>         controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.0
>> 0.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.3
>> 4.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.0
>> 0.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.0
>> 0.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
>>          > +    has prereqs nd_ns
>>          >
>>          >   # get_nd
>>          >   get_nd(outport, ip6.dst);
>>          >
>>
>>         _______________________________________________
>>         dev mailing list
>>         dev@openvswitch.org <mailto:dev@openvswitch.org>
>>         https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>         <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>>
>>
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index fb8f51509..638465193 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -68,6 +68,7 @@  struct ovn_extend_table;
     OVNACT(ICMP6,             ovnact_nest)            \
     OVNACT(TCP_RESET,         ovnact_nest)            \
     OVNACT(ND_NA,             ovnact_nest)            \
+    OVNACT(ND_NA_ROUTER,      ovnact_nest)            \
     OVNACT(GET_ARP,           ovnact_get_mac_bind)    \
     OVNACT(PUT_ARP,           ovnact_put_mac_bind)    \
     OVNACT(GET_ND,            ovnact_get_mac_bind)    \
@@ -444,6 +445,12 @@  enum action_opcode {
      * The actions, in OpenFlow 1.3 format, follow the action_header.
      */
     ACTION_OPCODE_TCP_RESET,
+
+    /* "nd_na_router { ...actions... }" with rso flag 'ND_RSO_ROUTER' set.
+        *
+        * The actions, in OpenFlow 1.3 format, follow the action_header.
+        */
+    ACTION_OPCODE_ND_NA_ROUTER,
 };
 
 /* Header. */
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 6e6aa1caa..305f20649 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -81,7 +81,8 @@  static void send_garp_run(struct controller_ctx *ctx,
                           struct sset *active_tunnels);
 static void pinctrl_handle_nd_na(const struct flow *ip_flow,
                                  const struct match *md,
-                                 struct ofpbuf *userdata);
+                                 struct ofpbuf *userdata,
+                                 bool is_router);
 static void reload_metadata(struct ofpbuf *ofpacts,
                             const struct match *md);
 static void pinctrl_handle_put_nd_ra_opts(
@@ -1154,7 +1155,11 @@  process_packet_in(const struct ofp_header *msg, struct controller_ctx *ctx)
         break;
 
     case ACTION_OPCODE_ND_NA:
-        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata);
+        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata, false);
+        break;
+
+    case ACTION_OPCODE_ND_NA_ROUTER:
+        pinctrl_handle_nd_na(&headers, &pin.flow_metadata, &userdata, true);
         break;
 
     case ACTION_OPCODE_PUT_ND:
@@ -2308,7 +2313,7 @@  reload_metadata(struct ofpbuf *ofpacts, const struct match *md)
 
 static void
 pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match *md,
-                     struct ofpbuf *userdata)
+                     struct ofpbuf *userdata, bool is_router)
 {
     /* This action only works for IPv6 ND packets, and the switch should only
      * send us ND packets this way, but check here just to be sure. */
@@ -2322,13 +2327,15 @@  pinctrl_handle_nd_na(const struct flow *ip_flow, const struct match *md,
     struct dp_packet packet;
     dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
 
-    /* xxx These flags are not exactly correct.  Look at section 7.2.4
-     * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
-     * xxx router's interfaces and ND_RSO_SOLICITED only if it was
-     * xxx requested. */
+    /* These flags are not exactly correct.  Look at section 7.2.4
+     * of RFC 4861. */
+    uint32_t rso_flags = ND_RSO_SOLICITED | ND_RSO_OVERRIDE;
+    if (is_router) {
+        rso_flags |= ND_RSO_ROUTER;
+    }
     compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
                   &ip_flow->nd_target, &ip_flow->ipv6_src,
-                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
+                  htonl(rso_flags));
 
     /* Reload previous packet metadata and set actions from userdata. */
     set_actions_and_enqueue_msg(&packet, md, userdata);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index a6945812d..0669cc1c9 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1155,6 +1155,12 @@  parse_ND_NA(struct action_context *ctx)
     parse_nested_action(ctx, OVNACT_ND_NA, "nd_ns");
 }
 
+static void
+parse_ND_NA_ROUTER(struct action_context *ctx)
+{
+    parse_nested_action(ctx, OVNACT_ND_NA_ROUTER, "nd_ns");
+}
+
 static void
 parse_ND_NS(struct action_context *ctx)
 {
@@ -1206,6 +1212,12 @@  format_ND_NA(const struct ovnact_nest *nest, struct ds *s)
     format_nested_action(nest, "nd_na", s);
 }
 
+static void
+format_ND_NA_ROUTER(const struct ovnact_nest *nest, struct ds *s)
+{
+    format_nested_action(nest, "nd_na_router", s);
+}
+
 static void
 format_ND_NS(const struct ovnact_nest *nest, struct ds *s)
 {
@@ -1282,6 +1294,14 @@  encode_ND_NA(const struct ovnact_nest *on,
     encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA, ofpacts);
 }
 
+static void
+encode_ND_NA_ROUTER(const struct ovnact_nest *on,
+             const struct ovnact_encode_params *ep,
+             struct ofpbuf *ofpacts)
+{
+    encode_nested_actions(on, ep, ACTION_OPCODE_ND_NA_ROUTER, ofpacts);
+}
+
 static void
 encode_ND_NS(const struct ovnact_nest *on,
              const struct ovnact_encode_params *ep,
@@ -2305,6 +2325,8 @@  parse_action(struct action_context *ctx)
         parse_TCP_RESET(ctx);
     } else if (lexer_match_id(ctx->lexer, "nd_na")) {
         parse_ND_NA(ctx);
+    } else if (lexer_match_id(ctx->lexer, "nd_na_router")) {
+        parse_ND_NA_ROUTER(ctx);
     } else if (lexer_match_id(ctx->lexer, "nd_ns")) {
         parse_ND_NS(ctx);
     } else if (lexer_match_id(ctx->lexer, "get_arp")) {
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ce472a536..b157cd1eb 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5104,7 +5104,7 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             ds_clear(&actions);
             ds_put_format(&actions,
                           "put_nd(inport, ip6.src, nd.sll); "
-                          "nd_na { "
+                          "nd_na_router { "
                           "eth.src = %s; "
                           "ip6.src = %s; "
                           "nd.target = %s; "
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 9c19b5b9a..1fd48f22e 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -1927,6 +1927,7 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
             break;
 
         case OVNACT_ND_NA:
+        case OVNACT_ND_NA_ROUTER:
             execute_nd_na(ovnact_get_ND_NA(a), dp, uflow, table_id, pipeline,
                           super);
             break;
diff --git a/tests/ovn.at b/tests/ovn.at
index 4a5316510..2c0ae9877 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1033,6 +1033,11 @@  nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inpor
     formats as nd_na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
     encodes as controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
     has prereqs nd_ns
+# nd_na_router
+nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out inport. */ output; };
+    formats as nd_na_router { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; output; };
+    encodes as controller(userdata=00.00.00.0c.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.1c.04.00.01.1e.04.00.19.00.10.00.01.1c.04.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00)
+    has prereqs nd_ns
 
 # get_nd
 get_nd(outport, ip6.dst);