librbd/cache/pwl/ssd: avoid corrupting m_first_free_entry

In append_op_log_entries(), new_first_free_entry is read after
append_ops() returns.  This can result in accessing freed memory
because all I/Os may complete and append_ctx callback may run
by the time new_first_free_entry is read.  Garbage value gets
written to m_first_free_entry and depending on the circumstances
it may allow AbstractWriteLog code to accept more dirty user data
than we have space for.  Luckily we usually crash before then.

Fixes: https://tracker.ceph.com/issues/50832
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit d83a0f6db8ff26eeb2c817b1bd192fb357f715df)

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index df25cd1c36a9e1c7b9d8aa10e76136f6f5886eec..1a31004983f28ccb3c188c1a23489916ccfb914f 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -455,14 +455,7 @@ void WriteLog<I>::append_op_log_entries(GenericLogOperations &ops) {
       }
   });
   // Append logs and update first_free_update
-  uint64_t bytes_allocated_updated;
-  append_ops(ops, append_ctx, new_first_free_entry, bytes_allocated_updated);
-
-  {
-    std::lock_guard locker1(m_lock);
-    m_first_free_entry = *new_first_free_entry;
-    m_bytes_allocated -= bytes_allocated_updated;
-  }
+  append_ops(ops, append_ctx, new_first_free_entry);
 
   if (ops.size()) {
     this->dispatch_deferred_writes();
@@ -738,12 +731,11 @@ bool WriteLog<I>::retire_entries(const unsigned long int frees_per_tx) {
 
 template <typename I>
 void WriteLog<I>::append_ops(GenericLogOperations &ops, Context *ctx,
-                             uint64_t* new_first_free_entry,
-                             uint64_t &bytes_allocated) {
+                             uint64_t* new_first_free_entry) {
   GenericLogEntriesVector log_entries;
   CephContext *cct = m_image_ctx.cct;
   uint64_t span_payload_len = 0;
-  bytes_allocated = 0;
+  uint64_t bytes_to_free = 0;
   ldout(cct, 20) << "Appending " << ops.size() << " log entries." << dendl;
 
   AioTransContext* aio = new AioTransContext(cct, ctx);
@@ -756,7 +748,7 @@ void WriteLog<I>::append_ops(GenericLogOperations &ops, Context *ctx,
     if (log_entries.size() == CONTROL_BLOCK_MAX_LOG_ENTRIES ||
         span_payload_len >= SPAN_MAX_DATA_LEN) {
       if (log_entries.size() > 1) {
-        bytes_allocated += (log_entries.size() - 1) * MIN_WRITE_ALLOC_SSD_SIZE;
+        bytes_to_free += (log_entries.size() - 1) * MIN_WRITE_ALLOC_SSD_SIZE;
       }
       write_log_entries(log_entries, aio);
       log_entries.clear();
@@ -767,10 +759,17 @@ void WriteLog<I>::append_ops(GenericLogOperations &ops, Context *ctx,
   }
   if (!span_payload_len || !log_entries.empty()) {
     if (log_entries.size() > 1) {
-      bytes_allocated += (log_entries.size() - 1) * MIN_WRITE_ALLOC_SSD_SIZE;
+      bytes_to_free += (log_entries.size() - 1) * MIN_WRITE_ALLOC_SSD_SIZE;
     }
     write_log_entries(log_entries, aio);
   }
+
+  {
+    std::lock_guard locker1(m_lock);
+    m_first_free_entry = *new_first_free_entry;
+    m_bytes_allocated -= bytes_to_free;
+  }
+
   bdev->aio_submit(&aio->ioc);
   *new_first_free_entry = pool_root.first_free_entry;
 }

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.h b/src/librbd/cache/pwl/ssd/WriteLog.h
index 83633dabf425e9e4133851fd617b9ba666b01ea6..30f8bb23bf0ea100f97c0272d291c16f95e9807e 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.h
+++ b/src/librbd/cache/pwl/ssd/WriteLog.h
@@ -123,8 +123,7 @@ private:
   Context* construct_flush_entry_ctx(
       std::shared_ptr<GenericLogEntry> log_entry);
   void append_ops(GenericLogOperations &ops, Context *ctx,
-                  uint64_t* new_first_free_entry,
-                  uint64_t &bytes_allocated);
+                  uint64_t* new_first_free_entry);
   void write_log_entries(GenericLogEntriesVector log_entries,
                          AioTransContext *aio);
   void schedule_update_root(std::shared_ptr<WriteLogPoolRoot> root,