From: Adam Emerson <aemerson@redhat.com>
Date: Tue, 4 Jun 2024 03:17:33 +0000 (-0400)
Subject: rgw/multisite: Fix use-after-move in retry logic in logbacking
X-Git-Tag: testing/wip-khiremat-testing-20250422.120708-squid-debug~57^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=c9e9744e9faf145289526c72d53d46ab75a04f01;p=ceph-ci.git

rgw/multisite: Fix use-after-move in retry logic in logbacking

Fitting a retry loop around a call that took the lock changed the
logic to potentially have the lock be used after moving.

Thanks to Suyash Dongre suyashd999@gmail.com for finding it.

This patch also fixes a lock being held across an I/O operation.

Fixes: https://tracker.ceph.com/issues/66340

Signed-off-by: Adam Emerson <aemerson@redhat.com>
(cherry picked from commit 80cf2ca3b44246ad8f2e333182b3c8b1835a71f7)

Fixed: https://tracker.ceph.com/issues/68053

Signed-off-by: Adam Emerson <aemerson@redhat.com>
---

diff --git a/src/rgw/driver/rados/rgw_log_backing.cc b/src/rgw/driver/rados/rgw_log_backing.cc
index 325d0510901..110a54015a3 100644
--- a/src/rgw/driver/rados/rgw_log_backing.cc
+++ b/src/rgw/driver/rados/rgw_log_backing.cc
@@ -444,13 +444,17 @@ bs::error_code logback_generations::write(const DoutPrefixProvider *dpp, entries
     encode(e, bl);
     op.write_full(bl);
     cls_version_inc(op);
+    auto oldv = version;
+    l.unlock();
     auto r = rgw_rados_operate(dpp, ioctx, oid, &op, y);
     if (r == 0) {
+      if (oldv != version) {
+	return { ECANCELED, bs::system_category() };
+      }
       entries_ = std::move(e);
       version.inc();
       return {};
     }
-    l.unlock();
     if (r < 0 && r != -ECANCELED) {
       ldpp_dout(dpp, -1) << __PRETTY_FUNCTION__ << ":" << __LINE__
 		 << ": failed reading oid=" << oid
@@ -609,17 +613,19 @@ bs::error_code logback_generations::remove_empty(const DoutPrefixProvider *dpp,
     if (ec) return ec;
     auto tries = 0;
     entries_t new_entries;
-    std::unique_lock l(m);
-    ceph_assert(!entries_.empty());
+    entries_t es;
+    auto now = ceph::real_clock::now();
     {
-      auto i = lowest_nomempty(entries_);
-      if (i == entries_.begin()) {
-	return {};
+      std::unique_lock l(m);
+      ceph_assert(!entries_.empty());
+      {
+	auto i = lowest_nomempty(entries_);
+	if (i == entries_.begin()) {
+	  return {};
+	}
       }
+      l.unlock();
     }
-    entries_t es;
-    auto now = ceph::real_clock::now();
-    l.unlock();
     do {
       std::copy_if(entries_.cbegin(), entries_.cend(),
 		   std::inserter(es, es.end()),
@@ -646,7 +652,7 @@ bs::error_code logback_generations::remove_empty(const DoutPrefixProvider *dpp,
 	  es2.erase(i);
 	}
       }
-      l.lock();
+      std::unique_lock l(m);
       es.clear();
       ec = write(dpp, std::move(es2), std::move(l), y);
       ++tries;