From 43895d31477772300e0bc8933004dcd7ea9df5b7 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Tue, 27 Aug 2019 23:38:48 -0400 Subject: [PATCH] nvme_disk: Better protection against underflows. This was using unsigned integer math and then trying to clamp to 0. That won't work. Use off_t instead, which is an int64 and thus signed. May fix behavior in some stranger error conditions. While I'm at it, avoid reading in the beginning partial block if we don't need to. --- .../kernel/drivers/disk/nvme/nvme_disk.cpp | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/add-ons/kernel/drivers/disk/nvme/nvme_disk.cpp b/src/add-ons/kernel/drivers/disk/nvme/nvme_disk.cpp index 5fc10d0a97..1aa8abe48e 100644 --- a/src/add-ons/kernel/drivers/disk/nvme/nvme_disk.cpp +++ b/src/add-ons/kernel/drivers/disk/nvme/nvme_disk.cpp @@ -75,8 +75,8 @@ typedef struct { pci_info info; struct nvme_ctrlr* ctrlr; - struct nvme_ns* ns; + struct nvme_ns* ns; uint64 capacity; uint32 block_size; size_t max_transfer_size; @@ -418,13 +418,13 @@ nvme_disk_read(void* cookie, off_t pos, void* buffer, size_t* length) return B_NO_MEMORY; } - status_t status = nvme_disk_read(cookie, rounded_pos, bounceBuffer, - &rounded_len); + status_t status = do_nvme_segmented_io(handle->info, rounded_pos, + bounceBuffer, &rounded_len); if (status != B_OK) { // The "rounded_len" will be the actual transferred length, but // of course it will contain the padding. - *length = std::min(*length, std::max((size_t)0, - rounded_len - (size_t)(pos - rounded_pos))); + *length = std::min(*length, (size_t)std::max((off_t)0, + rounded_len - (off_t)(pos - rounded_pos))); if (*length == 0) return status; } @@ -437,8 +437,8 @@ nvme_disk_read(void* cookie, off_t pos, void* buffer, size_t* length) return status; } - // If we got here, that means the arguments are already rounded to LBAs, - // so just do the I/O directly. + // If we got here, that means the arguments are already rounded to LBAs + // and the buffer is a kernel one, so just do the I/O directly. return do_nvme_segmented_io(handle->info, pos, buffer, length); } @@ -465,11 +465,14 @@ nvme_disk_write(void* cookie, off_t pos, const void* buffer, size_t* length) // blocks before we copy our information to the bounce buffer. // TODO: This would be faster if we queued both reads at once! size_t readlen = block_size; - status_t status = do_nvme_io(handle->info, rounded_pos, bounceBuffer, - &readlen); - if (status != B_OK) { - *length = 0; - return status; + status_t status; + if (rounded_pos != pos) { + status = do_nvme_io(handle->info, rounded_pos, bounceBuffer, + &readlen); + if (status != B_OK) { + *length = 0; + return status; + } } if (rounded_len > block_size) { off_t offset = rounded_len - block_size; @@ -481,6 +484,7 @@ nvme_disk_write(void* cookie, off_t pos, const void* buffer, size_t* length) } } + // Now we can copy in the actual data to be written. void* offsetBuffer = ((int8*)bounceBuffer) + (pos - rounded_pos); if (IS_USER_ADDRESS(buffer)) status = user_memcpy(offsetBuffer, buffer, *length); @@ -491,11 +495,11 @@ nvme_disk_write(void* cookie, off_t pos, const void* buffer, size_t* length) return status; } - status = nvme_disk_write(cookie, rounded_pos, bounceBuffer, - &rounded_len); + status = do_nvme_segmented_io(handle->info, rounded_pos, bounceBuffer, + &rounded_len, true); if (status != B_OK) { - *length = std::min(*length, std::max((size_t)0, - rounded_len - (size_t)(pos - rounded_pos))); + *length = std::min(*length, (size_t)std::max((off_t)0, + rounded_len - (off_t)(pos - rounded_pos))); } return status; }