| Message ID | 20231214152954.87316-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] controller: Avoid double controller action for ICMP errors | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 12/14/23 16:29, Ales Musil wrote: > The fields that are not directly supported by OvS were encoded > via additional controller action that changed the required value. > This was most notably needed for ICMP need frag messages. > > Encode the field value loads as note action instead. This allows > us to find the note and act accordingly in the first controller > action without the need to send it to pinctrl again, parse and > change the packet. This way we can also encode any future fields > that might be needed as this method should be flexible enough. > > This change is completely transparent to the user and shouldn't > cause any disruptions. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Fix the wrong checksum for the ICMP packet. > --- Hi Ales, Thanks for the patch, I have a few comments though. > controller/physical.c | 14 ++-- > controller/pinctrl.c | 183 +++++++++++++++++++++--------------------- > include/ovn/actions.h | 16 ++++ > lib/actions.c | 44 +++++++--- > tests/ovn.at | 10 +-- > 5 files changed, 154 insertions(+), 113 deletions(-) > > diff --git a/controller/physical.c b/controller/physical.c > index ce3b88d16..161b4f073 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -1222,7 +1222,7 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, > ip_ttl->ttl = 255; > > uint16_t frag_mtu = mtu - ETHERNET_OVERHEAD; > - size_t frag_mtu_oc_offset; > + size_t note_offset; > if (is_ipv6) { > /* icmp6.type = 2 (Packet Too Big) */ > /* icmp6.code = 0 */ > @@ -1234,9 +1234,8 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, > &inner_ofpacts, mf_from_id(MFF_ICMPV6_CODE), &icmp_code, NULL); > > /* icmp6.frag_mtu */ > - frag_mtu_oc_offset = encode_start_controller_op( > - ACTION_OPCODE_PUT_ICMP6_FRAG_MTU, true, NX_CTLR_NO_METER, > - &inner_ofpacts); > + note_offset = encode_start_ovn_field_note(OVN_ICMP6_FRAG_MTU, > + &inner_ofpacts); > ovs_be32 frag_mtu_ovs = htonl(frag_mtu); > ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); > } else { > @@ -1250,13 +1249,12 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, > &inner_ofpacts, mf_from_id(MFF_ICMPV4_CODE), &icmp_code, NULL); > > /* icmp4.frag_mtu = */ > - frag_mtu_oc_offset = encode_start_controller_op( > - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, NX_CTLR_NO_METER, > - &inner_ofpacts); > + note_offset = encode_start_ovn_field_note(OVN_ICMP4_FRAG_MTU, > + &inner_ofpacts); > ovs_be16 frag_mtu_ovs = htons(frag_mtu); > ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); > } > - encode_finish_controller_op(frag_mtu_oc_offset, &inner_ofpacts); > + encode_finish_ovn_field_note(note_offset, &inner_ofpacts); > > /* Finally, submit the ICMP error back to the ingress pipeline */ > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &inner_ofpacts); > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 5a35d56f6..7925f4c92 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -259,12 +259,7 @@ 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_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); > @@ -606,6 +601,23 @@ set_switch_config(struct rconn *swconn, > queue_msg(swconn, request); > } > > +static void > +enqueue_packet(struct rconn *swconn, enum ofp_version version, > + const struct dp_packet *packet, const struct ofpbuf *ofpacts) > +{ > + struct ofputil_packet_out po = { Nit: (struct ofputil_packet_out) { > + .packet = dp_packet_data(packet), > + .packet_len = dp_packet_size(packet), > + .buffer_id = UINT32_MAX, > + .ofpacts = ofpacts->data, > + .ofpacts_len = ofpacts->size, Nit: 4 spaces too much for indentation. > + }; > + > + match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > + enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version); > + queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > +} > + > static void > set_actions_and_enqueue_msg(struct rconn *swconn, > const struct dp_packet *packet, > @@ -631,19 +643,59 @@ set_actions_and_enqueue_msg(struct rconn *swconn, > return; > } > > - struct ofputil_packet_out po = { > - .packet = dp_packet_data(packet), > - .packet_len = dp_packet_size(packet), > - .buffer_id = UINT32_MAX, > - .ofpacts = ofpacts.data, > - .ofpacts_len = ofpacts.size, > - }; > - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > - enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version); > - queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > + enqueue_packet(swconn, version, packet, &ofpacts); > ofpbuf_uninit(&ofpacts); > } > > +static bool > +ofpacts_decode_and_reload_metadata(enum ofp_version version, > + const struct match *md, > + struct ofpbuf *userdata, > + struct ofpbuf *ofpacts) > +{ > + /* Copy metadata from 'md' into the packet-out via "set_field" > + * actions, then add actions from 'userdata'. > + */ > + reload_metadata(ofpacts, md); > + enum ofperr error = ofpacts_pull_openflow_actions(userdata, userdata->size, > + version, NULL, NULL, > + ofpacts); > + if (error) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_WARN_RL(&rl, "failed to parse actions from userdata (%s)", > + ofperr_to_string(error)); > + return false; > + } > + > + return true; > +} > + > +static void * > +ofpacts_get_ovn_field(const struct ofpbuf *ofpacts, > + enum ovn_field_id id) > +{ > + const struct ovn_field *field = ovn_field_from_id(id); > + > + struct ofpact *ofpact; > + OFPACT_FOR_EACH (ofpact, ofpacts->data, ofpacts->size) { > + if (ofpact->type != OFPACT_NOTE) { > + continue; > + } > + > + struct ofpact_note *note = ofpact_get_NOTE(ofpact); > + struct ovn_field_note_header *hdr = (void *) note->data; > + /* The data can contain padding bytes from NXAST_NOTE encode/decode. */ > + size_t data_len = note->length - sizeof *hdr; > + > + if (!strncmp(hdr->magic, OVN_FIELD_NOTE_MAGIC, sizeof hdr->magic) && > + field->id == ntohs(hdr->type) && data_len >= field->n_bytes) { > + return hdr->data; > + } > + } > + > + return NULL; > +} > + > /* Forwards a packet to 'out_port_key' even if that's on a remote > * hypervisor, i.e., the packet is re-injected in table OFTABLE_OUTPUT_INIT. > */ > @@ -1546,6 +1598,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > const struct match *md, struct ofpbuf *userdata, > bool set_icmp_code, bool loopback) > { > + enum ofp_version version = rconn_get_version(swconn); > + > /* This action only works for IP packets, and the switch should only send > * us IP packets this way, but check here just to be sure. */ > if (ip_flow->dl_type != htons(ETH_TYPE_IP) && > @@ -1557,6 +1611,14 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > return; > } > > + uint64_t ofpacts_stub[4096 / 8]; > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); > + > + if (!ofpacts_decode_and_reload_metadata(version, md, userdata, &ofpacts)) { > + ofpbuf_uninit(&ofpacts); > + return; > + } > + > uint64_t packet_stub[128 / 8]; > struct dp_packet packet; > > @@ -1572,6 +1634,7 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > VLOG_WARN_RL(&rl, > "ICMP action on IP packet with invalid length (%u)", > in_ip_len); > + ofpbuf_uninit(&ofpacts); > return; > } > > @@ -1615,6 +1678,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > void *data = ih + 1; > memcpy(data, in_ip, in_ip_len); > > + ovs_be16 *mtu = ofpacts_get_ovn_field(&ofpacts, OVN_ICMP4_FRAG_MTU); Doesn't this temporarily disrupt upgrades? Shouldn't we try to detect if this userdata is indeed using the new note action? > + if (mtu) { > + ih->icmp_fields.frag.mtu = *mtu; > + ih->icmp_code = 4; > + } > + > ih->icmp_csum = 0; > ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len); > } else { > @@ -1662,6 +1731,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > } > ih->icmp6_base.icmp6_cksum = 0; > > + ovs_be32 *mtu = ofpacts_get_ovn_field(&ofpacts, OVN_ICMP6_FRAG_MTU); > + if (mtu) { > + put_16aligned_be32(ih->icmp6_data.be32, *mtu); > + ih->icmp6_base.icmp6_type = ICMP6_PACKET_TOO_BIG; > + } > + > void *data = ih + 1; > memcpy(data, in_ip, in_ip_len); > uint32_t icmpv6_csum = > @@ -1676,8 +1751,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, > ip_flow->vlans[0].tci); > } > > - set_actions_and_enqueue_msg(swconn, &packet, md, userdata); > + enqueue_packet(swconn, version, &packet, &ofpacts); > dp_packet_uninit(&packet); > + ofpbuf_uninit(&ofpacts); > } > > /* Called with in the pinctrl_handler thread context. */ > @@ -3285,12 +3361,6 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg) > &userdata); > break; > > - case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU: > - case ACTION_OPCODE_PUT_ICMP6_FRAG_MTU: > - pinctrl_handle_put_icmp_frag_mtu(swconn, &headers, &packet, &pin, > - &userdata, &continuation); > - break; > - > case ACTION_OPCODE_EVENT: > ovs_mutex_lock(&pinctrl_mutex); > pinctrl_handle_event(&userdata); > @@ -6312,73 +6382,6 @@ exit: > dp_packet_uninit(pkt_out_ptr); > } > > -/* Called with in the pinctrl_handler thread context. */ > -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) > -{ > - 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/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_icmp(4/6)_frag_mtu action on non-ICMPv4/v6 packet"); > - 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; > - > - 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); > - > - /* 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); > - > -exit: > - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto)); > - if (pkt_out) { > - dp_packet_delete(pkt_out); > - } > -} > - > static void > wait_controller_event(struct ovsdb_idl_txn *ovnsb_idl_txn) > { > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 49cfe0624..70c11ff7a 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -514,6 +514,18 @@ struct ovnact_commit_lb_aff { > uint16_t timeout; > }; > > +#define OVN_FIELD_NOTE_MAGIC "ovn" > + > +struct ovn_field_note_header { > + char magic[4]; > + uint8_t pad[2]; > + ovs_be16 type; /* The type of ovn field note, based > + * on 'enum ovn_field_id'. */ > + uint8_t data[]; > +}; > + To make sure we always have the right padding this should be: struct ovn_field_note_header { OFPACT_PADDED_MEMBERS( char magic[4]; ovs_be16 type; /* The type of ovn field note, based * on 'enum ovn_field_id'. */ ); uint8_t data[]; }; > +BUILD_ASSERT_DECL(sizeof(struct ovn_field_note_header) == 8); > + > /* Internal use by the helpers below. */ > void ovnact_init(struct ovnact *, enum ovnact_type, size_t len); > void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len); > @@ -901,4 +913,8 @@ void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); > void encode_controller_op(enum action_opcode opcode, uint32_t meter_id, > struct ofpbuf *ofpacts); > > +size_t encode_start_ovn_field_note(enum ovn_field_id id, Nit: 'id' is not really needed in the prototype and without it this fits on a single line. > + struct ofpbuf *ofpacts); > +void encode_finish_ovn_field_note(size_t offset, struct ofpbuf *ofpacts); > + > #endif /* ovn/actions.h */ > diff --git a/lib/actions.c b/lib/actions.c > index a73fe1a1e..f1facc6ef 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -117,6 +117,34 @@ encode_controller_op(enum action_opcode opcode, uint32_t meter_id, > encode_finish_controller_op(ofs, ofpacts); > } > > +size_t > +encode_start_ovn_field_note(enum ovn_field_id id, struct ofpbuf *ofpacts) > +{ Nit: With "struct ovn_field_note_header *hdr;" here we'd avoid the not so pretty newline below where we set 'hdr'. > + size_t offset = ofpacts->size; > + > + ofpact_put_NOTE(ofpacts); > + struct ovn_field_note_header *hdr = ofpbuf_put_uninit(ofpacts, > + sizeof *hdr); If we remove the explicit 'pad' and rely on OFPACT_PADDED_MEMBERS in the structure definition then we should use ofpbuf_put_zeros() here. At least to make the unit tests happy. > + *hdr = (struct ovn_field_note_header) { > + .magic = OVN_FIELD_NOTE_MAGIC, > + .pad = {0}, > + .type = htons(id), Nit: 4 spaces too much for indentation. > + }; > + > + return offset; > +} > + > +void > +encode_finish_ovn_field_note(size_t offset, struct ofpbuf *ofpacts) > +{ > + struct ofpact_note *note = ofpbuf_at_assert(ofpacts, offset, sizeof *note); > + > + ofpacts->header = note; > + note->length = ofpacts->size - (offset + sizeof *note); > + > + ofpact_finish_NOTE(ofpacts, ¬e); > +} > + > static void > init_stack(struct ofpact_stack *stack, enum mf_field_id field) > { > @@ -3757,31 +3785,27 @@ format_OVNFIELD_LOAD(const struct ovnact_load *load , struct ds *s) > > static void > encode_OVNFIELD_LOAD(const struct ovnact_load *load, > - const struct ovnact_encode_params *ep, > - struct ofpbuf *ofpacts) > + const struct ovnact_encode_params *ep OVS_UNUSED, > + struct ofpbuf *ofpacts) > { > const struct ovn_field *f = ovn_field_from_name(load->dst.symbol->name); > + size_t offset = encode_start_ovn_field_note(f->id, ofpacts); > + > switch (f->id) { > case OVN_ICMP4_FRAG_MTU: { > - size_t oc_offset = encode_start_controller_op( > - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, > - ep->ctrl_meter_id, 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, > - ep->ctrl_meter_id, 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(); > } > + > + encode_finish_ovn_field_note(offset, ofpacts); > } > > static void > diff --git a/tests/ovn.at b/tests/ovn.at > index 918f97a9e..968997d1d 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1867,7 +1867,7 @@ icmp4 { }; > > # icmp4 with icmp4.frag_mtu > icmp4 { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output; > - encodes as controller(userdata=00.00.00.0a.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) > + encodes as controller(userdata=00.00.00.0a.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) > has prereqs ip4 > > # icmp4_error > @@ -1882,11 +1882,11 @@ icmp4_error { }; > > # icmp4_error with icmp4.frag_mtu > icmp4_error { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output; > - encodes as controller(userdata=00.00.00.0e.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) > + encodes as controller(userdata=00.00.00.0e.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) > has prereqs ip4 > > icmp4.frag_mtu = 1500; > - encodes as controller(userdata=00.00.00.0d.00.00.00.00.05.dc,pause) > + encodes as note:6f.76.6e.00.00.00.00.00.05.dc > > # icmp6 > icmp6 { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; > @@ -1910,11 +1910,11 @@ icmp6_error { }; > > # 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) > + 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.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.01.00.00.05.dc.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) > + encodes as note:6f.76.6e.00.00.00.00.01.00.00.05.dc > > # tcp_reset > tcp_reset { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; Regards, Dumitru
On Mon, Feb 5, 2024 at 11:03 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 12/14/23 16:29, Ales Musil wrote: > > The fields that are not directly supported by OvS were encoded > > via additional controller action that changed the required value. > > This was most notably needed for ICMP need frag messages. > > > > Encode the field value loads as note action instead. This allows > > us to find the note and act accordingly in the first controller > > action without the need to send it to pinctrl again, parse and > > change the packet. This way we can also encode any future fields > > that might be needed as this method should be flexible enough. > > > > This change is completely transparent to the user and shouldn't > > cause any disruptions. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v2: Fix the wrong checksum for the ICMP packet. > > --- > > Hi Ales, > > Thanks for the patch, I have a few comments though. > Hi Dumitru, thank you for the review. > > controller/physical.c | 14 ++-- > > controller/pinctrl.c | 183 +++++++++++++++++++++--------------------- > > include/ovn/actions.h | 16 ++++ > > lib/actions.c | 44 +++++++--- > > tests/ovn.at | 10 +-- > > 5 files changed, 154 insertions(+), 113 deletions(-) > > > > diff --git a/controller/physical.c b/controller/physical.c > > index ce3b88d16..161b4f073 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -1222,7 +1222,7 @@ reply_imcp_error_if_pkt_too_big(struct > ovn_desired_flow_table *flow_table, > > ip_ttl->ttl = 255; > > > > uint16_t frag_mtu = mtu - ETHERNET_OVERHEAD; > > - size_t frag_mtu_oc_offset; > > + size_t note_offset; > > if (is_ipv6) { > > /* icmp6.type = 2 (Packet Too Big) */ > > /* icmp6.code = 0 */ > > @@ -1234,9 +1234,8 @@ reply_imcp_error_if_pkt_too_big(struct > ovn_desired_flow_table *flow_table, > > &inner_ofpacts, mf_from_id(MFF_ICMPV6_CODE), &icmp_code, > NULL); > > > > /* icmp6.frag_mtu */ > > - frag_mtu_oc_offset = encode_start_controller_op( > > - ACTION_OPCODE_PUT_ICMP6_FRAG_MTU, true, NX_CTLR_NO_METER, > > - &inner_ofpacts); > > + note_offset = encode_start_ovn_field_note(OVN_ICMP6_FRAG_MTU, > > + &inner_ofpacts); > > ovs_be32 frag_mtu_ovs = htonl(frag_mtu); > > ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); > > } else { > > @@ -1250,13 +1249,12 @@ reply_imcp_error_if_pkt_too_big(struct > ovn_desired_flow_table *flow_table, > > &inner_ofpacts, mf_from_id(MFF_ICMPV4_CODE), &icmp_code, > NULL); > > > > /* icmp4.frag_mtu = */ > > - frag_mtu_oc_offset = encode_start_controller_op( > > - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, NX_CTLR_NO_METER, > > - &inner_ofpacts); > > + note_offset = encode_start_ovn_field_note(OVN_ICMP4_FRAG_MTU, > > + &inner_ofpacts); > > ovs_be16 frag_mtu_ovs = htons(frag_mtu); > > ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); > > } > > - encode_finish_controller_op(frag_mtu_oc_offset, &inner_ofpacts); > > + encode_finish_ovn_field_note(note_offset, &inner_ofpacts); > > > > /* Finally, submit the ICMP error back to the ingress pipeline */ > > put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &inner_ofpacts); > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 5a35d56f6..7925f4c92 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -259,12 +259,7 @@ 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_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); > > @@ -606,6 +601,23 @@ set_switch_config(struct rconn *swconn, > > queue_msg(swconn, request); > > } > > > > +static void > > +enqueue_packet(struct rconn *swconn, enum ofp_version version, > > + const struct dp_packet *packet, const struct ofpbuf > *ofpacts) > > +{ > > + struct ofputil_packet_out po = { > > Nit: (struct ofputil_packet_out) { > > > + .packet = dp_packet_data(packet), > > + .packet_len = dp_packet_size(packet), > > + .buffer_id = UINT32_MAX, > > + .ofpacts = ofpacts->data, > > + .ofpacts_len = ofpacts->size, > > Nit: 4 spaces too much for indentation. > > > + }; > > + > > + match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > > + enum ofputil_protocol proto = > ofputil_protocol_from_ofp_version(version); > > + queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > > +} > > + > > static void > > set_actions_and_enqueue_msg(struct rconn *swconn, > > const struct dp_packet *packet, > > @@ -631,19 +643,59 @@ set_actions_and_enqueue_msg(struct rconn *swconn, > > return; > > } > > > > - struct ofputil_packet_out po = { > > - .packet = dp_packet_data(packet), > > - .packet_len = dp_packet_size(packet), > > - .buffer_id = UINT32_MAX, > > - .ofpacts = ofpacts.data, > > - .ofpacts_len = ofpacts.size, > > - }; > > - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); > > - enum ofputil_protocol proto = > ofputil_protocol_from_ofp_version(version); > > - queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); > > + enqueue_packet(swconn, version, packet, &ofpacts); > > ofpbuf_uninit(&ofpacts); > > } > > > > +static bool > > +ofpacts_decode_and_reload_metadata(enum ofp_version version, > > + const struct match *md, > > + struct ofpbuf *userdata, > > + struct ofpbuf *ofpacts) > > +{ > > + /* Copy metadata from 'md' into the packet-out via "set_field" > > + * actions, then add actions from 'userdata'. > > + */ > > + reload_metadata(ofpacts, md); > > + enum ofperr error = ofpacts_pull_openflow_actions(userdata, > userdata->size, > > + version, NULL, > NULL, > > + ofpacts); > > + if (error) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > + VLOG_WARN_RL(&rl, "failed to parse actions from userdata (%s)", > > + ofperr_to_string(error)); > > + return false; > > + } > > + > > + return true; > > +} > > + > > +static void * > > +ofpacts_get_ovn_field(const struct ofpbuf *ofpacts, > > + enum ovn_field_id id) > > +{ > > + const struct ovn_field *field = ovn_field_from_id(id); > > + > > + struct ofpact *ofpact; > > + OFPACT_FOR_EACH (ofpact, ofpacts->data, ofpacts->size) { > > + if (ofpact->type != OFPACT_NOTE) { > > + continue; > > + } > > + > > + struct ofpact_note *note = ofpact_get_NOTE(ofpact); > > + struct ovn_field_note_header *hdr = (void *) note->data; > > + /* The data can contain padding bytes from NXAST_NOTE > encode/decode. */ > > + size_t data_len = note->length - sizeof *hdr; > > + > > + if (!strncmp(hdr->magic, OVN_FIELD_NOTE_MAGIC, sizeof > hdr->magic) && > > + field->id == ntohs(hdr->type) && data_len >= > field->n_bytes) { > > + return hdr->data; > > + } > > + } > > + > > + return NULL; > > +} > > + > > /* Forwards a packet to 'out_port_key' even if that's on a remote > > * hypervisor, i.e., the packet is re-injected in table > OFTABLE_OUTPUT_INIT. > > */ > > @@ -1546,6 +1598,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > const struct match *md, struct ofpbuf *userdata, > > bool set_icmp_code, bool loopback) > > { > > + enum ofp_version version = rconn_get_version(swconn); > > + > > /* This action only works for IP packets, and the switch should > only send > > * us IP packets this way, but check here just to be sure. */ > > if (ip_flow->dl_type != htons(ETH_TYPE_IP) && > > @@ -1557,6 +1611,14 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > return; > > } > > > > + uint64_t ofpacts_stub[4096 / 8]; > > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); > > + > > + if (!ofpacts_decode_and_reload_metadata(version, md, userdata, > &ofpacts)) { > > + ofpbuf_uninit(&ofpacts); > > + return; > > + } > > + > > uint64_t packet_stub[128 / 8]; > > struct dp_packet packet; > > > > @@ -1572,6 +1634,7 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > VLOG_WARN_RL(&rl, > > "ICMP action on IP packet with invalid length > (%u)", > > in_ip_len); > > + ofpbuf_uninit(&ofpacts); > > return; > > } > > > > @@ -1615,6 +1678,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > void *data = ih + 1; > > memcpy(data, in_ip, in_ip_len); > > > > + ovs_be16 *mtu = ofpacts_get_ovn_field(&ofpacts, > OVN_ICMP4_FRAG_MTU); > > Doesn't this temporarily disrupt upgrades? Shouldn't we try to detect > if this userdata is indeed using the new note action? > > Right, in v3 I have added back the old handler which should take over during the transition. > > > + if (mtu) { > > + ih->icmp_fields.frag.mtu = *mtu; > > + ih->icmp_code = 4; > > + } > > + > > ih->icmp_csum = 0; > > ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len); > > } else { > > @@ -1662,6 +1731,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > } > > ih->icmp6_base.icmp6_cksum = 0; > > > > + ovs_be32 *mtu = ofpacts_get_ovn_field(&ofpacts, > OVN_ICMP6_FRAG_MTU); > > + if (mtu) { > > + put_16aligned_be32(ih->icmp6_data.be32, *mtu); > > + ih->icmp6_base.icmp6_type = ICMP6_PACKET_TOO_BIG; > > + } > > + > > void *data = ih + 1; > > memcpy(data, in_ip, in_ip_len); > > uint32_t icmpv6_csum = > > @@ -1676,8 +1751,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const > struct flow *ip_flow, > > ip_flow->vlans[0].tci); > > } > > > > - set_actions_and_enqueue_msg(swconn, &packet, md, userdata); > > + enqueue_packet(swconn, version, &packet, &ofpacts); > > dp_packet_uninit(&packet); > > + ofpbuf_uninit(&ofpacts); > > } > > > > /* Called with in the pinctrl_handler thread context. */ > > @@ -3285,12 +3361,6 @@ process_packet_in(struct rconn *swconn, const > struct ofp_header *msg) > > &userdata); > > break; > > > > - case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU: > > - case ACTION_OPCODE_PUT_ICMP6_FRAG_MTU: > > - pinctrl_handle_put_icmp_frag_mtu(swconn, &headers, &packet, > &pin, > > - &userdata, &continuation); > > - break; > > - > > case ACTION_OPCODE_EVENT: > > ovs_mutex_lock(&pinctrl_mutex); > > pinctrl_handle_event(&userdata); > > @@ -6312,73 +6382,6 @@ exit: > > dp_packet_uninit(pkt_out_ptr); > > } > > > > -/* Called with in the pinctrl_handler thread context. */ > > -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) > > -{ > > - 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/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_icmp(4/6)_frag_mtu action on non-ICMPv4/v6 > packet"); > > - 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; > > - > > - 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); > > - > > - /* 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); > > - > > -exit: > > - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto)); > > - if (pkt_out) { > > - dp_packet_delete(pkt_out); > > - } > > -} > > - > > static void > > wait_controller_event(struct ovsdb_idl_txn *ovnsb_idl_txn) > > { > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > > index 49cfe0624..70c11ff7a 100644 > > --- a/include/ovn/actions.h > > +++ b/include/ovn/actions.h > > @@ -514,6 +514,18 @@ struct ovnact_commit_lb_aff { > > uint16_t timeout; > > }; > > > > +#define OVN_FIELD_NOTE_MAGIC "ovn" > > + > > +struct ovn_field_note_header { > > + char magic[4]; > > + uint8_t pad[2]; > > + ovs_be16 type; /* The type of ovn field note, based > > + * on 'enum ovn_field_id'. */ > > + uint8_t data[]; > > +}; > > + > > To make sure we always have the right padding this should be: > > struct ovn_field_note_header { > OFPACT_PADDED_MEMBERS( > char magic[4]; > ovs_be16 type; /* The type of ovn field note, based > * on 'enum ovn_field_id'. */ > ); > uint8_t data[]; > }; > > > +BUILD_ASSERT_DECL(sizeof(struct ovn_field_note_header) == 8); > > + > > /* Internal use by the helpers below. */ > > void ovnact_init(struct ovnact *, enum ovnact_type, size_t len); > > void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len); > > @@ -901,4 +913,8 @@ void encode_finish_controller_op(size_t ofs, struct > ofpbuf *ofpacts); > > void encode_controller_op(enum action_opcode opcode, uint32_t meter_id, > > struct ofpbuf *ofpacts); > > > > +size_t encode_start_ovn_field_note(enum ovn_field_id id, > > Nit: 'id' is not really needed in the prototype and without it this fits > on a single line. > > > + struct ofpbuf *ofpacts); > > +void encode_finish_ovn_field_note(size_t offset, struct ofpbuf > *ofpacts); > > + > > #endif /* ovn/actions.h */ > > diff --git a/lib/actions.c b/lib/actions.c > > index a73fe1a1e..f1facc6ef 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -117,6 +117,34 @@ encode_controller_op(enum action_opcode opcode, > uint32_t meter_id, > > encode_finish_controller_op(ofs, ofpacts); > > } > > > > +size_t > > +encode_start_ovn_field_note(enum ovn_field_id id, struct ofpbuf > *ofpacts) > > +{ > > Nit: With "struct ovn_field_note_header *hdr;" here we'd avoid the not > so pretty newline below where we set 'hdr'. > > > + size_t offset = ofpacts->size; > > + > > + ofpact_put_NOTE(ofpacts); > > + struct ovn_field_note_header *hdr = ofpbuf_put_uninit(ofpacts, > > + sizeof *hdr); > > If we remove the explicit 'pad' and rely on OFPACT_PADDED_MEMBERS in the > structure definition then we should use ofpbuf_put_zeros() here. At > least to make the unit tests happy. > > > + *hdr = (struct ovn_field_note_header) { > > + .magic = OVN_FIELD_NOTE_MAGIC, > > + .pad = {0}, > > + .type = htons(id), > > Nit: 4 spaces too much for indentation. > > > + }; > > + > > + return offset; > > +} > > + > > +void > > +encode_finish_ovn_field_note(size_t offset, struct ofpbuf *ofpacts) > > +{ > > + struct ofpact_note *note = ofpbuf_at_assert(ofpacts, offset, sizeof > *note); > > + > > + ofpacts->header = note; > > + note->length = ofpacts->size - (offset + sizeof *note); > > + > > + ofpact_finish_NOTE(ofpacts, ¬e); > > +} > > + > > static void > > init_stack(struct ofpact_stack *stack, enum mf_field_id field) > > { > > @@ -3757,31 +3785,27 @@ format_OVNFIELD_LOAD(const struct ovnact_load > *load , struct ds *s) > > > > static void > > encode_OVNFIELD_LOAD(const struct ovnact_load *load, > > - const struct ovnact_encode_params *ep, > > - struct ofpbuf *ofpacts) > > + const struct ovnact_encode_params *ep OVS_UNUSED, > > + struct ofpbuf *ofpacts) > > { > > const struct ovn_field *f = > ovn_field_from_name(load->dst.symbol->name); > > + size_t offset = encode_start_ovn_field_note(f->id, ofpacts); > > + > > switch (f->id) { > > case OVN_ICMP4_FRAG_MTU: { > > - size_t oc_offset = encode_start_controller_op( > > - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, > > - ep->ctrl_meter_id, 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, > > - ep->ctrl_meter_id, 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(); > > } > > + > > + encode_finish_ovn_field_note(offset, ofpacts); > > } > > > > static void > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 918f97a9e..968997d1d 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -1867,7 +1867,7 @@ icmp4 { }; > > > > # icmp4 with icmp4.frag_mtu > > icmp4 { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; > output; > > - encodes as > controller(userdata=00.00.00.0a.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) > > + encodes as > controller(userdata=00.00.00.0a.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) > > has prereqs ip4 > > > > # icmp4_error > > @@ -1882,11 +1882,11 @@ icmp4_error { }; > > > > # icmp4_error with icmp4.frag_mtu > > icmp4_error { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; > output; }; output; > > - encodes as > controller(userdata=00.00.00.0e.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) > > + encodes as > controller(userdata=00.00.00.0e.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) > > has prereqs ip4 > > > > icmp4.frag_mtu = 1500; > > - encodes as controller(userdata=00.00.00.0d.00.00.00.00.05.dc,pause) > > + encodes as note:6f.76.6e.00.00.00.00.00.05.dc > > > > # icmp6 > > icmp6 { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; > > @@ -1910,11 +1910,11 @@ icmp6_error { }; > > > > # 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) > > + 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.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.01.00.00.05.dc.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) > > + encodes as note:6f.76.6e.00.00.00.00.01.00.00.05.dc > > > > # tcp_reset > > tcp_reset { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; > > Regards, > Dumitru > > Everything else should be addressed in v3. Thanks, Ales
diff --git a/controller/physical.c b/controller/physical.c index ce3b88d16..161b4f073 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -1222,7 +1222,7 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, ip_ttl->ttl = 255; uint16_t frag_mtu = mtu - ETHERNET_OVERHEAD; - size_t frag_mtu_oc_offset; + size_t note_offset; if (is_ipv6) { /* icmp6.type = 2 (Packet Too Big) */ /* icmp6.code = 0 */ @@ -1234,9 +1234,8 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, &inner_ofpacts, mf_from_id(MFF_ICMPV6_CODE), &icmp_code, NULL); /* icmp6.frag_mtu */ - frag_mtu_oc_offset = encode_start_controller_op( - ACTION_OPCODE_PUT_ICMP6_FRAG_MTU, true, NX_CTLR_NO_METER, - &inner_ofpacts); + note_offset = encode_start_ovn_field_note(OVN_ICMP6_FRAG_MTU, + &inner_ofpacts); ovs_be32 frag_mtu_ovs = htonl(frag_mtu); ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); } else { @@ -1250,13 +1249,12 @@ reply_imcp_error_if_pkt_too_big(struct ovn_desired_flow_table *flow_table, &inner_ofpacts, mf_from_id(MFF_ICMPV4_CODE), &icmp_code, NULL); /* icmp4.frag_mtu = */ - frag_mtu_oc_offset = encode_start_controller_op( - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, NX_CTLR_NO_METER, - &inner_ofpacts); + note_offset = encode_start_ovn_field_note(OVN_ICMP4_FRAG_MTU, + &inner_ofpacts); ovs_be16 frag_mtu_ovs = htons(frag_mtu); ofpbuf_put(&inner_ofpacts, &frag_mtu_ovs, sizeof(frag_mtu_ovs)); } - encode_finish_controller_op(frag_mtu_oc_offset, &inner_ofpacts); + encode_finish_ovn_field_note(note_offset, &inner_ofpacts); /* Finally, submit the ICMP error back to the ingress pipeline */ put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &inner_ofpacts); diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 5a35d56f6..7925f4c92 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -259,12 +259,7 @@ 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_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); @@ -606,6 +601,23 @@ set_switch_config(struct rconn *swconn, queue_msg(swconn, request); } +static void +enqueue_packet(struct rconn *swconn, enum ofp_version version, + const struct dp_packet *packet, const struct ofpbuf *ofpacts) +{ + struct ofputil_packet_out po = { + .packet = dp_packet_data(packet), + .packet_len = dp_packet_size(packet), + .buffer_id = UINT32_MAX, + .ofpacts = ofpacts->data, + .ofpacts_len = ofpacts->size, + }; + + match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); + enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version); + queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); +} + static void set_actions_and_enqueue_msg(struct rconn *swconn, const struct dp_packet *packet, @@ -631,19 +643,59 @@ set_actions_and_enqueue_msg(struct rconn *swconn, return; } - struct ofputil_packet_out po = { - .packet = dp_packet_data(packet), - .packet_len = dp_packet_size(packet), - .buffer_id = UINT32_MAX, - .ofpacts = ofpacts.data, - .ofpacts_len = ofpacts.size, - }; - match_set_in_port(&po.flow_metadata, OFPP_CONTROLLER); - enum ofputil_protocol proto = ofputil_protocol_from_ofp_version(version); - queue_msg(swconn, ofputil_encode_packet_out(&po, proto)); + enqueue_packet(swconn, version, packet, &ofpacts); ofpbuf_uninit(&ofpacts); } +static bool +ofpacts_decode_and_reload_metadata(enum ofp_version version, + const struct match *md, + struct ofpbuf *userdata, + struct ofpbuf *ofpacts) +{ + /* Copy metadata from 'md' into the packet-out via "set_field" + * actions, then add actions from 'userdata'. + */ + reload_metadata(ofpacts, md); + enum ofperr error = ofpacts_pull_openflow_actions(userdata, userdata->size, + version, NULL, NULL, + ofpacts); + if (error) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "failed to parse actions from userdata (%s)", + ofperr_to_string(error)); + return false; + } + + return true; +} + +static void * +ofpacts_get_ovn_field(const struct ofpbuf *ofpacts, + enum ovn_field_id id) +{ + const struct ovn_field *field = ovn_field_from_id(id); + + struct ofpact *ofpact; + OFPACT_FOR_EACH (ofpact, ofpacts->data, ofpacts->size) { + if (ofpact->type != OFPACT_NOTE) { + continue; + } + + struct ofpact_note *note = ofpact_get_NOTE(ofpact); + struct ovn_field_note_header *hdr = (void *) note->data; + /* The data can contain padding bytes from NXAST_NOTE encode/decode. */ + size_t data_len = note->length - sizeof *hdr; + + if (!strncmp(hdr->magic, OVN_FIELD_NOTE_MAGIC, sizeof hdr->magic) && + field->id == ntohs(hdr->type) && data_len >= field->n_bytes) { + return hdr->data; + } + } + + return NULL; +} + /* Forwards a packet to 'out_port_key' even if that's on a remote * hypervisor, i.e., the packet is re-injected in table OFTABLE_OUTPUT_INIT. */ @@ -1546,6 +1598,8 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, const struct match *md, struct ofpbuf *userdata, bool set_icmp_code, bool loopback) { + enum ofp_version version = rconn_get_version(swconn); + /* This action only works for IP packets, and the switch should only send * us IP packets this way, but check here just to be sure. */ if (ip_flow->dl_type != htons(ETH_TYPE_IP) && @@ -1557,6 +1611,14 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, return; } + uint64_t ofpacts_stub[4096 / 8]; + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub); + + if (!ofpacts_decode_and_reload_metadata(version, md, userdata, &ofpacts)) { + ofpbuf_uninit(&ofpacts); + return; + } + uint64_t packet_stub[128 / 8]; struct dp_packet packet; @@ -1572,6 +1634,7 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, VLOG_WARN_RL(&rl, "ICMP action on IP packet with invalid length (%u)", in_ip_len); + ofpbuf_uninit(&ofpacts); return; } @@ -1615,6 +1678,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, void *data = ih + 1; memcpy(data, in_ip, in_ip_len); + ovs_be16 *mtu = ofpacts_get_ovn_field(&ofpacts, OVN_ICMP4_FRAG_MTU); + if (mtu) { + ih->icmp_fields.frag.mtu = *mtu; + ih->icmp_code = 4; + } + ih->icmp_csum = 0; ih->icmp_csum = csum(ih, sizeof *ih + in_ip_len); } else { @@ -1662,6 +1731,12 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, } ih->icmp6_base.icmp6_cksum = 0; + ovs_be32 *mtu = ofpacts_get_ovn_field(&ofpacts, OVN_ICMP6_FRAG_MTU); + if (mtu) { + put_16aligned_be32(ih->icmp6_data.be32, *mtu); + ih->icmp6_base.icmp6_type = ICMP6_PACKET_TOO_BIG; + } + void *data = ih + 1; memcpy(data, in_ip, in_ip_len); uint32_t icmpv6_csum = @@ -1676,8 +1751,9 @@ pinctrl_handle_icmp(struct rconn *swconn, const struct flow *ip_flow, ip_flow->vlans[0].tci); } - set_actions_and_enqueue_msg(swconn, &packet, md, userdata); + enqueue_packet(swconn, version, &packet, &ofpacts); dp_packet_uninit(&packet); + ofpbuf_uninit(&ofpacts); } /* Called with in the pinctrl_handler thread context. */ @@ -3285,12 +3361,6 @@ process_packet_in(struct rconn *swconn, const struct ofp_header *msg) &userdata); break; - case ACTION_OPCODE_PUT_ICMP4_FRAG_MTU: - case ACTION_OPCODE_PUT_ICMP6_FRAG_MTU: - pinctrl_handle_put_icmp_frag_mtu(swconn, &headers, &packet, &pin, - &userdata, &continuation); - break; - case ACTION_OPCODE_EVENT: ovs_mutex_lock(&pinctrl_mutex); pinctrl_handle_event(&userdata); @@ -6312,73 +6382,6 @@ exit: dp_packet_uninit(pkt_out_ptr); } -/* Called with in the pinctrl_handler thread context. */ -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) -{ - 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/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_icmp(4/6)_frag_mtu action on non-ICMPv4/v6 packet"); - 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; - - 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); - - /* 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); - -exit: - queue_msg(swconn, ofputil_encode_resume(pin, continuation, proto)); - if (pkt_out) { - dp_packet_delete(pkt_out); - } -} - static void wait_controller_event(struct ovsdb_idl_txn *ovnsb_idl_txn) { diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 49cfe0624..70c11ff7a 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -514,6 +514,18 @@ struct ovnact_commit_lb_aff { uint16_t timeout; }; +#define OVN_FIELD_NOTE_MAGIC "ovn" + +struct ovn_field_note_header { + char magic[4]; + uint8_t pad[2]; + ovs_be16 type; /* The type of ovn field note, based + * on 'enum ovn_field_id'. */ + uint8_t data[]; +}; + +BUILD_ASSERT_DECL(sizeof(struct ovn_field_note_header) == 8); + /* Internal use by the helpers below. */ void ovnact_init(struct ovnact *, enum ovnact_type, size_t len); void *ovnact_put(struct ofpbuf *, enum ovnact_type, size_t len); @@ -901,4 +913,8 @@ void encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts); void encode_controller_op(enum action_opcode opcode, uint32_t meter_id, struct ofpbuf *ofpacts); +size_t encode_start_ovn_field_note(enum ovn_field_id id, + struct ofpbuf *ofpacts); +void encode_finish_ovn_field_note(size_t offset, struct ofpbuf *ofpacts); + #endif /* ovn/actions.h */ diff --git a/lib/actions.c b/lib/actions.c index a73fe1a1e..f1facc6ef 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -117,6 +117,34 @@ encode_controller_op(enum action_opcode opcode, uint32_t meter_id, encode_finish_controller_op(ofs, ofpacts); } +size_t +encode_start_ovn_field_note(enum ovn_field_id id, struct ofpbuf *ofpacts) +{ + size_t offset = ofpacts->size; + + ofpact_put_NOTE(ofpacts); + struct ovn_field_note_header *hdr = ofpbuf_put_uninit(ofpacts, + sizeof *hdr); + *hdr = (struct ovn_field_note_header) { + .magic = OVN_FIELD_NOTE_MAGIC, + .pad = {0}, + .type = htons(id), + }; + + return offset; +} + +void +encode_finish_ovn_field_note(size_t offset, struct ofpbuf *ofpacts) +{ + struct ofpact_note *note = ofpbuf_at_assert(ofpacts, offset, sizeof *note); + + ofpacts->header = note; + note->length = ofpacts->size - (offset + sizeof *note); + + ofpact_finish_NOTE(ofpacts, ¬e); +} + static void init_stack(struct ofpact_stack *stack, enum mf_field_id field) { @@ -3757,31 +3785,27 @@ format_OVNFIELD_LOAD(const struct ovnact_load *load , struct ds *s) static void encode_OVNFIELD_LOAD(const struct ovnact_load *load, - const struct ovnact_encode_params *ep, - struct ofpbuf *ofpacts) + const struct ovnact_encode_params *ep OVS_UNUSED, + struct ofpbuf *ofpacts) { const struct ovn_field *f = ovn_field_from_name(load->dst.symbol->name); + size_t offset = encode_start_ovn_field_note(f->id, ofpacts); + switch (f->id) { case OVN_ICMP4_FRAG_MTU: { - size_t oc_offset = encode_start_controller_op( - ACTION_OPCODE_PUT_ICMP4_FRAG_MTU, true, - ep->ctrl_meter_id, 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, - ep->ctrl_meter_id, 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(); } + + encode_finish_ovn_field_note(offset, ofpacts); } static void diff --git a/tests/ovn.at b/tests/ovn.at index 918f97a9e..968997d1d 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1867,7 +1867,7 @@ icmp4 { }; # icmp4 with icmp4.frag_mtu icmp4 { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output; - encodes as controller(userdata=00.00.00.0a.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) + encodes as controller(userdata=00.00.00.0a.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) has prereqs ip4 # icmp4_error @@ -1882,11 +1882,11 @@ icmp4_error { }; # icmp4_error with icmp4.frag_mtu icmp4_error { eth.dst = ff:ff:ff:ff:ff:ff; icmp4.frag_mtu = 1500; output; }; output; - encodes as controller(userdata=00.00.00.0e.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.0e.00.00.00.0d.00.00.00.00.05.dc.00.00.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) + encodes as controller(userdata=00.00.00.0e.00.00.00.00.00.19.00.10.80.00.06.06.ff.ff.ff.ff.ff.ff.00.00.ff.ff.00.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.00.05.dc.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00),resubmit(,64) has prereqs ip4 icmp4.frag_mtu = 1500; - encodes as controller(userdata=00.00.00.0d.00.00.00.00.05.dc,pause) + encodes as note:6f.76.6e.00.00.00.00.00.05.dc # icmp6 icmp6 { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output; @@ -1910,11 +1910,11 @@ icmp6_error { }; # 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) + 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.18.00.00.23.20.00.08.6f.76.6e.00.00.00.00.01.00.00.05.dc.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) + encodes as note:6f.76.6e.00.00.00.00.01.00.00.05.dc # tcp_reset tcp_reset { eth.dst = ff:ff:ff:ff:ff:ff; output; }; output;
The fields that are not directly supported by OvS were encoded via additional controller action that changed the required value. This was most notably needed for ICMP need frag messages. Encode the field value loads as note action instead. This allows us to find the note and act accordingly in the first controller action without the need to send it to pinctrl again, parse and change the packet. This way we can also encode any future fields that might be needed as this method should be flexible enough. This change is completely transparent to the user and shouldn't cause any disruptions. Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Fix the wrong checksum for the ICMP packet. --- controller/physical.c | 14 ++-- controller/pinctrl.c | 183 +++++++++++++++++++++--------------------- include/ovn/actions.h | 16 ++++ lib/actions.c | 44 +++++++--- tests/ovn.at | 10 +-- 5 files changed, 154 insertions(+), 113 deletions(-)