On Wed, Jul 8, 2020 at 2:12 PM Numan Siddique <[email protected]> wrote:
> > > On Tue, Jul 7, 2020 at 8:49 PM Lorenzo Bianconi < > [email protected]> wrote: > >> Intoduce icmp6_error action in order to generate an ICMPv6 packet in >> response to an error in original IPv6 packet >> >> Signed-off-by: Lorenzo Bianconi <[email protected]> >> --- >> > > Hi Lorenzo, > > Thanks for v3. I just have one small nit. > > You need to update a few comments at the beginning > of pinctrl_handle_put_icmp_frag_mtu() function. > > ***** > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index f07fc92c8..4ad3d1fb1 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -5477,10 +5477,10 @@ pinctrl_handle_put_icmp_frag_mtu(struct rconn > *swconn, > enum ofputil_protocol proto = > ofputil_protocol_from_ofp_version(version); > struct dp_packet *pkt_out = NULL; > > - /* This action only works for ICMPv4 packets. */ > + /* This action only works for ICMPv4/v6 packets. */ > if (!is_icmpv4(in_flow, NULL) && !is_icmpv6(in_flow, NULL)) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > - VLOG_WARN_RL(&rl, "put_icmp4_frag_mtu action on non-ICMPv4 > packet"); > + VLOG_WARN_RL(&rl, "put_icmp(4/6)_frag_mtu action on non-ICMPv4/v6 > packet"); > goto exit; > } > > **** > > I was thinking to update this patch myself and apply the series. > But then I've a comment in p3 and I think it's better to address that. > Hi Lorenzo, Actually the comment/thought which I had won't work. Hence I applied this patch series to master with the above changes I suggested. I also did the below change in the patch 3 of the series. ***** diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 98d87d1c1..ddfcebe3f 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -10356,7 +10356,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, "ip6.dst = ip6.src; " "ip6.src = %s; " "ip.ttl = 255; " - "icmp6.type = 2; " + "icmp6.type = 2; /* Packet Too Big. */ " "icmp6.code = 0; " "icmp6.frag_mtu = %d; " "next(pipeline=ingress, table=0); };", ********* Thanks Numan > Thanks > Numan > > > controller/pinctrl.c | 1 + >> include/ovn/actions.h | 9 ++++++++- >> lib/actions.c | 22 ++++++++++++++++++++++ >> ovn-sb.xml | 8 ++++++++ >> tests/ovn.at | 10 ++++++++++ >> utilities/ovn-trace.c | 5 +++++ >> 6 files changed, 54 insertions(+), 1 deletion(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index b2c656efb..61dad9752 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -2772,6 +2772,7 @@ process_packet_in(struct rconn *swconn, const >> struct ofp_header *msg) >> break; >> >> case ACTION_OPCODE_ICMP4_ERROR: >> + case ACTION_OPCODE_ICMP6_ERROR: >> pinctrl_handle_icmp(swconn, &headers, &packet, >> &pin.flow_metadata, >> &userdata, false); >> break; >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index 4a54abe17..652873676 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -92,7 +92,8 @@ struct ovn_extend_table; >> OVNACT(BIND_VPORT, ovnact_bind_vport) \ >> OVNACT(HANDLE_SVC_CHECK, ovnact_handle_svc_check) \ >> OVNACT(FWD_GROUP, ovnact_fwd_group) \ >> - OVNACT(DHCP6_REPLY, ovnact_null) >> + OVNACT(DHCP6_REPLY, ovnact_null) \ >> + OVNACT(ICMP6_ERROR, ovnact_nest) >> >> /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ >> enum OVS_PACKED_ENUM ovnact_type { >> @@ -591,6 +592,12 @@ enum action_opcode { >> * The actions, in OpenFlow 1.3 format, follow the action_header. >> */ >> ACTION_OPCODE_DHCP6_SERVER, >> + >> + /* "icmp6_error { ...actions... }". >> + * >> + * The actions, in OpenFlow 1.3 format, follow the action_header. >> + */ >> + ACTION_OPCODE_ICMP6_ERROR, >> }; >> >> /* Header. */ >> diff --git a/lib/actions.c b/lib/actions.c >> index cd086d2fa..6a3b1ba87 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -1408,6 +1408,12 @@ parse_ICMP6(struct action_context *ctx) >> parse_nested_action(ctx, OVNACT_ICMP6, "ip6"); >> } >> >> +static void >> +parse_ICMP6_ERROR(struct action_context *ctx) >> +{ >> + parse_nested_action(ctx, OVNACT_ICMP6_ERROR, "ip6"); >> +} >> + >> static void >> parse_TCP_RESET(struct action_context *ctx) >> { >> @@ -1471,6 +1477,12 @@ format_ICMP6(const struct ovnact_nest *nest, >> struct ds *s) >> format_nested_action(nest, "icmp6", s); >> } >> >> +static void >> +format_ICMP6_ERROR(const struct ovnact_nest *nest, struct ds *s) >> +{ >> + format_nested_action(nest, "icmp6_error", s); >> +} >> + >> static void >> format_IGMP(const struct ovnact_null *a OVS_UNUSED, struct ds *s) >> { >> @@ -1582,6 +1594,14 @@ encode_ICMP6(const struct ovnact_nest *on, >> encode_nested_actions(on, ep, ACTION_OPCODE_ICMP, ofpacts); >> } >> >> +static void >> +encode_ICMP6_ERROR(const struct ovnact_nest *on, >> + const struct ovnact_encode_params *ep, >> + struct ofpbuf *ofpacts) >> +{ >> + encode_nested_actions(on, ep, ACTION_OPCODE_ICMP6_ERROR, ofpacts); >> +} >> + >> static void >> encode_IGMP(const struct ovnact_null *a OVS_UNUSED, >> const struct ovnact_encode_params *ep OVS_UNUSED, >> @@ -3447,6 +3467,8 @@ parse_action(struct action_context *ctx) >> parse_ICMP4_ERROR(ctx); >> } else if (lexer_match_id(ctx->lexer, "icmp6")) { >> parse_ICMP6(ctx); >> + } else if (lexer_match_id(ctx->lexer, "icmp6_error")) { >> + parse_ICMP6_ERROR(ctx); >> } else if (lexer_match_id(ctx->lexer, "igmp")) { >> ovnact_put_IGMP(ctx->ovnacts); >> } else if (lexer_match_id(ctx->lexer, "tcp_reset")) { >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index cf95a7c9c..bc69b58c0 100644 >> --- a/ovn-sb.xml >> +++ b/ovn-sb.xml >> @@ -2035,6 +2035,9 @@ >> </dd> >> >> <dt><code>icmp6 { <var>action</var>; </code>...<code> >> };</code></dt> >> + <dt> >> + <code>icmp6_error { <var>action</var>; </code>...<code> >> };</code> >> + </dt> >> <dd> >> <p> >> Temporarily replaces the IPv6 packet being processed by an >> ICMPv6 >> @@ -2057,6 +2060,11 @@ >> <li><code>icmp6.code = 1</code> (administratively >> prohibited)</li> >> </ul> >> >> + <p> >> + <code>icmp6_error</code> action is expected to be used to >> + generate an ICMPv6 packet in response to an error in >> original >> + IPv6 packet. >> + </p> >> <p><b>Prerequisite:</b> <code>ip6</code></p> >> </dd> >> >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 9522b55f8..30ee27ae9 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1500,6 +1500,16 @@ icmp6 { }; >> encodes as controller(userdata=00.00.00.0a.00.00.00.00) >> has prereqs ip6 >> >> +# icmp6_error >> +icmp6_error { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; >> + encodes as >> controller(userdata=00.00.00.14.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) >> + has prereqs ip6 >> + >> +icmp6_error { }; >> + formats as icmp6_error { drop; }; >> + encodes as controller(userdata=00.00.00.14.00.00.00.00) >> + has prereqs ip6 >> + >> # tcp_reset >> tcp_reset { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; >> encodes as >> controller(userdata=00.00.00.0b.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >> index 2666c1062..d1ab051ce 100644 >> --- a/utilities/ovn-trace.c >> +++ b/utilities/ovn-trace.c >> @@ -2270,6 +2270,11 @@ trace_actions(const struct ovnact *ovnacts, size_t >> ovnacts_len, >> super); >> break; >> >> + case OVNACT_ICMP6_ERROR: >> + execute_icmp6(ovnact_get_ICMP6_ERROR(a), dp, uflow, table_id, >> + pipeline, super); >> + break; >> + >> case OVNACT_IGMP: >> /* Nothing to do for tracing. */ >> break; >> -- >> 2.26.2 >> >> _______________________________________________ >> 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
