osdc: Objecter::linger_by_cookie() for safe cast from uint64#65698
osdc: Objecter::linger_by_cookie() for safe cast from uint64#65698
Conversation
preserve a reference to LingerOp in case their invocation races with another linger_cancel() Signed-off-by: Casey Bodley <cbodley@redhat.com>
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<LingerOp> `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 <cbodley@redhat.com>
|
jenkins test container make check |
i was mistaken about this part, as |
| Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie); | ||
| boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie); | ||
| if (!linger_op) { | ||
| return -ENOTCONN; |
There was a problem hiding this comment.
Hi @cbodley,
This introduced a regression in RBD: librbd asserts that IoCtx::aio_watch() returns 0 because that has been the expected behavior for years meaning that we are experiencing sporadic crashes now. If the linger op is canceled, we expect to get ENOTCONN via the callback, not from the initiating function. Having the initiating function fail this way is prone to inconsistent behavior: in the scenario where the removal of the object on which the watch is established races with a call to IoCtx::aio_watch(), one could get the error from either the initiating function or the callback depending on the outcome of the race. Writing code such that it's prepared to handle both cases is suboptimal and also isn't aligned with neorados which always delivers the error via the callback.
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 bylinger_cancel()- for example when called byCB_DoWatchError
I'm not convinced there was a potential for a use-after-free in IoCtx::unwatch() or IoCtx::aio_watch() for a cookie that had gotten invalidated in response to a watch error -- assuming the caller passed a cookie previously obtained from IoCtx::watch(), of course. My understanding has been that Objecter::linger_register() returns a linger op with two references, one of which is intended for the caller of IoCtx::watch() (i.e. the user of the public API). That reference remains alive until the user performs the unwatch. A linger op that is canceled just gets marked as such (LingerOp::canceled bool) and loses the internal reference -- nothing should happen to the external reference. If that is correct, an extra validity check isn't strictly required but just a nice to have to catch totally bogus cookies. Am I missing something?
There was a problem hiding this comment.
I'm not convinced there was a potential for a use-after-free
sorry for the confusion, i was mistaken about that comment and tried to clarify in #65698 (comment)
If that is correct, an extra validity check isn't strictly required but just a nice to have to catch totally bogus cookies.
this includes cookies that the client already unwatched, so ioctx.unwatch(cookie); ioctx.unwatch(cookie); can lead to undefined behavior
in rgw, our librados::WatchCtx2::handle_error() override schedules calls to reinitialize the watch with ioctx.unwatch() -> ioctx.watch(). it doesn't look like anything prevents that unwatch from racing with our shutdown code that also calls unwatch, so there are still bugs to fix here
regardless, i'd argue that correct use of watch/notify is subtle enough that it's worth detecting these cases in librados to avoid dereferencing invalid memory
apologies for the regression. if you find my argument convincing, i can address the error handling issue in aio_unwatch()
There was a problem hiding this comment.
Sure, I don't see anything wrong with protecting against duplicate unwatch calls or similar mishaps. I just wanted to ensure that my understanding of the existing code was correct.
From the RBD POV, what we need to move past this is IoCtx::aio_watch() delivering this early ENOTCONN via the callback, just like its neorados counterpart. Is that what you have in mind?
There was a problem hiding this comment.
From the RBD POV, what we need to move past this is
IoCtx::aio_watch()delivering this earlyENOTCONNvia the callback, just like its neorados counterpart. Is that what you have in mind?
👍 on it
a
linger_ops_setwas added forObjecter::handle_watch_notify()as a safety check before castinguint64_t cookietoLingerOp*and deferencing itneorados 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 foris_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 bylinger_cancel()- for example when called byCB_DoWatchErrorreplace
is_valid_watch()with alinger_by_cookie()function thatlinger_ops_set,reinterpret_casts the cookie toLingerOp*, andintrusive_ptr<LingerOp>librados::IoCtxImpl::watch_check(),unwatch()andaio_unwatch()now calllinger_by_cookie(), so have to handle the null case by returning-ENOTCONN(this matches neorados' existing behavior)Fixes: https://tracker.ceph.com/issues/72771
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job DefinitionYou must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.