From 3ecbb34240d2f647222aa53ff0b5dca7f3873c33 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Fri, 9 Aug 2024 18:03:21 -0400 Subject: [PATCH] IORequest: Correct major oversight in finished callback API. The IORequest internally likes to deal with transferEndOffset not transferredBytes because of sub-requests potentially being prepared all at once (in some paths in the I/O scheduler), thus fTransferSize can get incremented in Advance() before we have actually executed that transfer. But external consumers much prefer just knowing transferredBytes not transferEndOffset. And many of them actually named their variables that (or "bytesTransferred") and just passed the transferEndOffset through to variables with that name! That's obviously wrong, and it's surprising it wasn't discovered before now. The problem was uncovered by repeated KDLs in PrecacheIO. That method used the "bytesTransferred" value as a count of pages transferred, which would then run past the end of the array if the transfer start offset was not 0 (which the majority of the time it would be, since this method gets called on the first mmap() of a file, probably before any pages are read in.) Most other consumers of this API did not check the value, it seems, or otherwise had some mitigating factor that prevented it from causing more problems. An exception is the page code, which may have spuriously considered writes as successful when they really weren't. May fix some of the "invalid concurrent access to page" KDLs. --- headers/private/kernel/vfs.h | 4 ++-- src/system/kernel/cache/file_cache.cpp | 2 +- src/system/kernel/device_manager/IORequest.cpp | 12 ++++++------ src/system/kernel/device_manager/IORequest.h | 5 ++--- src/system/kernel/fs/vfs_request_io.cpp | 17 +++++++---------- src/system/kernel/vm/VMAnonymousCache.cpp | 1 - src/system/kernel/vm/vm_page.cpp | 1 + 7 files changed, 19 insertions(+), 23 deletions(-) diff --git a/headers/private/kernel/vfs.h b/headers/private/kernel/vfs.h index 30392ed14c..5f1da653a8 100644 --- a/headers/private/kernel/vfs.h +++ b/headers/private/kernel/vfs.h @@ -302,10 +302,10 @@ public: bool partialTransfer, generic_size_t bytesTransferred) = 0; - static status_t IORequestCallback(void* data, + static void IORequestCallback(void* data, io_request* request, status_t status, bool partialTransfer, - generic_size_t transferEndOffset); + generic_size_t bytesTransferred); }; diff --git a/src/system/kernel/cache/file_cache.cpp b/src/system/kernel/cache/file_cache.cpp index 49b6dee5b7..5ef0929165 100644 --- a/src/system/kernel/cache/file_cache.cpp +++ b/src/system/kernel/cache/file_cache.cpp @@ -195,7 +195,7 @@ PrecacheIO::IOFinished(status_t status, bool partialTransfer, phys_size_t pagesTransferred = (bytesTransferred + B_PAGE_SIZE - 1) / B_PAGE_SIZE; - if (fOffset + (off_t)bytesTransferred > fCache->virtual_end) + if ((fOffset + (off_t)bytesTransferred) > fCache->virtual_end) bytesTransferred = fCache->virtual_end - fOffset; for (uint32 i = 0; i < pagesTransferred; i++) { diff --git a/src/system/kernel/device_manager/IORequest.cpp b/src/system/kernel/device_manager/IORequest.cpp index 0ed0aaae54..48b2e10d33 100644 --- a/src/system/kernel/device_manager/IORequest.cpp +++ b/src/system/kernel/device_manager/IORequest.cpp @@ -965,6 +965,7 @@ IORequest::NotifyFinished() ASSERT(fPendingChildren == 0); ASSERT(fChildren.IsEmpty() || dynamic_cast(fChildren.Head()) == NULL); + ASSERT(fTransferSize <= fLength); // unlock the memory if (fBuffer->IsMemoryLocked()) @@ -977,8 +978,9 @@ IORequest::NotifyFinished() io_request_finished_callback finishedCallback = fFinishedCallback; void* finishedCookie = fFinishedCookie; status_t status = fStatus; + generic_size_t transferredBytes = fTransferSize; generic_size_t lastTransferredOffset - = fRelativeParentOffset + fTransferSize; + = fRelativeParentOffset + transferredBytes; bool partialTransfer = status != B_OK || fPartialTransfer; bool deleteRequest = (fFlags & B_DELETE_IO_REQUEST) != 0; @@ -991,7 +993,7 @@ IORequest::NotifyFinished() // notify callback if (finishedCallback != NULL) { finishedCallback(finishedCookie, this, status, partialTransfer, - lastTransferredOffset); + transferredBytes); } // notify parent @@ -1076,8 +1078,8 @@ void IORequest::SubRequestFinished(IORequest* request, status_t status, bool partialTransfer, generic_size_t transferEndOffset) { - TRACE("IORequest::SubrequestFinished(%p, %#" B_PRIx32 ", %d, %" - B_PRIuGENADDR "): request: %p\n", request, status, partialTransfer, transferEndOffset, this); + TRACE("IORequest::SubrequestFinished(%p, %#" B_PRIx32 ", %d, %" B_PRIuGENADDR + "): request: %p\n", request, status, partialTransfer, transferEndOffset, this); MutexLocker locker(fLock); @@ -1122,8 +1124,6 @@ IORequest::SetTransferredBytes(bool partialTransfer, MutexLocker _(fLock); - ASSERT(transferredBytes <= fLength); - fPartialTransfer = partialTransfer; fTransferSize = transferredBytes; } diff --git a/src/system/kernel/device_manager/IORequest.h b/src/system/kernel/device_manager/IORequest.h index 8b262967e9..3d113a23de 100644 --- a/src/system/kernel/device_manager/IORequest.h +++ b/src/system/kernel/device_manager/IORequest.h @@ -199,10 +199,9 @@ typedef IOOperation io_operation; typedef DoublyLinkedList IOOperationList; typedef struct IORequest io_request; -typedef status_t (*io_request_finished_callback)(void* data, +typedef void (*io_request_finished_callback)(void* data, io_request* request, status_t status, bool partialTransfer, - generic_size_t transferEndOffset); - // TODO: Return type: status_t -> void + generic_size_t transferredBytes); typedef status_t (*io_request_iterate_callback)(void* data, io_request* request, bool* _partialTransfer); diff --git a/src/system/kernel/fs/vfs_request_io.cpp b/src/system/kernel/fs/vfs_request_io.cpp index c8335244f6..57214cc41c 100644 --- a/src/system/kernel/fs/vfs_request_io.cpp +++ b/src/system/kernel/fs/vfs_request_io.cpp @@ -26,13 +26,12 @@ AsyncIOCallback::~AsyncIOCallback() } -/* static */ status_t +/* static */ void AsyncIOCallback::IORequestCallback(void* data, io_request* request, - status_t status, bool partialTransfer, generic_size_t transferEndOffset) + status_t status, bool partialTransfer, generic_size_t bytesTransferred) { ((AsyncIOCallback*)data)->IOFinished(status, partialTransfer, - transferEndOffset); - return B_OK; + bytesTransferred); } @@ -257,27 +256,25 @@ do_iterative_fd_io_iterate(void* _cookie, io_request* request, } -static status_t +static void do_iterative_fd_io_finish(void* _cookie, io_request* request, status_t status, - bool partialTransfer, generic_size_t transferEndOffset) + bool partialTransfer, generic_size_t bytesTransferred) { iterative_io_cookie* cookie = (iterative_io_cookie*)_cookie; if (cookie->finished != NULL) { cookie->finished(cookie->cookie, request, status, partialTransfer, - transferEndOffset); + bytesTransferred); } put_fd(cookie->descriptor); if (cookie->next_finished_callback != NULL) { cookie->next_finished_callback(cookie->next_finished_cookie, request, - status, partialTransfer, transferEndOffset); + status, partialTransfer, bytesTransferred); } delete cookie; - - return B_OK; } diff --git a/src/system/kernel/vm/VMAnonymousCache.cpp b/src/system/kernel/vm/VMAnonymousCache.cpp index 6f94fcfc9f..04bae4bbfe 100644 --- a/src/system/kernel/vm/VMAnonymousCache.cpp +++ b/src/system/kernel/vm/VMAnonymousCache.cpp @@ -429,7 +429,6 @@ public: } fNextCallback->IOFinished(status, partialTransfer, bytesTransferred); - delete this; } diff --git a/src/system/kernel/vm/vm_page.cpp b/src/system/kernel/vm/vm_page.cpp index 3b43b16083..ee4b956bac 100644 --- a/src/system/kernel/vm/vm_page.cpp +++ b/src/system/kernel/vm/vm_page.cpp @@ -1966,6 +1966,7 @@ public: virtual void IOFinished(status_t status, bool partialTransfer, generic_size_t bytesTransferred); + private: PageWriterRun* fRun; struct VMCache* fCache;