From 9bdb2e3c3105bec2041f282fb7d5104f7d69a8c4 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Mon, 7 Dec 2020 14:57:04 +0100 Subject: [PATCH] os/bluestore: Added asserts for allocator regions Functions release/init_add_free/init_rm_free did not check its input against device size. It is incorrect and had been a problem when you shrink device. Signed-off-by: Adam Kupczyk --- src/os/bluestore/Allocator.cc | 4 ++-- src/os/bluestore/Allocator.h | 8 ++++---- src/os/bluestore/AvlAllocator.cc | 3 +++ src/os/bluestore/BitmapAllocator.cc | 9 ++++++--- src/os/bluestore/BitmapAllocator.h | 1 - src/os/bluestore/StupidAllocator.cc | 14 +++++++------- src/os/bluestore/StupidAllocator.h | 5 ++--- 7 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/os/bluestore/Allocator.cc b/src/os/bluestore/Allocator.cc index 91a001b5c97..75f3172ca55 100644 --- a/src/os/bluestore/Allocator.cc +++ b/src/os/bluestore/Allocator.cc @@ -109,7 +109,7 @@ public: Allocator::Allocator(const std::string& name, int64_t _capacity, int64_t _block_size) - : capacity(_capacity), block_size(_block_size) + : device_size(_capacity), block_size(_block_size) { asok_hook = new SocketHook(this, name); } @@ -129,7 +129,7 @@ Allocator *Allocator::create(CephContext* cct, string type, { Allocator* alloc = nullptr; if (type == "stupid") { - alloc = new StupidAllocator(cct, name, size, block_size); + alloc = new StupidAllocator(cct, size, block_size, name); } else if (type == "bitmap") { alloc = new BitmapAllocator(cct, size, block_size, name); } else if (type == "avl") { diff --git a/src/os/bluestore/Allocator.h b/src/os/bluestore/Allocator.h index 177438025d7..db778a98883 100644 --- a/src/os/bluestore/Allocator.h +++ b/src/os/bluestore/Allocator.h @@ -79,7 +79,7 @@ public: const string& get_name() const; int64_t get_capacity() const { - return capacity; + return device_size; } int64_t get_block_size() const { @@ -89,9 +89,9 @@ public: private: class SocketHook; SocketHook* asok_hook = nullptr; - - int64_t capacity = 0; - int64_t block_size = 0; +protected: + const int64_t device_size = 0; + const int64_t block_size = 0; }; #endif diff --git a/src/os/bluestore/AvlAllocator.cc b/src/os/bluestore/AvlAllocator.cc index 8c43b37dc26..f6a11b4f9be 100644 --- a/src/os/bluestore/AvlAllocator.cc +++ b/src/os/bluestore/AvlAllocator.cc @@ -268,6 +268,7 @@ void AvlAllocator::_release(const interval_set& release_set) for (auto p = release_set.begin(); p != release_set.end(); ++p) { const auto offset = p.get_start(); const auto length = p.get_len(); + ceph_assert(offset + length <= uint64_t(num_total)); ldout(cct, 10) << __func__ << std::hex << " offset 0x" << offset << " length 0x" << length @@ -406,6 +407,7 @@ void AvlAllocator::dump(std::function no void AvlAllocator::init_add_free(uint64_t offset, uint64_t length) { std::lock_guard l(lock); + ceph_assert(offset + length <= uint64_t(num_total)); ldout(cct, 10) << __func__ << std::hex << " offset 0x" << offset << " length 0x" << length @@ -416,6 +418,7 @@ void AvlAllocator::init_add_free(uint64_t offset, uint64_t length) void AvlAllocator::init_rm_free(uint64_t offset, uint64_t length) { std::lock_guard l(lock); + ceph_assert(offset + length <= uint64_t(num_total)); ldout(cct, 10) << __func__ << std::hex << " offset 0x" << offset << " length 0x" << length diff --git a/src/os/bluestore/BitmapAllocator.cc b/src/os/bluestore/BitmapAllocator.cc index 75f909c071e..3571ddd2192 100644 --- a/src/os/bluestore/BitmapAllocator.cc +++ b/src/os/bluestore/BitmapAllocator.cc @@ -52,9 +52,10 @@ void BitmapAllocator::release( const interval_set& release_set) { if (cct->_conf->subsys.should_gather()) { - for (auto r : release_set) { - ldout(cct, 10) << __func__ << " 0x" << std::hex << r.first << "~" << r.second - << std::dec << dendl; + for (auto& [offset, len] : release_set) { + ldout(cct, 10) << __func__ << " 0x" << std::hex << offset << "~" << len + << std::dec << dendl; + ceph_assert(offset + len <= (uint64_t)device_size); } } _free_l2(release_set); @@ -70,6 +71,7 @@ void BitmapAllocator::init_add_free(uint64_t offset, uint64_t length) auto mas = get_min_alloc_size(); uint64_t offs = round_up_to(offset, mas); uint64_t l = p2align(offset + length - offs, mas); + ceph_assert(offs + l <= (uint64_t)device_size); _mark_free(offs, l); ldout(cct, 10) << __func__ << " done" << dendl; @@ -81,6 +83,7 @@ void BitmapAllocator::init_rm_free(uint64_t offset, uint64_t length) auto mas = get_min_alloc_size(); uint64_t offs = round_up_to(offset, mas); uint64_t l = p2align(offset + length - offs, mas); + ceph_assert(offs + l <= (uint64_t)device_size); _mark_allocated(offs, l); ldout(cct, 10) << __func__ << " done" << dendl; } diff --git a/src/os/bluestore/BitmapAllocator.h b/src/os/bluestore/BitmapAllocator.h index 5c768a4ac2f..ec61e210823 100644 --- a/src/os/bluestore/BitmapAllocator.h +++ b/src/os/bluestore/BitmapAllocator.h @@ -15,7 +15,6 @@ class BitmapAllocator : public Allocator, public AllocatorLevel02 { CephContext* cct; - public: BitmapAllocator(CephContext* _cct, int64_t capacity, int64_t alloc_unit, const std::string& name); ~BitmapAllocator() override diff --git a/src/os/bluestore/StupidAllocator.cc b/src/os/bluestore/StupidAllocator.cc index 533f279d780..51d444170c4 100644 --- a/src/os/bluestore/StupidAllocator.cc +++ b/src/os/bluestore/StupidAllocator.cc @@ -11,14 +11,15 @@ #define dout_prefix *_dout << "stupidalloc 0x" << this << " " StupidAllocator::StupidAllocator(CephContext* cct, - const std::string& name, - int64_t _size, - int64_t _block_size) - : Allocator(name, _size, _block_size), cct(cct), num_free(0), + int64_t capacity, + int64_t _block_size, + const std::string& name) + : Allocator(name, capacity, _block_size), + cct(cct), num_free(0), free(10) { ceph_assert(cct != nullptr); - bdev_block_size = cct->_conf->bdev_block_size; + ceph_assert(block_size > 0); } StupidAllocator::~StupidAllocator() @@ -27,8 +28,7 @@ StupidAllocator::~StupidAllocator() unsigned StupidAllocator::_choose_bin(uint64_t orig_len) { - ceph_assert(bdev_block_size > 0); - uint64_t len = orig_len / bdev_block_size; + uint64_t len = orig_len / block_size; int bin = std::min((int)cbits(len), (int)free.size() - 1); ldout(cct, 30) << __func__ << " len 0x" << std::hex << orig_len << std::dec << " -> " << bin << dendl; diff --git a/src/os/bluestore/StupidAllocator.h b/src/os/bluestore/StupidAllocator.h index bd99a7c835f..9b18e4bb674 100644 --- a/src/os/bluestore/StupidAllocator.h +++ b/src/os/bluestore/StupidAllocator.h @@ -18,7 +18,6 @@ class StupidAllocator : public Allocator { ceph::mutex lock = ceph::make_mutex("StupidAllocator::lock"); int64_t num_free; ///< total bytes in freelist - uint64_t bdev_block_size; template using allocator_t = mempool::bluestore_alloc::pool_allocator>; @@ -38,9 +37,9 @@ class StupidAllocator : public Allocator { public: StupidAllocator(CephContext* cct, - const std::string& name, int64_t size, - int64_t block_size); + int64_t block_size, + const std::string& name); ~StupidAllocator() override; const char* get_type() const override { -- 2.39.5