Hello,
Any update on the patch review?
Best regards,
Ming
> -----Original Message-----
> From: Li, Ming3 <ming3...@intel.com>
> Sent: Tuesday, September 12, 2023 7:18 PM
> To: Li, Ming3 <ming3...@intel.com>
> Cc: dev@dpdk.org; dmitry.kozl...@gmail.com; roret...@linux.microsoft.com
> Subject: [PATCH v3] windows/virt2phys: fix block MDL not updated
>
> 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
> chenged to a double linked list.
>
> Also fix the printing of PVOID type.
>
> Bugzilla ID: 1201
> Bugzilla ID: 1213
>
> Signed-off-by: Ric Li <ming3...@intel.com>
> ---
> v3:
> * Change refresh action to block removal
> * Change block list to double linked list
>
> v2:
> * Revert wrong usage of MmGetMdlStartVa
>
> 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..531f08c 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 base) {
> + /*
> + * 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, base);
> + real_phys = MmGetPhysicalAddress(base);
> +
> + if (block_phys.QuadPart == real_phys.QuadPart)
> + return TRUE;
> +
> + TraceWarning("VA = %p, invalid block physical address %llx, valid
> address %llx",
> + base, 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);
> --
> 2.40.1.windows.1
