RE: [PATCH v3] windows/virt2phys: fix block MDL not updated

["Li, Ming3" <ming3.li@intel.com>]

Hello,

Any update on the patch review?

Best regards,
Ming

> -----Original Message-----
> From: Li, Ming3 <ming3.li@intel.com>
> Sent: Tuesday, September 12, 2023 7:18 PM
> To: Li, Ming3 <ming3.li@intel.com>
> Cc: dev@dpdk.org; dmitry.kozliuk@gmail.com; roretzla@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.li@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