[ovs-dev] [PATCH ovn 2/3] Introduce icmp6.frag_mtu action
Numan Siddique
numans at ovn.org
Tue Jul 7 09:03:45 UTC 2020
On Fri, Jul 3, 2020 at 7:55 PM Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
wrote:
> Similar to what have been already done for IPv4, introduce
> icmp6.frag_mtu action in order to set correct mtu in ICMPv6 "packet too
> big" error message
>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
>
Hi Lorenzo,
This patch has a compilation warning when configured with --enable-Werror
--enable-sparse
****
../controller/pinctrl.c:5515:9: error: incorrect type in argument 1
(different base types)
../controller/pinctrl.c:5515:9: expected restricted ovs_be16 [usertype] x
../controller/pinctrl.c:5515:9: got restricted ovs_be32 [usertype]
depbase=`echo controller/patch.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
*****
I see from this and the first patch of the series that the ICMPv6 type 1
(ICMP6_DST_UNREACH) and
code 1 or 4 is used.
But for PMTU we should use type 2 right ? That's what I see in the RFC 4443
section 3.2
So shouldn't we use type 2 instead ?
Please see below for a few comments.
Thanks
Numan
> ---
> controller/pinctrl.c | 73 +++++++++++++++++++++++-------------
> include/ovn/actions.h | 4 ++
> include/ovn/logical-fields.h | 7 ++++
> lib/actions.c | 13 ++++++-
> lib/logical-fields.c | 5 +++
> ovn-sb.xml | 7 ++--
> tests/ovn.at | 8 ++++
> utilities/ovn-trace.c | 7 +++-
> 8 files changed, 91 insertions(+), 33 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 61dad9752..47cc354cb 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -228,12 +228,12 @@ static void pinctrl_handle_nd_ns(struct rconn
> *swconn,
> struct dp_packet *pkt_in,
> const struct match *md,
> struct ofpbuf *userdata);
> -static void pinctrl_handle_put_icmp4_frag_mtu(struct rconn *swconn,
> - const struct flow *in_flow,
> - struct dp_packet *pkt_in,
> - struct ofputil_packet_in
> *pin,
> - struct ofpbuf *userdata,
> - struct ofpbuf
> *continuation);
> +static void pinctrl_handle_put_icmp_frag_mtu(struct rconn *swconn,
> + const struct flow *in_flow,
> + struct dp_packet *pkt_in,
> + struct ofputil_packet_in
> *pin,
> + struct ofpbuf *userdata,
> + struct ofpbuf *continuation);
> static void
> pinctrl_handle_event(struct ofpbuf *userdata)
> OVS_REQUIRES(pinctrl_mutex);
> @@ -2783,8 +2783,9 @@ process_packet_in(struct rconn *swconn, const struct
> ofp_header *msg)
> break;
>
> case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU:
> - pinctrl_handle_put_icmp4_frag_mtu(swconn, &headers, &packet,
> - &pin, &userdata, &continuation);
> + case ACTION_OPCODE_PUT_ICMP6_FRAG_MTU:
> + pinctrl_handle_put_icmp_frag_mtu(swconn, &headers, &packet, &pin,
> + &userdata, &continuation);
> break;
>
> case ACTION_OPCODE_EVENT:
> @@ -5465,42 +5466,60 @@ exit:
>
> /* Called with in the pinctrl_handler thread context. */
> static void
> -pinctrl_handle_put_icmp4_frag_mtu(struct rconn *swconn,
> - const struct flow *in_flow,
> - struct dp_packet *pkt_in,
> - struct ofputil_packet_in *pin,
> - struct ofpbuf *userdata,
> - struct ofpbuf *continuation)
> +pinctrl_handle_put_icmp_frag_mtu(struct rconn *swconn,
> + const struct flow *in_flow,
> + struct dp_packet *pkt_in,
> + struct ofputil_packet_in *pin,
> + struct ofpbuf *userdata,
> + struct ofpbuf *continuation)
> {
> enum ofp_version version = rconn_get_version(swconn);
> enum ofputil_protocol proto =
> ofputil_protocol_from_ofp_version(version);
> struct dp_packet *pkt_out = NULL;
>
> /* This action only works for ICMPv4 packets. */
> - if (!is_icmpv4(in_flow, NULL)) {
> + 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");
> goto exit;
> }
>
> - ovs_be16 *mtu = ofpbuf_try_pull(userdata, sizeof *mtu);
> - if (!mtu) {
> - goto exit;
> - }
> -
> pkt_out = dp_packet_clone(pkt_in);
> pkt_out->l2_5_ofs = pkt_in->l2_5_ofs;
> pkt_out->l2_pad_size = pkt_in->l2_pad_size;
> pkt_out->l3_ofs = pkt_in->l3_ofs;
> pkt_out->l4_ofs = pkt_in->l4_ofs;
>
> - struct ip_header *nh = dp_packet_l3(pkt_out);
> - struct icmp_header *ih = dp_packet_l4(pkt_out);
> - ovs_be16 old_frag_mtu = ih->icmp_fields.frag.mtu;
> - ih->icmp_fields.frag.mtu = *mtu;
> - ih->icmp_csum = recalc_csum16(ih->icmp_csum, old_frag_mtu, *mtu);
> - nh->ip_csum = 0;
> - nh->ip_csum = csum(nh, sizeof *nh);
> + if (is_icmpv4(in_flow, NULL)) {
> + ovs_be16 *mtu = ofpbuf_try_pull(userdata, sizeof *mtu);
> + if (!mtu) {
> + goto exit;
> + }
> +
> + struct ip_header *nh = dp_packet_l3(pkt_out);
> + struct icmp_header *ih = dp_packet_l4(pkt_out);
> + ovs_be16 old_frag_mtu = ih->icmp_fields.frag.mtu;
> + ih->icmp_fields.frag.mtu = *mtu;
> + ih->icmp_csum = recalc_csum16(ih->icmp_csum, old_frag_mtu, *mtu);
> + nh->ip_csum = 0;
> + nh->ip_csum = csum(nh, sizeof *nh);
> + } else {
> + ovs_be32 *mtu = ofpbuf_try_pull(userdata, sizeof *mtu);
> + if (!mtu) {
> + goto exit;
> + }
> +
> + struct icmp6_data_header *ih = dp_packet_l4(pkt_out);
> + put_16aligned_be32(ih->icmp6_data.be32, *mtu);
> +
> + VLOG_WARN("%s-%d: mtu=%d\n", __func__, __LINE__, ntohs(*mtu));
>
Looks like this was used for debugging and you forgot to clean it up ?
And the compilation warning is because of this.
> + /* compute checksum and set correct mtu */
> + ih->icmp6_base.icmp6_cksum = 0;
> + uint32_t csum = packet_csum_pseudoheader6(dp_packet_l3(pkt_out));
> + uint32_t size = (uint8_t *)dp_packet_tail(pkt_out) - (uint8_t
> *)ih;
> + ih->icmp6_base.icmp6_cksum = csum_finish(
> + csum_continue(csum, ih, size));
> + }
>
> pin->packet = dp_packet_data(pkt_out);
> pin->packet_len = dp_packet_size(pkt_out);
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 652873676..0f7f4cdb8 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -598,6 +598,10 @@ enum action_opcode {
> * The actions, in OpenFlow 1.3 format, follow the action_header.
> */
> ACTION_OPCODE_ICMP6_ERROR,
> +
> + /* MTU value (to put in the icmp6 header field - frag_mtu) follow the
> + * action header. */
> + ACTION_OPCODE_PUT_ICMP6_FRAG_MTU,
> };
>
> /* Header. */
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index c7bd2dba9..25ef19c5e 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -108,6 +108,13 @@ enum ovn_field_id {
> * packet as per the RFC 1191.
> */
> OVN_ICMP4_FRAG_MTU,
> + /*
> + * Name: "icmp6.frag_mtu" -
> + * Type: be32
> + * Description: Sets the low-order 16 bits of the ICMP6 header field
> + * of the ICMP6 packet
>
The description seems to be wrong. It sets the 32-bit but it says 16-bits
field.
> + */
> + OVN_ICMP6_FRAG_MTU,
>
> OVN_FIELD_N_IDS
> };
> diff --git a/lib/actions.c b/lib/actions.c
> index 6a3b1ba87..e14907e3d 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -3056,6 +3056,7 @@ format_OVNFIELD_LOAD(const struct ovnact_load *load
> , struct ds *s)
> const struct ovn_field *f =
> ovn_field_from_name(load->dst.symbol->name);
> switch (f->id) {
> case OVN_ICMP4_FRAG_MTU:
> + case OVN_ICMP6_FRAG_MTU:
> ds_put_format(s, "%s = %u;", f->name,
> ntohs(load->imm.value.be16_int));
> break;
> @@ -3075,12 +3076,20 @@ encode_OVNFIELD_LOAD(const struct ovnact_load
> *load,
> switch (f->id) {
> case OVN_ICMP4_FRAG_MTU: {
> size_t oc_offset = encode_start_controller_op(
> - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, NX_CTLR_NO_METER,
> - ofpacts);
> + ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true,
> + NX_CTLR_NO_METER, ofpacts);
> ofpbuf_put(ofpacts, &load->imm.value.be16_int, sizeof(ovs_be16));
> encode_finish_controller_op(oc_offset, ofpacts);
> break;
> }
> + case OVN_ICMP6_FRAG_MTU: {
> + size_t oc_offset = encode_start_controller_op(
> + ACTION_OPCODE_PUT_ICMP6_FRAG_MTU, true,
> + NX_CTLR_NO_METER, ofpacts);
> + ofpbuf_put(ofpacts, &load->imm.value.be32_int, sizeof(ovs_be32));
> + encode_finish_controller_op(oc_offset, ofpacts);
> + break;
> + }
> case OVN_FIELD_N_IDS:
> default:
> OVS_NOT_REACHED();
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index 8ad56aa53..8639523ea 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -29,6 +29,10 @@ const struct ovn_field ovn_fields[OVN_FIELD_N_IDS] = {
> OVN_ICMP4_FRAG_MTU,
> "icmp4.frag_mtu",
> 2, 16,
> + }, {
> + OVN_ICMP6_FRAG_MTU,
> + "icmp6.frag_mtu",
> + 4, 32,
> },
> };
>
> @@ -257,6 +261,7 @@ ovn_init_symtab(struct shash *symtab)
> expr_symtab_add_field(symtab, "pkt.mark", MFF_PKT_MARK, NULL, false);
>
> expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu",
> OVN_ICMP4_FRAG_MTU);
> + expr_symtab_add_ovn_field(symtab, "icmp6.frag_mtu",
> OVN_ICMP6_FRAG_MTU);
> }
>
> const char *
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index bc69b58c0..a626dbba8 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1170,10 +1170,11 @@
> <ul>
> <li>
> <code>icmp4.frag_mtu</code>
> + <code>icmp6.frag_mtu</code>
> <p>
> - This field sets the low-order 16 bits of the ICMP4 header
> field
> - that is labelled "unused" in the ICMP specification as
> defined
> - in the RFC 1191 with the value specified in
> + This field sets the low-order 16 bits of the ICMP{4,6}
> header
> + field that is labelled "unused" in the ICMP specification
> as
> + defined in the RFC 1191 with the value specified in
> <var>constant</var>.
> </p>
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 61132c291..124dd78d2 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1510,6 +1510,14 @@ icmp6_error { };
> encodes as controller(userdata=00.00.00.14.00.00.00.00)
> has prereqs ip6
>
> +# icmp6_error with icmp6.frag_mtu
> +icmp6_error { eth.dst = ff:ff:ff:ff:ff:ff; icmp6.frag_mtu = 1500; 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.28.00.00.23.20.00.25.00.00.00.00.00.00.00.03.00.10.00.00.00.15.00.00.00.00.00.00.05.dc.00.04.00.04.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64)
> + has prereqs ip6
> +
> +icmp6.frag_mtu = 1500;
> + encodes as
> controller(userdata=00.00.00.15.00.00.00.00.00.00.05.dc,pause)
> +
> # 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 d1ab051ce..de7508851 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -2081,7 +2081,12 @@ execute_ovnfield_load(const struct ovnact_load
> *load,
> ntohs(load->imm.value.be16_int));
> break;
> }
> -
> + case OVN_ICMP6_FRAG_MTU: {
> + ovntrace_node_append(super, OVNTRACE_NODE_MODIFY,
> + "icmp6.frag_mtu = %u",
> + ntohs(load->imm.value.be16_int));
> + break;
> + }
> case OVN_FIELD_N_IDS:
> default:
> OVS_NOT_REACHED();
> --
> 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