From: Alejandro Pacheco <[email protected]>
The OpenFlow header length is 16 bits, but do_send() stores the buffer size there without a check. A packet-in always carries the whole packet, so a packet longer than about 65400 bytes that reaches a controller action, for example a TSO packet in the userspace datapath, is sent with a length that does not match the bytes on the wire. The controller then reads the rest of the packet data as further OpenFlow messages.
Make vconn_send() return EMSGSIZE for such a message, and have rconn_send() drop it with a warning before it is queued, since an error from vconn_send() would otherwise disconnect the controller. Packet-in data is not truncated to fit instead: a continuation needs the full packet to resume. Also free the encoded request when sending a bundle message fails, check the inner message length there, and stop waiting for multipart replies to requests that were not sent. The test sends a 65535-byte packet, the largest a DPDK build can hold, to the controller, then a short one, and checks that only the short one arrives on the still-open connection. Assisted-by: claude-opus-5-5, Claude Code Signed-off-by: Alejandro Pacheco <[email protected]> --- v2: Test with a 65535-byte packet; a DPDK build aborts on a larger one. lib/rconn.c | 21 ++++++++++++++++----- lib/vconn.c | 18 +++++++++++++++++- tests/ofproto-dpif.at | 27 +++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/lib/rconn.c b/lib/rconn.c index 0a5584239..267611925 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -745,7 +745,16 @@ rconn_send__(struct rconn *rc, struct ofpbuf *b, struct rconn_packet_counter *counter) OVS_REQUIRES(rc->mutex) { - if (rconn_is_connected(rc)) { + if (b->size > UINT16_MAX) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); + + /* vconn_send() would refuse it and 'rc' would disconnect. */ + VLOG_WARN_RL(&rl, "%s: dropping %"PRIu32"-byte message longer than " + "the OpenFlow maximum", rc->name, b->size); + COVERAGE_INC(rconn_discarded); + ofpbuf_delete(b); + return EMSGSIZE; + } else if (rconn_is_connected(rc)) { COVERAGE_INC(rconn_queued); copy_to_monitor(rc, b); @@ -772,8 +781,9 @@ rconn_send__(struct rconn *rc, struct ofpbuf *b, } } -/* Sends 'b' on 'rc'. Returns 0 if successful, or ENOTCONN if 'rc' is not - * currently connected. Takes ownership of 'b'. +/* Sends 'b' on 'rc'. Returns 0 if successful, EMSGSIZE if 'b' is longer than + * an OpenFlow message can be, or ENOTCONN if 'rc' is not currently connected. + * Takes ownership of 'b'. * * If 'counter' is non-null, then 'counter' will be incremented while the * packet is in flight, then decremented when it has been sent (or discarded @@ -801,8 +811,9 @@ rconn_send(struct rconn *rc, struct ofpbuf *b, /* Sends 'b' on 'rc'. Increments 'counter' while the packet is in flight; it * will be decremented when it has been sent (or discarded due to * disconnection). Returns 0 if successful, EAGAIN if 'counter->n' is already - * at least as large as 'queue_limit', or ENOTCONN if 'rc' is not currently - * connected. Regardless of return value, 'b' is destroyed. + * at least as large as 'queue_limit', EMSGSIZE if 'b' is longer than an + * OpenFlow message can be, or ENOTCONN if 'rc' is not currently connected. + * Regardless of return value, 'b' is destroyed. * * Because 'b' may be sent (or discarded) before this function returns, the * caller may not be able to observe any change in 'counter'. diff --git a/lib/vconn.c b/lib/vconn.c index 040955d67..f48a14375 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -659,7 +659,8 @@ do_recv(struct vconn *vconn, struct ofpbuf **msgp) * only that it has been queued for transmission. * * Returns a positive errno value on failure, in which case the caller - * retains ownership of 'msg'. + * retains ownership of 'msg'. Returns EMSGSIZE if 'msg' is longer than + * UINT16_MAX bytes, the most an OpenFlow header can express. * * vconn_send will not block. If 'msg' cannot be immediately accepted for * transmission, it returns EAGAIN immediately. */ @@ -680,6 +681,14 @@ do_send(struct vconn *vconn, struct ofpbuf *msg) ovs_assert(msg->size >= sizeof(struct ofp_header)); + if (msg->size > UINT16_MAX) { + /* The OpenFlow header can't express this length. */ + VLOG_WARN_RL(&bad_ofmsg_rl, "%s: not sending %"PRIu32"-byte message " + "longer than the OpenFlow maximum", vconn->name, + msg->size); + return EMSGSIZE; + } + ofpmsg_update_length(msg); if (!VLOG_IS_DBG_ENABLED()) { retval = (vconn->vclass->send)(vconn, msg); @@ -968,6 +977,8 @@ vconn_transact_multipart(struct vconn *vconn, int error = vconn_send_block(vconn, b); if (error) { ofpbuf_delete(b); + ofpbuf_list_delete(requests); + return error; } } @@ -1243,6 +1254,9 @@ vconn_bundle_add_msg(struct vconn *vconn, struct ofputil_bundle_ctrl_msg *bc, struct ofpbuf *request; int error; + if (msg->size > UINT16_MAX) { + return EMSGSIZE; + } ofpmsg_update_length(msg); bam.bundle_id = bc->bundle_id; @@ -1256,6 +1270,8 @@ vconn_bundle_add_msg(struct vconn *vconn, struct ofputil_bundle_ctrl_msg *bc, /* Check for an error return, so that the socket buffer does not become * full of errors. */ vconn_recv_error(vconn, errors); + } else { + ofpbuf_delete(request); } return error; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index cd8991814..fdc97bf75 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -2778,6 +2778,33 @@ OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - controller packet-in longer than OpenFlow maximum]) +OVS_VSWITCHD_START +add_of_ports br0 1 + +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=controller]) + +AT_CAPTURE_FILE([ofctl_monitor.log]) +AT_CHECK([ovs-ofctl monitor br0 65534 -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) + +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)' --len 65535]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)']) + +OVS_WAIT_UNTIL_EQUAL([grep -c "total_len=14 " ofctl_monitor.log], [1]) +OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) + +AT_CHECK([cat ofctl_monitor.log], [0], [dnl +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 +]) + +AT_CHECK([grep -c "dropping [[0-9]]*-byte message longer than the OpenFlow maximum" ovs-vswitchd.log], [0], [1 +]) + +OVS_VSWITCHD_STOP(["/dropping .*-byte message longer than the OpenFlow maximum/d"]) +AT_CLEANUP + + AT_SETUP([ofproto-dpif - controller action without megaflows]) OVS_VSWITCHD_START add_of_ports br0 1
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
