From patchwork Mon Dec 4 10:22:37 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ric Li X-Patchwork-Id: 134788 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3A0AF4366A; Mon, 4 Dec 2023 11:22:46 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AD1A140294; Mon, 4 Dec 2023 11:22:45 +0100 (CET) Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 69C574028C for ; Mon, 4 Dec 2023 11:22:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701685363; x=1733221363; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=eKT2oeCv7l5KyKHi1XFUyQ6SqERfgdAntAxu4wPSmso=; b=T9NlpdkfpaTWBGRbQfGvOrV9bsD1aealHGBk6figMrgihF4rbFuX3opq KD+FjeIjvftG9mWT2EiP0NBCWZ9MrkHPhBk/Yl7tdqSHBPyo5elKSRb04 GWApy5CJJRSn3ccxmLkZHOKH2ul3NYJKrUSaDpS0ApAnL9YvnHG6cTGcV OkBWLlnb3kn2U90G1DLZtC+WnxsTnewynnLu9ESoyLeSFkDDWCHDXM9UK uMAmHjCXBz791U3kkSF1/zgLsZBNv9aNf1dSmdlKA5u1MH4soseTJl6mi SAJAGjholZ5aApYUnGecM30NpHWczyV0k7V/m+cY6j5wstWbAGvmuPqC8 Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10913"; a="458034815" X-IronPort-AV: E=Sophos;i="6.04,249,1695711600"; d="scan'208";a="458034815" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Dec 2023 02:22:40 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10913"; a="840980367" X-IronPort-AV: E=Sophos;i="6.04,249,1695711600"; d="scan'208";a="840980367" Received: from media-ric-kahawai-icl.sh.intel.com ([10.67.119.129]) by fmsmga004.fm.intel.com with ESMTP; 04 Dec 2023 02:22:39 -0800 From: Ric Li To: dmitry.kozliuk@gmail.com Cc: dev@dpdk.org, ming3.li@intel.com, roretzla@linux.microsoft.com Subject: [PATCH v4] windows/virt2phys: fix block MDL not updated Date: Mon, 4 Dec 2023 18:22:37 +0800 Message-Id: <20231204102237.2502486-1-ming3.li@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231130071001.4c7931c8@sovereign> References: <20231130071001.4c7931c8@sovereign> MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org The virt2phys_translate function previously scanned existing blocks, returning the physical address from the stored MDL info if present. This method was problematic when a virtual address pointed to a freed and reallocated memory segment, potentially changing the physical address mapping. Yet, virt2phys_translate would consistently return the originally stored physical address, which could be invalid. This issue surfaced when allocating a memory region larger than 2MB using rte_malloc. This action would allocate a new memory segment and use virt2phy to set the IOVA. The driver would store the MDL and lock the pages initially. When this region was freed, the memory segment used as a whole page could be freed, invalidating the virtual to physical mapping. Before this fix, the driver would only return the initial physical address, leading to illegal IOVA for some pages when allocating a new memory region larger than the hugepage size (2MB). To address this, a function to check block physical address has been added. If a block with the same base address is detected in the driver's context, the MDL's physical address is compared with the real physical address. If they don't match, the block is removed and a new one is created to store the correct mapping. To make the removal action clear, the list to store MDL blocks is changed to a double linked list. Also fix the printing of PVOID type. Bugzilla ID: 1201 Bugzilla ID: 1213 Signed-off-by: Ric Li --- windows/virt2phys/virt2phys.c | 7 +-- windows/virt2phys/virt2phys_logic.c | 70 ++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/windows/virt2phys/virt2phys.c b/windows/virt2phys/virt2phys.c index f4d5298..b64a13d 100644 --- a/windows/virt2phys/virt2phys.c +++ b/windows/virt2phys/virt2phys.c @@ -182,7 +182,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request) { WDF_REQUEST_PARAMETERS params; ULONG code; - PVOID *virt; + PVOID *pvirt, virt; PHYSICAL_ADDRESS *phys; size_t size; NTSTATUS status; @@ -207,12 +207,13 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request) } status = WdfRequestRetrieveInputBuffer( - request, sizeof(*virt), (PVOID *)&virt, &size); + request, sizeof(*pvirt), (PVOID *)&pvirt, &size); if (!NT_SUCCESS(status)) { TraceWarning("Retrieving input buffer: %!STATUS!", status); WdfRequestComplete(request, status); return; } + virt = *pvirt; status = WdfRequestRetrieveOutputBuffer( request, sizeof(*phys), (PVOID *)&phys, &size); @@ -222,7 +223,7 @@ virt2phys_device_EvtIoInCallerContext(WDFDEVICE device, WDFREQUEST request) return; } - status = virt2phys_translate(*virt, phys); + status = virt2phys_translate(virt, phys); if (NT_SUCCESS(status)) WdfRequestSetInformation(request, sizeof(*phys)); diff --git a/windows/virt2phys/virt2phys_logic.c b/windows/virt2phys/virt2phys_logic.c index e3ff293..175924a 100644 --- a/windows/virt2phys/virt2phys_logic.c +++ b/windows/virt2phys/virt2phys_logic.c @@ -12,13 +12,13 @@ struct virt2phys_process { HANDLE id; LIST_ENTRY next; - SINGLE_LIST_ENTRY blocks; + LIST_ENTRY blocks; ULONG64 memory; }; struct virt2phys_block { PMDL mdl; - SINGLE_LIST_ENTRY next; + LIST_ENTRY next; }; static struct virt2phys_params g_params; @@ -69,24 +69,28 @@ virt2phys_process_create(HANDLE process_id) struct virt2phys_process *process; process = ExAllocatePoolZero(NonPagedPool, sizeof(*process), 'pp2v'); - if (process != NULL) + if (process != NULL) { process->id = process_id; + InitializeListHead(&process->blocks); + } + return process; } static void virt2phys_process_free(struct virt2phys_process *process, BOOLEAN unmap) { - PSINGLE_LIST_ENTRY node; + PLIST_ENTRY node, next; struct virt2phys_block *block; TraceInfo("ID = %p, unmap = %!bool!", process->id, unmap); - node = process->blocks.Next; - while (node != NULL) { + for (node = process->blocks.Flink; node != &process->blocks; node = next) { + next = node->Flink; block = CONTAINING_RECORD(node, struct virt2phys_block, next); - node = node->Next; - virt2phys_block_free(block, unmap); + RemoveEntryList(&block->next); + + virt2phys_block_free(block, TRUE); } ExFreePool(process); @@ -109,10 +113,10 @@ virt2phys_process_find(HANDLE process_id) static struct virt2phys_block * virt2phys_process_find_block(struct virt2phys_process *process, PVOID virt) { - PSINGLE_LIST_ENTRY node; + PLIST_ENTRY node; struct virt2phys_block *cur; - for (node = process->blocks.Next; node != NULL; node = node->Next) { + for (node = process->blocks.Flink; node != &process->blocks; node = node->Flink) { cur = CONTAINING_RECORD(node, struct virt2phys_block, next); if (cur->mdl->StartVa == virt) return cur; @@ -182,7 +186,7 @@ virt2phys_process_cleanup(HANDLE process_id) } static struct virt2phys_block * -virt2phys_find_block(HANDLE process_id, void *virt, +virt2phys_find_block(HANDLE process_id, PVOID virt, struct virt2phys_process **process) { PLIST_ENTRY node; @@ -244,13 +248,13 @@ virt2phys_add_block(struct virt2phys_process *process, return STATUS_QUOTA_EXCEEDED; } - PushEntryList(&process->blocks, &block->next); + InsertHeadList(&process->blocks, &block->next); process->memory += size; return STATUS_SUCCESS; } static NTSTATUS -virt2phys_query_memory(void *virt, void **base, size_t *size) +virt2phys_query_memory(PVOID virt, PVOID *base, size_t *size) { MEMORY_BASIC_INFORMATION info; SIZE_T info_size; @@ -321,7 +325,7 @@ virt2phys_check_memory(PMDL mdl) } static NTSTATUS -virt2phys_lock_memory(void *virt, size_t size, PMDL *mdl) +virt2phys_lock_memory(PVOID virt, size_t size, PMDL *mdl) { *mdl = IoAllocateMdl(virt, (ULONG)size, FALSE, FALSE, NULL); if (*mdl == NULL) @@ -346,12 +350,35 @@ virt2phys_unlock_memory(PMDL mdl) IoFreeMdl(mdl); } +static BOOLEAN +virt2phys_is_valid_block(struct virt2phys_block *block, PVOID virt) +{ + /* + * Check if MDL in block stores the valid physical address. + * The virtual to physical memory mapping may be changed when the + * virtual memory region is freed by the user process and malloc again, + * then we need to remove the block and create a new one. + */ + PHYSICAL_ADDRESS block_phys, real_phys; + + block_phys = virt2phys_block_translate(block, virt); + real_phys = MmGetPhysicalAddress(virt); + + if (block_phys.QuadPart == real_phys.QuadPart) + return TRUE; + + TraceWarning("VA = %p, invalid block physical address %llx, valid address %llx", + virt, block_phys.QuadPart, real_phys.QuadPart); + + return FALSE; +} + NTSTATUS virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys) { PMDL mdl; HANDLE process_id; - void *base; + PVOID base; size_t size; struct virt2phys_process *process; struct virt2phys_block *block; @@ -371,8 +398,17 @@ virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys) /* Don't lock the same memory twice. */ if (block != NULL) { - *phys = virt2phys_block_translate(block, virt); - return STATUS_SUCCESS; + if (virt2phys_is_valid_block(block, base)) { + *phys = virt2phys_block_translate(block, virt); + return STATUS_SUCCESS; + } + /* Remove the invalid block. */ + KeAcquireSpinLock(g_lock, &irql); + RemoveEntryList(&block->next); + process->memory -= MmGetMdlByteCount(block->mdl); + KeReleaseSpinLock(g_lock, irql); + + virt2phys_block_free(block, TRUE); } status = virt2phys_lock_memory(base, size, &mdl);