From ee005111fb6cc6557463e544cac424372c02d3a7 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Tue, 8 Jun 2021 09:41:44 +0800 Subject: [PATCH] crimson/onode-staged-tree: validate node size where possible Signed-off-by: Yingxin Cheng --- .../onode_manager/staged-fltree/fwd.h | 5 +++++ .../staged-fltree/node_extent_accessor.h | 22 +++++++++++-------- .../staged-fltree/node_extent_mutable.h | 10 ++++++++- .../stages/item_iterator_stage.h | 1 + .../staged-fltree/stages/node_stage.h | 3 +-- .../staged-fltree/stages/sub_items_stage.h | 2 ++ .../onode_manager/staged-fltree/value.cc | 6 ++--- 7 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/fwd.h b/src/crimson/os/seastore/onode_manager/staged-fltree/fwd.h index 9180ad868a0..7f835de474b 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/fwd.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/fwd.h @@ -66,6 +66,11 @@ using node_offset_t = uint16_t; constexpr node_offset_t DISK_BLOCK_SIZE = 1u << 12; constexpr auto MAX_NODE_SIZE = (extent_len_t)std::numeric_limits::max() + 1; +inline bool is_valid_node_size(extent_len_t node_size) { + return (node_size > 0 && + node_size <= MAX_NODE_SIZE && + node_size % DISK_BLOCK_SIZE == 0); +} using string_size_t = uint16_t; diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h index b2e2d749901..afa695fa128 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h @@ -83,7 +83,6 @@ class DeltaRecorderT final: public DeltaRecorder { ceph::encode(new_addr, encoded); int node_offset = reinterpret_cast(p_addr) - p_node_start; assert(node_offset > 0 && node_offset < (int)node_size); - assert(node_offset < (int)MAX_NODE_SIZE); ceph::encode(static_cast(node_offset), encoded); } @@ -287,6 +286,7 @@ class NodeExtentAccessorT { : extent{extent}, node_stage{reinterpret_cast(extent->get_read()), extent->get_length()} { + assert(is_valid_node_size(extent->get_length())); if (extent->is_initial_pending()) { state = nextent_state_t::FRESH; mut.emplace(extent->get_mutable()); @@ -314,7 +314,7 @@ class NodeExtentAccessorT { auto ref_recorder = recorder_t::create_for_replay(); test_recorder = static_cast(ref_recorder.get()); test_extent = TestReplayExtent::create( - extent->get_length(), std::move(ref_recorder)); + get_length(), std::move(ref_recorder)); #endif } ~NodeExtentAccessorT() = default; @@ -325,7 +325,11 @@ class NodeExtentAccessorT { const node_stage_t& read() const { return node_stage; } laddr_t get_laddr() const { return extent->get_laddr(); } - extent_len_t get_length() const { return extent->get_length(); } + extent_len_t get_length() const { + auto len = extent->get_length(); + assert(is_valid_node_size(len)); + return len; + } nextent_state_t get_state() const { assert(!is_retired()); // we cannot rely on the underlying extent state because @@ -355,7 +359,7 @@ class NodeExtentAccessorT { state = nextent_state_t::MUTATION_PENDING; assert(extent->is_mutation_pending()); node_stage = node_stage_t(reinterpret_cast(extent->get_read()), - extent->get_length()); + get_length()); assert(recorder == static_cast(extent->get_recorder())); mut.emplace(extent->get_mutable()); } @@ -500,7 +504,7 @@ class NodeExtentAccessorT { void test_copy_to(NodeExtentMutable& to) const { assert(extent->get_length() == to.get_length()); - std::memcpy(to.get_write(), extent->get_read(), extent->get_length()); + std::memcpy(to.get_write(), extent->get_read(), get_length()); } eagain_future rebuild(context_t c) { @@ -512,7 +516,7 @@ class NodeExtentAccessorT { return eagain_ertr::make_ready_future(*mut); } assert(!extent->is_initial_pending()); - auto alloc_size = extent->get_length(); + auto alloc_size = get_length(); return c.nm.alloc_extent(c.t, alloc_size ).handle_error( eagain_ertr::pass_further{}, @@ -527,14 +531,14 @@ class NodeExtentAccessorT { c.t, extent->get_laddr(), fresh_extent->get_laddr()); assert(fresh_extent->is_initial_pending()); assert(fresh_extent->get_recorder() == nullptr); - assert(extent->get_length() == fresh_extent->get_length()); + assert(get_length() == fresh_extent->get_length()); auto fresh_mut = fresh_extent->get_mutable(); - std::memcpy(fresh_mut.get_write(), extent->get_read(), extent->get_length()); + std::memcpy(fresh_mut.get_write(), extent->get_read(), get_length()); NodeExtentRef to_discard = extent; extent = fresh_extent; node_stage = node_stage_t(reinterpret_cast(extent->get_read()), - extent->get_length()); + get_length()); state = nextent_state_t::FRESH; mut.emplace(fresh_mut); recorder = nullptr; diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_mutable.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_mutable.h index 5082361740f..6f92ca9edf3 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_mutable.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_mutable.h @@ -68,7 +68,14 @@ class NodeExtentMutable { const char* get_read() const { return p_start; } char* get_write() { return p_start; } - extent_len_t get_length() const { return length; } + extent_len_t get_length() const { +#ifndef NDEBUG + if (node_offset == 0) { + assert(is_valid_node_size(length)); + } +#endif + return length; + } node_offset_t get_node_offset() const { return node_offset; } NodeExtentMutable get_mutable_absolute(const void* dst, node_offset_t len) const { @@ -77,6 +84,7 @@ class NodeExtentMutable { assert((const char*)dst != get_read()); auto ret = *this; node_offset_t offset = (const char*)dst - get_read(); + assert(offset != 0); ret.p_start += offset; ret.length = len; ret.node_offset = offset; diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/item_iterator_stage.h b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/item_iterator_stage.h index 8def28b26fa..248ed606537 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/item_iterator_stage.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/item_iterator_stage.h @@ -42,6 +42,7 @@ class item_iterator_t { : node_size{range.node_size}, p_items_start(range.range.p_start), p_items_end(range.range.p_end) { + assert(is_valid_node_size(node_size)); assert(p_items_start < p_items_end); next_item_range(p_items_end); } diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/node_stage.h b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/node_stage.h index 5e05c678bbe..e10577f06db 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/node_stage.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/node_stage.h @@ -36,8 +36,7 @@ class node_extent_t { node_extent_t(const FieldType* p_fields, extent_len_t node_size) : p_fields{p_fields}, node_size{node_size} { - assert(node_size <= MAX_NODE_SIZE); - assert(node_size % DISK_BLOCK_SIZE == 0); + assert(is_valid_node_size(node_size)); validate(*p_fields); } diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/sub_items_stage.h b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/sub_items_stage.h index f2b791ed5a7..1e01c033185 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/stages/sub_items_stage.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/stages/sub_items_stage.h @@ -44,6 +44,7 @@ class internal_sub_items_t { internal_sub_items_t(const container_range_t& _range) : node_size{_range.node_size} { + assert(is_valid_node_size(node_size)); auto& range = _range.range; assert(range.p_start < range.p_end); assert((range.p_end - range.p_start) % sizeof(internal_sub_item_t) == 0); @@ -170,6 +171,7 @@ class leaf_sub_items_t { leaf_sub_items_t(const container_range_t& _range) : node_size{_range.node_size} { + assert(is_valid_node_size(node_size)); auto& range = _range.range; assert(range.p_start < range.p_end); auto _p_num_keys = range.p_end - sizeof(num_keys_t); diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/value.cc b/src/crimson/os/seastore/onode_manager/staged-fltree/value.cc index bfaba92b4dc..2b4ec61c8ef 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/value.cc +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/value.cc @@ -111,10 +111,8 @@ void validate_tree_config(const tree_conf_t& conf) string_key_view_t::VALID_UPPER_BOUND); ceph_assert(conf.max_oid_size < string_key_view_t::VALID_UPPER_BOUND); - ceph_assert(conf.internal_node_size <= MAX_NODE_SIZE); - ceph_assert(conf.internal_node_size % DISK_BLOCK_SIZE == 0); - ceph_assert(conf.leaf_node_size <= MAX_NODE_SIZE); - ceph_assert(conf.leaf_node_size % DISK_BLOCK_SIZE == 0); + ceph_assert(is_valid_node_size(conf.internal_node_size)); + ceph_assert(is_valid_node_size(conf.leaf_node_size)); if (conf.do_split_check) { // In hope to comply with 3 * (oid + ns) + 2 * value < node -- 2.39.5