From 9605a60eac00ec296d91af4cfe3c56baddb74336 Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Thu, 27 Jul 2023 14:56:25 +0200 Subject: [PATCH] tresor: no local copy of snapshots in vbd module The Virtual Block Device module used to create a local copy of the Snapshots array respectively Snapshot root it received with an incoming request. After finishing the VBD operation on the copy, the source module of the request used to back-copy the resulting Snapshot array resp. Snapshot root. This is not only less efficient than referencing but also allowed a bug to sneak into the new C++ implementation. In contrast to the old Ada/SPARK implementation (CBE), the new design doesn't allow for global objects that can be accessed by any module without receiving a reference in a module request. Therefore, the Free Tree module has to receive a reference to a Snapshots array with each request in order to be able to use it. In our case, these requests are allocations for a "Write" operation from the VBD. However, the VBD itself receives only the one Snapshot required for writing and therefore causes the Free Tree to make bad decisions on whether or not a block can be re-allocated or not. With this commit, the VBD always receive a reference to the whole Snapshots array and also propagates it this way to the Free Tree. Ref #4971 --- .../tresor/include/tresor/virtual_block_device.h | 5 +++-- repos/gems/src/lib/tresor/superblock_control.cc | 14 +++++++------- repos/gems/src/lib/tresor/virtual_block_device.cc | 12 ++++++------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/repos/gems/src/lib/tresor/include/tresor/virtual_block_device.h b/repos/gems/src/lib/tresor/include/tresor/virtual_block_device.h index 401e2c63ec..856c75a167 100644 --- a/repos/gems/src/lib/tresor/include/tresor/virtual_block_device.h +++ b/repos/gems/src/lib/tresor/include/tresor/virtual_block_device.h @@ -41,6 +41,7 @@ class Tresor::Virtual_block_device_request : public Module_request Type _type { INVALID }; Virtual_block_address _vba { 0 }; Snapshots _snapshots { }; + Snapshot_index _curr_snap_idx { 0 }; Tree_degree _snapshots_degree { 0 }; Generation _curr_gen { INVALID_GENERATION }; Key_id _new_key_id { 0 }; @@ -99,7 +100,7 @@ class Tresor::Virtual_block_device_request : public Module_request uint64_t vbd_highest_vba, bool rekeying, Virtual_block_address vba, - Snapshot const *snapshot_ptr, + Snapshot_index curr_snap_idx, Snapshots const *snapshots_ptr, Tree_degree snapshots_degree, Key_id old_key_id, @@ -117,7 +118,7 @@ class Tresor::Virtual_block_device_request : public Module_request Number_of_leaves nr_of_leaves() const { return _nr_of_leaves; } - Snapshot *snapshot_ptr() { return &_snapshots.items[0]; } + Snapshot_index curr_snap_idx() const { return _curr_snap_idx; } Snapshots *snapshots_ptr() { return &_snapshots; } diff --git a/repos/gems/src/lib/tresor/superblock_control.cc b/repos/gems/src/lib/tresor/superblock_control.cc index cc6424bf14..b58c90c4cd 100644 --- a/repos/gems/src/lib/tresor/superblock_control.cc +++ b/repos/gems/src/lib/tresor/superblock_control.cc @@ -1607,8 +1607,8 @@ bool Superblock_control::_peek_generated_request(uint8_t *buf_ptr, max_vba(), _sb.state == Superblock::REKEYING ? 1 : 0, req._vba, - &_sb.snapshots.items[_sb.curr_snap], - nullptr, + _sb.curr_snap, + &_sb.snapshots, _sb.degree, 0, 0, _curr_gen, chan._curr_key_plaintext.id, 0, 0); @@ -1638,8 +1638,8 @@ bool Superblock_control::_peek_generated_request(uint8_t *buf_ptr, max_vba(), _sb.state == Superblock::REKEYING ? 1 : 0, req._vba, - &_sb.snapshots.items[_sb.curr_snap], - nullptr, + _sb.curr_snap, + &_sb.snapshots, _sb.degree, 0, 0, _curr_gen, chan._curr_key_plaintext.id, 0, 0); @@ -1701,7 +1701,7 @@ bool Superblock_control::_peek_generated_request(uint8_t *buf_ptr, max_vba(), _sb.state == Superblock::REKEYING ? 1 : 0, _sb.rekeying_vba, - nullptr, + _sb.curr_snap, &_sb.snapshots, _sb.degree, _sb.previous_key.id, @@ -1734,7 +1734,7 @@ bool Superblock_control::_peek_generated_request(uint8_t *buf_ptr, max_vba(), _sb.state == Superblock::REKEYING ? 1 : 0, 0, - nullptr, + _sb.curr_snap, &_sb.snapshots, _sb.degree, 0, @@ -1936,7 +1936,7 @@ void Superblock_control::generated_request_complete(Module_request &mod_req) case Channel::READ_VBA_AT_VBD_IN_PROGRESS: chan._state = Channel::READ_VBA_AT_VBD_COMPLETED; break; case Channel::WRITE_VBA_AT_VBD_IN_PROGRESS: chan._state = Channel::WRITE_VBA_AT_VBD_COMPLETED; - chan._snapshots.items[0] = *(gen_req.snapshot_ptr()); + chan._snapshots.items[0] = gen_req.snapshots_ptr()->items[gen_req.curr_snap_idx()]; break; case Channel::REKEY_VBA_IN_VBD_IN_PROGRESS: chan._state = Channel::REKEY_VBA_IN_VBD_COMPLETED; diff --git a/repos/gems/src/lib/tresor/virtual_block_device.cc b/repos/gems/src/lib/tresor/virtual_block_device.cc index c63b3a3d34..4c44196c14 100644 --- a/repos/gems/src/lib/tresor/virtual_block_device.cc +++ b/repos/gems/src/lib/tresor/virtual_block_device.cc @@ -65,7 +65,7 @@ void Virtual_block_device_request::create(void *buf_ptr, uint64_t vbd_highest_vba, bool rekeying, Virtual_block_address vba, - Snapshot const *snapshot_ptr, + Snapshot_index curr_snap_idx, Snapshots const *snapshots_ptr, Tree_degree snapshots_degree, Key_id old_key_id, @@ -95,20 +95,19 @@ void Virtual_block_device_request::create(void *buf_ptr, req._vbd_highest_vba = vbd_highest_vba; req._rekeying = rekeying; req._vba = vba; + req._curr_snap_idx = curr_snap_idx; + req._snapshots = *snapshots_ptr; switch (req_type) { case READ_VBA: case WRITE_VBA: - req._snapshots.items[0] = *snapshot_ptr; req._new_key_id = key_id; break; case REKEY_VBA: - req._snapshots = *snapshots_ptr; req._old_key_id = old_key_id; req._new_key_id = new_key_id; break; case VBD_EXTENSION_STEP: - req._snapshots = *snapshots_ptr; req._pba = first_pba; req._nr_of_pbas = nr_of_pbas; break; @@ -193,6 +192,7 @@ void Virtual_block_device::submit_request(Module_request &mod_req) if (chan._request._type == Request::INVALID) { mod_req.dst_request_id(id); chan._request = *static_cast(&mod_req); + chan._vba = chan._request._vba; chan._state = Channel::SUBMITTED; return; } @@ -274,7 +274,7 @@ void Virtual_block_device::_execute_read_vba(Channel &channel, { Request &request = channel._request; - channel._snapshot_idx = 0; + channel._snapshot_idx = request._curr_snap_idx; channel._vba = request._vba; Snapshot &snapshot = channel.snapshots(channel._snapshot_idx); @@ -578,7 +578,7 @@ void Virtual_block_device::_execute_write_vba(Channel &chan, case Channel::State::SUBMITTED: { Request &request { chan._request }; - chan._snapshot_idx = 0; + chan._snapshot_idx = request._curr_snap_idx; chan._vba = request._vba; chan._t1_blk_idx = chan.snapshots(chan._snapshot_idx).max_level;