[ovs-dev] [PATCH v2 ovn 1/3] Introduce icmp6_error action
Numan Siddique
numans at ovn.org
Wed Jul 8 09:02:35 UTC 2020
On Wed, Jul 8, 2020 at 2:12 PM Numan Siddique <numans at ovn.org> wrote:
>
>
> On Tue, Jul 7, 2020 at 8:49 PM Lorenzo Bianconi <
> lorenzo.bianconi at redhat.com> 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 <lorenzo.bianconi at redhat.com>
>> ---
>>
>
> 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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
More information about the dev
mailing list