From bd02d81c24550ef4bfba6d59091a147f632e5120 Mon Sep 17 00:00:00 2001 From: David Sebek Date: Wed, 30 Jun 2021 17:49:04 -0400 Subject: [PATCH] Fix trim-related issues Fixes: * Use uint64 instead of off_t when handling offset and size of the trimmed range in the fs_trim_data structure * BlockAllocator::Trim: Correct the size of a buffer * ram_disk, mmc: Do not trim past device capacity Improvements: * BlockAllocator::Trim: Because the received offset and size are ignored by BFS (the functionality is not implemented yet), return B_UNSUPPORTED if the range does not cover the whole partition * ram_disk, mmc: More accurate calculation of the number of trimmed bytes * devfs: Add a uint64 version of translate_partition_access() Change-Id: I24f4c08674f123ad33a5fef6e28996a4ada6ff0d Reviewed-on: https://review.haiku-os.org/c/haiku/+/4155 Tested-by: Commit checker robot Reviewed-by: Adrien Destugues --- headers/os/drivers/Drivers.h | 4 +- headers/private/kernel/util/fs_trim_support.h | 2 +- .../kernel/drivers/disk/mmc/mmc_disk.cpp | 43 ++++++++++--- .../disk/virtual/ram_disk/ram_disk.cpp | 50 +++++++++++---- .../file_systems/bfs/BlockAllocator.cpp | 39 +++++++++--- .../kernel/file_systems/bfs/BlockAllocator.h | 2 +- src/system/kernel/device_manager/devfs.cpp | 63 +++++++++++++++++-- 7 files changed, 164 insertions(+), 39 deletions(-) diff --git a/headers/os/drivers/Drivers.h b/headers/os/drivers/Drivers.h index a8b4d7f788..a854305b38 100644 --- a/headers/os/drivers/Drivers.h +++ b/headers/os/drivers/Drivers.h @@ -175,8 +175,8 @@ typedef struct { uint32 range_count; uint64 trimmed_size; /* filled on return */ struct range { - off_t offset; /* offset (in bytes) */ - off_t size; + uint64 offset; /* offset (in bytes) */ + uint64 size; } ranges[1]; } fs_trim_data; diff --git a/headers/private/kernel/util/fs_trim_support.h b/headers/private/kernel/util/fs_trim_support.h index 317929faa9..ea740f915e 100644 --- a/headers/private/kernel/util/fs_trim_support.h +++ b/headers/private/kernel/util/fs_trim_support.h @@ -64,7 +64,7 @@ copy_trim_data_to_user(void* buffer, fs_trim_data* trimData) return B_BAD_ADDRESS; // Do not copy any ranges - return user_memcpy(buffer, trimData, sizeof(uint64) * 2); + return user_memcpy(buffer, trimData, offsetof(fs_trim_data, ranges)); } diff --git a/src/add-ons/kernel/drivers/disk/mmc/mmc_disk.cpp b/src/add-ons/kernel/drivers/disk/mmc/mmc_disk.cpp index 66a66b5ce4..ae8fd000e3 100644 --- a/src/add-ons/kernel/drivers/disk/mmc/mmc_disk.cpp +++ b/src/add-ons/kernel/drivers/disk/mmc/mmc_disk.cpp @@ -491,38 +491,62 @@ mmc_block_trim(mmc_disk_driver_info* info, fs_trim_data* trimData) }; TRACE("trim_device()\n"); + trimData->trimmed_size = 0; + + const off_t deviceSize = info->DeviceSize(); // in bytes + if (deviceSize < 0) + return B_BAD_VALUE; + + STATIC_ASSERT(sizeof(deviceSize) <= sizeof(uint64)); + ASSERT(deviceSize >= 0); + + // Do not trim past device end + for (uint32 i = 0; i < trimData->range_count; i++) { + uint64 offset = trimData->ranges[i].offset; + uint64& size = trimData->ranges[i].size; + + if (offset >= (uint64)deviceSize) + return B_BAD_VALUE; + size = min_c(size, (uint64)deviceSize - offset); + } + uint64 trimmedSize = 0; status_t result = B_OK; for (uint32 i = 0; i < trimData->range_count; i++) { - off_t offset = trimData->ranges[i].offset; - off_t length = trimData->ranges[i].size; + uint64 offset = trimData->ranges[i].offset; + uint64 length = trimData->ranges[i].size; // Round up offset and length to multiple of the sector size // The offset is rounded up, so some space may be left // (not trimmed) at the start of the range. offset = ROUNDUP(offset, kBlockSize); // Adjust the length for the possibly skipped range - length -= trimData->ranges[i].offset - offset; + length -= offset - trimData->ranges[i].offset; // The length is rounded down, so some space at the end may also // be left (not trimmed). length &= ~(kBlockSize - 1); - if (length == 0) { - trimmedSize += trimData->ranges[i].size; + if (length == 0) continue; - } - TRACE("trim %" B_PRIdOFF " bytes from %" B_PRIdOFF "\n", + TRACE("trim %" B_PRIu64 " bytes from %" B_PRIu64 "\n", length, offset); ASSERT(offset % kBlockSize == 0); ASSERT(length % kBlockSize == 0); - if (info->flags & kIoCommandOffsetAsSectors) { + if ((info->flags & kIoCommandOffsetAsSectors) != 0) { offset /= kBlockSize; length /= kBlockSize; } + // Parameter of execute_command is uint32_t + if (offset > UINT32_MAX + || length > UINT32_MAX - offset) { + result = B_BAD_VALUE; + break; + } + uint32_t response; result = info->mmc->execute_command(info->parent, info->parentCookie, info->rca, SD_ERASE_WR_BLK_START, offset, &response); @@ -537,7 +561,8 @@ mmc_block_trim(mmc_disk_driver_info* info, fs_trim_data* trimData) if (result != B_OK) break; - trimmedSize += trimData->ranges[i].size; + trimmedSize += (info->flags & kIoCommandOffsetAsSectors) != 0 + ? length * kBlockSize : length; } trimData->trimmed_size = trimmedSize; diff --git a/src/add-ons/kernel/drivers/disk/virtual/ram_disk/ram_disk.cpp b/src/add-ons/kernel/drivers/disk/virtual/ram_disk/ram_disk.cpp index 280e370abb..be4e6126c3 100644 --- a/src/add-ons/kernel/drivers/disk/virtual/ram_disk/ram_disk.cpp +++ b/src/add-ons/kernel/drivers/disk/virtual/ram_disk/ram_disk.cpp @@ -531,54 +531,78 @@ struct RawDevice : Device, DoublyLinkedListLinkImpl { { TRACE("trim_device()\n"); + trimData->trimmed_size = 0; + + const off_t deviceSize = fDeviceSize; // in bytes + if (deviceSize < 0) + return B_BAD_VALUE; + + STATIC_ASSERT(sizeof(deviceSize) <= sizeof(uint64)); + ASSERT(deviceSize >= 0); + + // Do not trim past device end + for (uint32 i = 0; i < trimData->range_count; i++) { + uint64 offset = trimData->ranges[i].offset; + uint64& size = trimData->ranges[i].size; + + if (offset >= (uint64)deviceSize) + return B_BAD_VALUE; + size = min_c(size, (uint64)deviceSize - offset); + } + + status_t result = B_OK; uint64 trimmedSize = 0; for (uint32 i = 0; i < trimData->range_count; i++) { - trimmedSize += trimData->ranges[i].size; - - off_t offset = trimData->ranges[i].offset; - off_t length = trimData->ranges[i].size; + uint64 offset = trimData->ranges[i].offset; + uint64 length = trimData->ranges[i].size; // Round up offset and length to multiple of the page size // The offset is rounded up, so some space may be left // (not trimmed) at the start of the range. offset = (offset + B_PAGE_SIZE - 1) & ~(B_PAGE_SIZE - 1); // Adjust the length for the possibly skipped range - length -= trimData->ranges[i].offset - offset; + length -= offset - trimData->ranges[i].offset; // The length is rounded down, so some space at the end may also // be left (not trimmed). length &= ~(B_PAGE_SIZE - 1); - TRACE("ramdisk: trim %" B_PRIdOFF " bytes from %" B_PRIdOFF "\n", + if (length == 0) + continue; + + TRACE("ramdisk: trim %" B_PRIu64 " bytes from %" B_PRIu64 "\n", length, offset); ASSERT(offset % B_PAGE_SIZE == 0); ASSERT(length % B_PAGE_SIZE == 0); vm_page** pages = new(std::nothrow) vm_page*[length / B_PAGE_SIZE]; - if (pages == NULL) - return B_NO_MEMORY; + if (pages == NULL) { + result = B_NO_MEMORY; + break; + } ArrayDeleter pagesDeleter(pages); - _GetPages(offset, length, false, pages); + _GetPages((off_t)offset, (off_t)length, false, pages); AutoLocker locker(fCache); - uint32 j; + uint64 j; for (j = 0; j < length / B_PAGE_SIZE; j++) { // If we run out of pages (some may already be trimmed), stop. if (pages[j] == NULL) break; - TRACE("free range %" B_PRIu32 ", page %" B_PRIu32 ", offset %" - B_PRIdOFF "\n", i, j, offset); + TRACE("free range %" B_PRIu32 ", page %" B_PRIu64 ", offset %" + B_PRIu64 "\n", i, j, offset); if (pages[j]->Cache()) fCache->RemovePage(pages[j]); vm_page_free(NULL, pages[j]); + trimmedSize += B_PAGE_SIZE; } } trimData->trimmed_size = trimmedSize; - return B_OK; + return result; } diff --git a/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp b/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp index e78a35897e..af0bdaa831 100644 --- a/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp +++ b/src/add-ons/kernel/file_systems/bfs/BlockAllocator.cpp @@ -1158,9 +1158,18 @@ BlockAllocator::_CheckGroup(int32 groupIndex) const status_t BlockAllocator::Trim(uint64 offset, uint64 size, uint64& trimmedSize) { + // TODO: Remove this check when offset and size handling is implemented + if (offset != 0 + || fVolume->NumBlocks() < 0 + || size < (uint64)fVolume->NumBlocks() * fVolume->BlockSize()) { + INFORM(("BFS Trim: Ranges smaller than the file system size" + " are not supported yet.\n")); + return B_UNSUPPORTED; + } + const uint32 kTrimRanges = 128; fs_trim_data* trimData = (fs_trim_data*)malloc(sizeof(fs_trim_data) - + sizeof(uint64) * kTrimRanges); + + 2 * sizeof(uint64) * (kTrimRanges - 1)); if (trimData == NULL) return B_NO_MEMORY; @@ -1175,7 +1184,7 @@ BlockAllocator::Trim(uint64 offset, uint64 size, uint64& trimmedSize) uint32 blockShift = fVolume->BlockShift(); uint64 firstFree = 0; - size_t freeLength = 0; + uint64 freeLength = 0; trimData->range_count = 0; trimmedSize = 0; @@ -1191,6 +1200,15 @@ BlockAllocator::Trim(uint64 offset, uint64 size, uint64& trimmedSize) if (cached.IsUsed(i)) { // Block is in use if (freeLength > 0) { + // Overflow is unlikely to happen, but check it anyway + if ((firstFree << blockShift) >> blockShift + != firstFree + || (freeLength << blockShift) >> blockShift + != freeLength) { + FATAL(("BlockAllocator::Trim:" + " Overflow detected!\n")); + return B_ERROR; + } status_t status = _TrimNext(*trimData, kTrimRanges, firstFree << blockShift, freeLength << blockShift, false, trimmedSize); @@ -1306,7 +1324,7 @@ BlockAllocator::CheckBlockRun(block_run run, const char* type, bool allocated) } -status_t +bool BlockAllocator::_AddTrim(fs_trim_data& trimData, uint32 maxRanges, uint64 offset, uint64 size) { @@ -1333,13 +1351,16 @@ BlockAllocator::_TrimNext(fs_trim_data& trimData, uint32 maxRanges, if (!pushed || force) { // Trim now trimData.trimmed_size = 0; -dprintf("TRIM FS:\n"); -for (uint32 i = 0; i < trimData.range_count; i++) { - dprintf("[%3" B_PRIu32 "] %" B_PRIu64 " : %" B_PRIu64 "\n", i, - trimData.ranges[i].offset, trimData.ranges[i].size); -} +#ifdef DEBUG_TRIM + dprintf("TRIM: BFS: free ranges (bytes):\n"); + for (uint32 i = 0; i < trimData.range_count; i++) { + dprintf("[%3" B_PRIu32 "] %" B_PRIu64 " : %" B_PRIu64 "\n", i, + trimData.ranges[i].offset, trimData.ranges[i].size); + } +#endif if (ioctl(fVolume->Device(), B_TRIM_DEVICE, &trimData, - sizeof(fs_trim_data)) != 0) { + sizeof(fs_trim_data) + + 2 * sizeof(uint64) * (trimData.range_count - 1)) != 0) { return errno; } diff --git a/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h b/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h index 7d81292e56..f2148048d6 100644 --- a/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h +++ b/src/add-ons/kernel/file_systems/bfs/BlockAllocator.h @@ -68,7 +68,7 @@ private: #ifdef DEBUG_ALLOCATION_GROUPS void _CheckGroup(int32 group) const; #endif - status_t _AddTrim(fs_trim_data& trimData, uint32 maxRanges, + bool _AddTrim(fs_trim_data& trimData, uint32 maxRanges, uint64 offset, uint64 size); status_t _TrimNext(fs_trim_data& trimData, uint32 maxRanges, uint64 offset, uint64 size, bool force, diff --git a/src/system/kernel/device_manager/devfs.cpp b/src/system/kernel/device_manager/devfs.cpp index c6b45616e4..ec413dc8aa 100644 --- a/src/system/kernel/device_manager/devfs.cpp +++ b/src/system/kernel/device_manager/devfs.cpp @@ -495,18 +495,49 @@ err1: } -template static inline void +static inline void translate_partition_access(devfs_partition* partition, off_t& offset, - size_type& size) + size_t& size) { ASSERT(offset >= 0); ASSERT(offset < partition->info.size); - size = (size_type)min_c((off_t)size, partition->info.size - offset); + size = (size_t)min_c((off_t)size, partition->info.size - offset); offset += partition->info.offset; } +static bool +translate_partition_access(devfs_partition* partition, uint64& offset, + uint64& size) +{ + const off_t partitionSize = partition->info.size; + const off_t partitionOffset = partition->info.offset; + + // Check that off_t values can be cast to uint64, + // partition offset can theoretically be negative + ASSERT(partitionSize >= 0); + STATIC_ASSERT(sizeof(partitionSize) <= sizeof(uint64)); + STATIC_ASSERT(sizeof(partitionOffset) <= sizeof(uint64)); + + // Check that calculations give expected results + if (offset >= (uint64)partitionSize) + return false; + if (partitionOffset >= 0 && offset > UINT64_MAX - (uint64)partitionOffset) + return false; + if (partitionOffset < 0 && offset < (uint64)-partitionOffset) + return false; + + size = min_c(size, (uint64)partitionSize - offset); + if (partitionOffset >= 0) + offset += (uint64)partitionOffset; + else + offset -= (uint64)-partitionOffset; + + return true; +} + + static inline void translate_partition_access(devfs_partition* partition, io_request* request) { @@ -1479,14 +1510,38 @@ devfs_ioctl(fs_volume* _volume, fs_vnode* _vnode, void* _cookie, uint32 op, if (status != B_OK) return status; +#ifdef DEBUG_TRIM + dprintf("TRIM: devfs: received TRIM ranges (bytes):\n"); + for (uint32 i = 0; i < trimData->range_count; i++) { + dprintf("[%3" B_PRIu32 "] %" B_PRIu64 " : %" + B_PRIu64 "\n", i, + trimData->ranges[i].offset, + trimData->ranges[i].size); + } +#endif + if (partition != NULL) { // If there is a partition, offset all ranges according // to the partition start. + // Range size may be reduced to fit the partition size. for (uint32 i = 0; i < trimData->range_count; i++) { - translate_partition_access(partition, + if (!translate_partition_access(partition, + trimData->ranges[i].offset, + trimData->ranges[i].size)) { + return B_BAD_VALUE; + } + } + +#ifdef DEBUG_TRIM + dprintf("TRIM: devfs: TRIM ranges after partition" + " translation (bytes):\n"); + for (uint32 i = 0; i < trimData->range_count; i++) { + dprintf("[%3" B_PRIu32 "] %" B_PRIu64 " : %" + B_PRIu64 "\n", i, trimData->ranges[i].offset, trimData->ranges[i].size); } +#endif } status = vnode->stream.u.dev.device->Control(