Ack, makes sense to me On Tue, May 8, 2018 at 11:36 AM, Numan Siddique <[email protected]> wrote:
> > > On Tue, May 8, 2018 at 1:20 PM, Miguel Angel Ajo Pelayo < > [email protected]> 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 <[email protected]> >> 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, [email protected] wrote: >>> > From: Numan Siddique <[email protected]> >>> > >>> > 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 <[email protected]> >>> > CC: Mark Michelson <[email protected]> >>> > Signed-off-by: Numan Siddique <[email protected]> >>> > --- >>> > 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 >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
