From b11c0eaedc3c9108bab9c85b72d4fcc074c8e1d5 Mon Sep 17 00:00:00 2001 From: Dax Kelson Date: Sat, 20 Jun 2026 13:18:19 -0600 Subject: [PATCH] msg/async/rdma: don't abort on an invalid per-connection eventfd The async RDMA messenger reports each connection's per-connection eventfd (notify_fd) as its socket fd(). notify_fd can legitimately be -1 -- under fd-table pressure eventfd() returns -1, and the base RDMAConnectedSocketImpl constructor skips queue pair *and* eventfd creation entirely when ms_async_rdma_cm=true (the cm path is only implemented by the iwarp socket classes). Either way an fd() == -1 socket reaches the success path of Processor::accept and trips ceph_assert(socket.fd() >= 0) in AsyncConnection::accept, or is fed to EventCenter::create_file_event() and indexes file_events[-1] out of bounds. Both abort the OSD under connection load. Make the messenger surface the failure instead of handing back a broken socket: - Initialize RDMAConnectedSocketImpl::qp to nullptr. It was uninitialized and read via get_qp() when ms_async_rdma_cm=true skips its assignment. - Create the notify eventfd before the queue pair in the base constructor, and adopt the cm id/channel before the eventfd in the iwarp constructor, so a failure leaks nothing and the two failure causes stay distinguishable. Check the rdma_create_event_channel()/rdma_create_id() return values on the touched paths, and destroy the per-request cm id on the accept error paths (acking the event does not free it). - At each accept/connect construction site, reject a socket with fd() < 0 (-EMFILE) or a null queue pair (-EIO) instead of returning it. Free the half-built socket on its owning worker thread via center.submit_to() so ~RDMAConnectedSocketImpl's center.in_thread() assertion holds and no RDMA/fd state leaks (this also fixes the pre-existing leak on the rdma_accept() failure path). - Require ms_async_rdma_cm and ms_async_rdma_type=iwarp to be enabled together in RDMAWorker::listen()/connect() (-EOPNOTSUPP with a clear message). The two are coupled -- only the iwarp classes implement rdma_cm and they always use it -- so any mismatch leaves a socket half-built and would otherwise crash-loop the daemon. - Guard EventCenter::create_file_event() against a negative fd as defense in depth. Fixes: https://tracker.ceph.com/issues/77547 Signed-off-by: Dax Kelson --- src/msg/async/Event.cc | 1 + src/msg/async/rdma/RDMAConnectedSocketImpl.cc | 14 +++++- .../rdma/RDMAIWARPConnectedSocketImpl.cc | 38 +++++++++++++--- .../async/rdma/RDMAIWARPServerSocketImpl.cc | 31 ++++++++++++- src/msg/async/rdma/RDMAServerSocketImpl.cc | 20 ++++++--- src/msg/async/rdma/RDMAStack.cc | 43 ++++++++++++++++++- src/msg/async/rdma/RDMAStack.h | 2 +- 7 files changed, 131 insertions(+), 18 deletions(-) diff --git a/src/msg/async/Event.cc b/src/msg/async/Event.cc index 7399494a93f..567bb9e0ea0 100644 --- a/src/msg/async/Event.cc +++ b/src/msg/async/Event.cc @@ -253,6 +253,7 @@ void EventCenter::set_owner() int EventCenter::create_file_event(int fd, int mask, EventCallbackRef ctxt) { ceph_assert(in_thread()); + ceph_assert(fd >= 0); int r = 0; if (fd >= nevent) { int new_size = nevent << 2; diff --git a/src/msg/async/rdma/RDMAConnectedSocketImpl.cc b/src/msg/async/rdma/RDMAConnectedSocketImpl.cc index d5ebf8ae2cc..6bbd672968e 100644 --- a/src/msg/async/rdma/RDMAConnectedSocketImpl.cc +++ b/src/msg/async/rdma/RDMAConnectedSocketImpl.cc @@ -58,13 +58,22 @@ RDMAConnectedSocketImpl::RDMAConnectedSocketImpl(CephContext *cct, std::shared_p active(false), pending(false) { if (!cct->_conf->ms_async_rdma_cm) { + // Create the per-connection notify eventfd before the queue pair. Under fd + // pressure eventfd() can fail (-1); doing it first means a failure leaves no + // queue pair to leak. The two failure states are kept distinct on purpose so + // the accept/connect paths can return an honest errno: eventfd failure -> + // fd() < 0 && qp == nullptr; queue pair failure -> fd() >= 0 && qp == nullptr. + notify_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); + if (notify_fd < 0) { + lderr(cct) << __func__ << " failed to create eventfd: " << cpp_strerror(errno) << dendl; + return; + } qp = ib->create_queue_pair(cct, dispatcher->get_tx_cq(), dispatcher->get_rx_cq(), IBV_QPT_RC, NULL); if (!qp) { lderr(cct) << __func__ << " queue pair create failed" << dendl; return; } local_qpn = qp->get_local_qp_number(); - notify_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); dispatcher->register_qp(qp, this); dispatcher->perf_logger->inc(l_msgr_rdma_created_queue_pair); dispatcher->perf_logger->inc(l_msgr_rdma_active_queue_pair); @@ -76,7 +85,8 @@ RDMAConnectedSocketImpl::~RDMAConnectedSocketImpl() ldout(cct, 20) << __func__ << " destruct." << dendl; cleanup(); worker->remove_pending_conn(this); - dispatcher->schedule_qp_destroy(local_qpn); + if (qp) // null on construction-failure paths; local_qpn 0 is unregistered + dispatcher->schedule_qp_destroy(local_qpn); for (unsigned i=0; i < wc.size(); ++i) { dispatcher->post_chunk_to_pool(reinterpret_cast(wc[i].wr_id)); diff --git a/src/msg/async/rdma/RDMAIWARPConnectedSocketImpl.cc b/src/msg/async/rdma/RDMAIWARPConnectedSocketImpl.cc index 606dbd2817c..1844f695c88 100644 --- a/src/msg/async/rdma/RDMAIWARPConnectedSocketImpl.cc +++ b/src/msg/async/rdma/RDMAIWARPConnectedSocketImpl.cc @@ -13,13 +13,43 @@ RDMAIWARPConnectedSocketImpl::RDMAIWARPConnectedSocketImpl(CephContext *cct, std : RDMAConnectedSocketImpl(cct, ib, rdma_dispatcher, w), cm_con_handler(new C_handle_cm_connection(this)) { status = IDLE; - notify_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); + // Take ownership of the CM id/channel *before* creating the notify eventfd, so + // that if eventfd() fails (e.g. under fd exhaustion) the destructor still frees + // them (it only does so for status >= RDMA_ID_CREATED). Every failure path + // calls close_notify() so a later delete cannot block on close_condition. if (info) { is_server = true; cm_id = info->cm_id; cm_channel = info->cm_channel; status = RDMA_ID_CREATED; peer_qpn = info->qp_num; + } else { + is_server = false; + cm_channel = rdma_create_event_channel(); + if (!cm_channel) { + lderr(cct) << __func__ << " failed to create cm event channel: " << cpp_strerror(errno) << dendl; + close_notify(); + return; + } + if (rdma_create_id(cm_channel, &cm_id, NULL, RDMA_PS_TCP)) { + lderr(cct) << __func__ << " failed to create cm id: " << cpp_strerror(errno) << dendl; + rdma_destroy_event_channel(cm_channel); + cm_channel = nullptr; + close_notify(); + return; + } + status = RDMA_ID_CREATED; + ldout(cct, 20) << __func__ << " successfully created cm id: " << cm_id << dendl; + } + + notify_fd = eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK); + if (notify_fd < 0) { + lderr(cct) << __func__ << " failed to create eventfd: " << cpp_strerror(errno) << dendl; + close_notify(); + return; + } + + if (info) { if (alloc_resource()) { close_notify(); return; @@ -31,12 +61,6 @@ RDMAIWARPConnectedSocketImpl::RDMAIWARPConnectedSocketImpl(CephContext *cct, std status = RESOURCE_ALLOCATED; qp->get_local_cm_meta().peer_qpn = peer_qpn; qp->get_peer_cm_meta().local_qpn = peer_qpn; - } else { - is_server = false; - cm_channel = rdma_create_event_channel(); - rdma_create_id(cm_channel, &cm_id, NULL, RDMA_PS_TCP); - status = RDMA_ID_CREATED; - ldout(cct, 20) << __func__ << " successfully created cm id: " << cm_id << dendl; } } diff --git a/src/msg/async/rdma/RDMAIWARPServerSocketImpl.cc b/src/msg/async/rdma/RDMAIWARPServerSocketImpl.cc index 0500b4420f9..f5c3479b1de 100644 --- a/src/msg/async/rdma/RDMAIWARPServerSocketImpl.cc +++ b/src/msg/async/rdma/RDMAIWARPServerSocketImpl.cc @@ -71,13 +71,25 @@ int RDMAIWARPServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions rdma_get_cm_event(cm_channel, &cm_event); ldout(cct, 20) << __func__ << " event name: " << rdma_event_str(cm_event->event) << dendl; + // rdma_get_cm_event() handed us a new cm id for the incoming connection; on any + // error path before it is adopted into a socket (whose destructor would free it) + // we must rdma_destroy_id() it ourselves -- acking the event does not. The event + // must be acked *before* destroying the id: rdma_destroy_id() blocks until the + // connect-request event for that id has been acknowledged. struct rdma_cm_id *event_cm_id = cm_event->id; struct rdma_event_channel *event_channel = rdma_create_event_channel(); + if (!event_channel) { + lderr(cct) << __func__ << " failed to create event channel: " << cpp_strerror(errno) << dendl; + rdma_ack_cm_event(cm_event); + rdma_destroy_id(event_cm_id); + return -EMFILE; + } if (net.set_nonblock(event_channel->fd) < 0) { lderr(cct) << __func__ << " failed to switch event channel to non-block, close event channel " << dendl; rdma_destroy_event_channel(event_channel); rdma_ack_cm_event(cm_event); + rdma_destroy_id(event_cm_id); return -errno; } @@ -86,15 +98,32 @@ int RDMAIWARPServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions struct rdma_conn_param *remote_conn_param = &cm_event->param.conn; struct rdma_conn_param local_conn_param; + RDMAWorker *rw = dynamic_cast(w); RDMACMInfo info(event_cm_id, event_channel, remote_conn_param->qp_num); RDMAIWARPConnectedSocketImpl* server = - new RDMAIWARPConnectedSocketImpl(cct, ib, dispatcher, dynamic_cast(w), &info); + new RDMAIWARPConnectedSocketImpl(cct, ib, dispatcher, rw, &info); + + // Construction can fail under fd pressure (eventfd) or on queue pair creation; + // such a socket must not reach the messenger (its fd() is -1). Free it on its + // owning worker thread (close() marks it closed so ~IWARP won't block and frees + // the adopted cm id/channel) and return an honest errno (fd-first). + if (server->fd() < 0 || !server->get_qp()) { + int err = server->fd() < 0 ? -EMFILE : -EIO; + lderr(cct) << __func__ << " failed to create connected socket: " + << cpp_strerror(err) << dendl; + rdma_ack_cm_event(cm_event); + rw->center.submit_to(rw->center.get_id(), [server]() { server->close(); delete server; }, false); + return err; + } // FIPS zeroization audit 20191115: this memset is not security related. memset(&local_conn_param, 0, sizeof(local_conn_param)); local_conn_param.qp_num = server->get_local_qpn(); if (rdma_accept(event_cm_id, &local_conn_param)) { + lderr(cct) << __func__ << " rdma_accept failed: " << cpp_strerror(errno) << dendl; + rdma_ack_cm_event(cm_event); + rw->center.submit_to(rw->center.get_id(), [server]() { server->close(); delete server; }, false); return -EAGAIN; } server->activate(); diff --git a/src/msg/async/rdma/RDMAServerSocketImpl.cc b/src/msg/async/rdma/RDMAServerSocketImpl.cc index 69ad2d2dc94..4435c95915a 100644 --- a/src/msg/async/rdma/RDMAServerSocketImpl.cc +++ b/src/msg/async/rdma/RDMAServerSocketImpl.cc @@ -111,15 +111,23 @@ int RDMAServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions &opt out->set_sockaddr((sockaddr*)&ss); net.set_priority(sd, opt.priority, out->get_family()); + RDMAWorker *rw = dynamic_cast(w); RDMAConnectedSocketImpl* server; //Worker* w = dispatcher->get_stack()->get_worker(); - server = new RDMAConnectedSocketImpl(cct, ib, dispatcher, dynamic_cast(w)); - if (!server->get_qp()) { - lderr(cct) << __func__ << " server->qp is null" << dendl; - // cann't use delete server here, destructor will fail. - server->cleanup(); + server = new RDMAConnectedSocketImpl(cct, ib, dispatcher, rw); + // Construction can fail under fd pressure (eventfd) or on queue pair creation. + // Such a socket has fd() < 0 and/or a null qp; handing it back on the success + // path would trip ceph_assert(socket.fd() >= 0) in AsyncConnection::accept. + // Return an honest errno (fd-first, so eventfd exhaustion maps to -EMFILE rather + // than -EIO) and free the half-built socket on its owning worker thread, where + // ~RDMAConnectedSocketImpl's center.in_thread() assertion holds. + if (server->fd() < 0 || !server->get_qp()) { + int err = server->fd() < 0 ? -EMFILE : -EIO; + lderr(cct) << __func__ << " failed to create connected socket: " + << cpp_strerror(err) << dendl; + rw->center.submit_to(rw->center.get_id(), [server]() { delete server; }, false); ::close(sd); - return -1; + return err; } server->set_accept_fd(sd); ldout(cct, 20) << __func__ << " accepted a new QP, tcp_fd: " << sd << dendl; diff --git a/src/msg/async/rdma/RDMAStack.cc b/src/msg/async/rdma/RDMAStack.cc index 34dda383a3f..e8a86631628 100644 --- a/src/msg/async/rdma/RDMAStack.cc +++ b/src/msg/async/rdma/RDMAStack.cc @@ -692,6 +692,17 @@ void RDMAWorker::initialize() int RDMAWorker::listen(entity_addr_t &sa, unsigned addr_slot, const SocketOptions &opt,ServerSocket *sock) { + // The rdma_cm path and the iwarp socket classes are coupled: only the iwarp + // classes implement rdma_cm, and they always use it. Any other pairing leaves a + // socket half-built (the base ctor skips queue pair / eventfd creation when + // ms_async_rdma_cm=true, and a non-cm iwarp socket has no usable cm channel), so + // require the two to be enabled together and fail fast with a clear message. + if (cct->_conf->ms_async_rdma_cm != (cct->_conf->ms_async_rdma_type == "iwarp")) { + lderr(cct) << __func__ << " ms_async_rdma_cm and ms_async_rdma_type=iwarp must be " + "enabled together" << dendl; + return -EOPNOTSUPP; + } + ib->init(); dispatcher->polling_start(); @@ -713,15 +724,45 @@ int RDMAWorker::listen(entity_addr_t &sa, unsigned addr_slot, int RDMAWorker::connect(const entity_addr_t &addr, const SocketOptions &opts, ConnectedSocket *socket) { + const bool iwarp = cct->_conf->ms_async_rdma_type == "iwarp"; + // The rdma_cm (connection manager) path and the iwarp socket classes are coupled: + // only the iwarp classes implement rdma_cm, and they always use it. Any other + // pairing leaves a socket half-built -- with ms_async_rdma_cm=true and a non-iwarp + // type the base ctor skips queue pair / eventfd creation (fd() == -1 on every + // connection); with an iwarp type and ms_async_rdma_cm=false the cm channel is + // never set up. Require the two together and fail fast. + if (cct->_conf->ms_async_rdma_cm != iwarp) { + lderr(cct) << __func__ << " ms_async_rdma_cm and ms_async_rdma_type=iwarp must be " + "enabled together" << dendl; + return -EOPNOTSUPP; + } + ib->init(); dispatcher->polling_start(); RDMAConnectedSocketImpl* p; - if (cct->_conf->ms_async_rdma_type == "iwarp") { + if (iwarp) { p = new RDMAIWARPConnectedSocketImpl(cct, ib, dispatcher, this); } else { p = new RDMAConnectedSocketImpl(cct, ib, dispatcher, this); } + + // Reject a socket whose construction failed before wiring it up, otherwise it + // reaches the messenger with fd() == -1. fd-first so eventfd exhaustion maps to + // -EMFILE; the non-iwarp qp check then catches a queue pair failure that left a + // valid eventfd (an iwarp client legitimately has a null qp until its route is + // resolved, so it is excluded here). + if (p->fd() < 0) { + lderr(cct) << __func__ << " failed to create eventfd (fd limit reached?)" << dendl; + delete p; + return -EMFILE; + } + if (!iwarp && !p->get_qp()) { + lderr(cct) << __func__ << " failed to create queue pair" << dendl; + delete p; + return -EIO; + } + int r = p->try_connect(addr, opts); if (r < 0) { diff --git a/src/msg/async/rdma/RDMAStack.h b/src/msg/async/rdma/RDMAStack.h index dc01763ce34..7e225c8a915 100644 --- a/src/msg/async/rdma/RDMAStack.h +++ b/src/msg/async/rdma/RDMAStack.h @@ -176,7 +176,7 @@ class RDMAConnectedSocketImpl : public ConnectedSocketImpl { protected: CephContext *cct; - Infiniband::QueuePair *qp; + Infiniband::QueuePair *qp = nullptr; uint32_t peer_qpn = 0; uint32_t local_qpn = 0; int connected; -- 2.47.3