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. >> 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 >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
