[ovs-dev] [PATCH ovn 2/3] Introduce icmp6.frag_mtu action
Numan Siddique
numans at ovn.org
Tue Jul 7 10:00:29 UTC 2020
On Tue, Jul 7, 2020 at 2:33 PM Numan Siddique <numans at ovn.org> wrote:
>
>
> 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 ?
>
>
My bad. You're doing that in patch 3. I see now.
Thanks
Numan
> 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