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>

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index 322b07cd53288e90c962743b4cf58a428082a8f1..748d5c69086ca1729591bfc9f3aee4879ca67a65 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -434,14 +434,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();
@@ -717,12 +710,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;
 
   *new_first_free_entry = pool_root.first_free_entry;
@@ -736,7 +728,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, new_first_free_entry);
       log_entries.clear();
@@ -747,11 +739,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, new_first_free_entry);
   }
 
+  {
+    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);
 }
 

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.h b/src/librbd/cache/pwl/ssd/WriteLog.h
index e3b2aeafe9f0e3d539e94340986a09557919fdf8..36c0605d4a92eac5e0e2f88d397d6882d724beb2 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.h
+++ b/src/librbd/cache/pwl/ssd/WriteLog.h
@@ -122,8 +122,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, uint64_t *pos);
   void schedule_update_root(std::shared_ptr<WriteLogPoolRoot> root,