From 50d62b1b2b6eac043a98500223a1e56174b50229 Mon Sep 17 00:00:00 2001 From: Casey Bodley Date: Fri, 26 Sep 2025 17:25:53 -0400 Subject: [PATCH] osdc: Objecter::linger_by_cookie() for safe cast from uint64 a `linger_ops_set` was added for `Objecter::handle_watch_notify()` as a safety check before casting `uint64_t cookie` to `LingerOp*` and deferencing it neorados also made use of this set through `Objecter::is_valid_watch()` checks. however, this approach was still susceptible to use-after-free, because the callers didn't preserve a LingerOp reference between this check and its use - and the Objecter lock is dropped in between. in addition, `neorados::RADOS::unwatch_()` was missing its check for `is_valid_watch()` librados did not make use of this `is_valid_watch()` at all, so was casting cookies directly to LingerOp* and dereferencing. this results in use-after-free for any cookies invalidated by `linger_cancel()` - for example when called by `CB_DoWatchError` replace `is_valid_watch()` with a `linger_by_cookie()` function that * performs the validity check with `linger_ops_set`, * safely reinterpret_casts the cookie to LingerOp*, and * returns a reference to the caller via intrusive_ptr `librados::IoCtxImpl::watch_check()`, `unwatch()` and `aio_unwatch()` now call `linger_by_cookie()`, so have to handle the null case by returning `-ENOTCONN` (this matches neorados' existing behavior) Fixes: https://tracker.ceph.com/issues/72771 Signed-off-by: Casey Bodley --- src/librados/IoCtxImpl.cc | 20 +++++++++++++++----- src/neorados/RADOS.cc | 19 ++++++++++++------- src/osdc/Objecter.cc | 36 +++++++++++++++++++++++++++--------- src/osdc/Objecter.h | 13 ++++++++----- 4 files changed, 62 insertions(+), 26 deletions(-) diff --git a/src/librados/IoCtxImpl.cc b/src/librados/IoCtxImpl.cc index 02ef3022274..ae93b72f480 100644 --- a/src/librados/IoCtxImpl.cc +++ b/src/librados/IoCtxImpl.cc @@ -1745,8 +1745,11 @@ int librados::IoCtxImpl::notify_ack( int librados::IoCtxImpl::watch_check(uint64_t cookie) { - auto linger_op = reinterpret_cast(cookie); - auto r = objecter->linger_check(linger_op); + boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie); + if (!linger_op) { + return -ENOTCONN; + } + auto r = objecter->linger_check(linger_op.get()); if (r) return 1 + std::chrono::duration_cast< std::chrono::milliseconds>(*r).count(); @@ -1756,7 +1759,11 @@ int librados::IoCtxImpl::watch_check(uint64_t cookie) int librados::IoCtxImpl::unwatch(uint64_t cookie) { - Objecter::LingerOp *linger_op = reinterpret_cast(cookie); + boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie); + if (!linger_op) { + return -ENOTCONN; + } + C_SaferCond onfinish; version_t ver = 0; @@ -1766,7 +1773,7 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie) objecter->mutate(linger_op->target.base_oid, oloc, wr, snapc, ceph::real_clock::now(), extra_op_flags, &onfinish, &ver); - objecter->linger_cancel(linger_op); + objecter->linger_cancel(linger_op.get()); int r = onfinish.wait(); set_sync_op_version(ver); @@ -1776,7 +1783,10 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie) int librados::IoCtxImpl::aio_unwatch(uint64_t cookie, AioCompletionImpl *c) { c->io = this; - Objecter::LingerOp *linger_op = reinterpret_cast(cookie); + boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie); + if (!linger_op) { + return -ENOTCONN; + } Context *oncomplete = new C_aio_linger_Complete(c, linger_op, true); ::ObjectOperation wr; diff --git a/src/neorados/RADOS.cc b/src/neorados/RADOS.cc index 4dcba611907..53b1b42a24c 100644 --- a/src/neorados/RADOS.cc +++ b/src/neorados/RADOS.cc @@ -1570,8 +1570,8 @@ void RADOS::watch_(Object o, IOContext _ioc, WatchComp c, } void RADOS::next_notification_(uint64_t cookie, NextNotificationComp c) { - Objecter::LingerOp* linger_op = reinterpret_cast(cookie); - if (!impl->objecter->is_valid_watch(linger_op)) { + boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie); + if (!linger_op) { dispatch(asio::append(std::move(c), bs::error_code(ENOTCONN, bs::generic_category()), Notification{})); @@ -1611,9 +1611,9 @@ void RADOS::notify_ack_(Object o, IOContext _ioc, tl::expected RADOS::check_watch(uint64_t cookie) { - auto linger_op = reinterpret_cast(cookie); - if (impl->objecter->is_valid_watch(linger_op)) { - return impl->objecter->linger_check(linger_op); + boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie); + if (linger_op) { + return impl->objecter->linger_check(linger_op.get()); } else { return tl::unexpected(bs::error_code(ENOTCONN, bs::generic_category())); } @@ -1624,7 +1624,12 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc, { auto ioc = reinterpret_cast(&_ioc.impl); - Objecter::LingerOp *linger_op = reinterpret_cast(cookie); + boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie); + if (!linger_op) { + dispatch(asio::append(std::move(c), + bs::error_code(ENOTCONN, bs::generic_category()))); + return; + } ObjectOperation op; op.watch(cookie, CEPH_OSD_WATCH_OP_UNWATCH); @@ -1637,7 +1642,7 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc, [objecter = impl->objecter, linger_op, c = std::move(c)] (bs::error_code ec) mutable { - objecter->linger_cancel(linger_op); + objecter->linger_cancel(linger_op.get()); asio::dispatch(asio::append(std::move(c), ec)); })); } diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc index 6ccaf9a76aa..f062d69b830 100644 --- a/src/osdc/Objecter.cc +++ b/src/osdc/Objecter.cc @@ -657,9 +657,10 @@ class CB_DoWatchError { boost::intrusive_ptr info; bs::error_code ec; public: - CB_DoWatchError(Objecter *o, Objecter::LingerOp *i, + CB_DoWatchError(Objecter *o, + boost::intrusive_ptr i, bs::error_code ec) - : objecter(o), info(i), ec(ec) { + : objecter(o), info(std::move(i)), ec(ec) { info->_queued_async(); } void operator()() { @@ -812,7 +813,22 @@ void Objecter::_linger_cancel(LingerOp *info) } } +auto Objecter::linger_by_cookie(uint64_t cookie) + -> boost::intrusive_ptr +{ + auto lock = std::shared_lock{rwlock}; + return _linger_by_cookie(cookie); +} +auto Objecter::_linger_by_cookie(uint64_t cookie) + -> boost::intrusive_ptr +{ + auto info = reinterpret_cast(cookie); + if (!linger_ops_set.contains(info)) { + return nullptr; // invalid or canceled op + } + return info; +} Objecter::LingerOp *Objecter::linger_register(const object_t& oid, const object_locator_t& oloc, @@ -919,8 +935,10 @@ struct CB_DoWatchNotify { Objecter *objecter; boost::intrusive_ptr info; boost::intrusive_ptr msg; - CB_DoWatchNotify(Objecter *o, Objecter::LingerOp *i, MWatchNotify *m) - : objecter(o), info(i), msg(m) { + CB_DoWatchNotify(Objecter *o, + boost::intrusive_ptr i, + MWatchNotify *m) + : objecter(o), info(std::move(i)), msg(m) { info->_queued_async(); } void operator()() { @@ -935,8 +953,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m) return; } - LingerOp *info = reinterpret_cast(m->cookie); - if (linger_ops_set.count(info) == 0) { + boost::intrusive_ptr info = _linger_by_cookie(m->cookie); + if (!info) { ldout(cct, 7) << __func__ << " cookie " << m->cookie << " dne" << dendl; return; } @@ -945,8 +963,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m) if (!info->last_error) { info->last_error = bs::error_code(ENOTCONN, osd_category()); if (info->handle) { - asio::defer(finish_strand, CB_DoWatchError(this, info, - info->last_error)); + boost::system::error_code ec = info->last_error; + asio::defer(finish_strand, CB_DoWatchError(this, std::move(info), ec)); } } } else if (!info->is_watch) { @@ -967,7 +985,7 @@ void Objecter::handle_watch_notify(MWatchNotify *m) info->on_notify_finish = nullptr; } } else { - asio::defer(finish_strand, CB_DoWatchNotify(this, info, m)); + asio::defer(finish_strand, CB_DoWatchNotify(this, std::move(info), m)); } } diff --git a/src/osdc/Objecter.h b/src/osdc/Objecter.h index 51f1687fc49..b3ab73281fe 100644 --- a/src/osdc/Objecter.h +++ b/src/osdc/Objecter.h @@ -2592,11 +2592,6 @@ public: friend class CB_DoWatchError; public: - bool is_valid_watch(LingerOp* op) { - std::shared_lock l(rwlock); - return linger_ops_set.contains(op); - } - template auto linger_callback_flush(CT&& ct) { auto consigned = boost::asio::consign( @@ -3253,6 +3248,14 @@ public: void linger_cancel(LingerOp *info); // releases a reference void _linger_cancel(LingerOp *info); + // return the LingerOp associated with the given cookie. + // may return nullptr if the cookie is no longer valid + boost::intrusive_ptr linger_by_cookie(uint64_t cookie); + private: + // internal version that expects the caller to hold rwlock + boost::intrusive_ptr _linger_by_cookie(uint64_t cookie); + public: + void _do_watch_notify(boost::intrusive_ptr info, boost::intrusive_ptr m); -- 2.39.5