[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