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 an oversized packet 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]> --- 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..a4f92a2af 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 70000]) +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
