From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8FE0642573; Mon, 11 Sep 2023 23:50:46 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 127A64027C; Mon, 11 Sep 2023 23:50:46 +0200 (CEST) Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by mails.dpdk.org (Postfix) with ESMTP id 9576840270 for ; Mon, 11 Sep 2023 23:50:44 +0200 (CEST) Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-50078e52537so8316425e87.1 for ; Mon, 11 Sep 2023 14:50:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694469044; x=1695073844; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=OY7PGWflQ6R4nT3l+MpqvTapbSWvkPDqy2cJXRbog2M=; b=TiAuTiW/uMoZAAGnp/oiA5Vm4pL8ns2QdRbEJ2AhVquhpkaY8Qw0MvXCKGbDchD6OG OYgD5nt9jHZTLTQhwues4bKAqwJ6Av+W8Fx1HLOswM2lkU18BTjB1z1nSS5VBH8ffzK2 3F+ZIXPhMvoGIu7uiPkiDScbbklmKRYU8w4ZrniI6tRxKw8908usMcoprcStkfjfaJ9s phq/QYesoyY3WoX7uSeeZab+dksi3DGD5l64ocLliDRDKOYRRy0/fIse6aOVf+EMDCjD WTAMOqmVNL4/lbokBCL9hxA9SaykjD7FxUMN17UkeLX2uYuMDfzZSIKgeOFH/HbfM+cm 0n+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694469044; x=1695073844; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OY7PGWflQ6R4nT3l+MpqvTapbSWvkPDqy2cJXRbog2M=; b=QFZSDlLKkOSzNHgwIZ04a6v2a0tSBLLtw+BTeTU6/exyCmUHAzGWwL0SbqPfsUJ1nK sob4XrY+4wu4zGvk9imfswUGBp7N80lCpDvMzzEqUMvZxNv+SIW4OQTzbgn/r/QcqOtD p5DngRhHz508jxj8AEOr80Tky/MK8+AVplFoQCYUS68yZcwH5HM8l+GogKvtUSKb016/ poY9bDTDaaunuHRnR4Ji23YehSXWgeTS7Yhm1wUEHfgfA1MEbrsZQbvPDPOmd1YQ9XUv VMzlbahSOmSEkYkYqKT2N78BzlN3qdm04s8JfF/wHlkOlZNFMww/FS1q6tMJ2odUQGjN 1I1w== X-Gm-Message-State: AOJu0YzxXDRNBTZ7cHOOW9GzCRB52H+pozWE4/ubrsMg+AM4hhLkaTZg TOd+5EKcf9qU/3ZVyEXgo0xCsFpD81E= X-Google-Smtp-Source: AGHT+IEikyFXBcMyzgqUNBD+GrJ7r02z9/3LztM9ObEMrdYgchoaJUXWmfop/V0+PKuqLUdwi/xaLA== X-Received: by 2002:a05:6512:4007:b0:500:ac10:1641 with SMTP id br7-20020a056512400700b00500ac101641mr10493777lfb.46.1694469043594; Mon, 11 Sep 2023 14:50:43 -0700 (PDT) Received: from sovereign (broadband-109-173-110-33.ip.moscow.rt.ru. [109.173.110.33]) by smtp.gmail.com with ESMTPSA id l21-20020a19c215000000b00501bd5ebd15sm1478398lfc.61.2023.09.11.14.50.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 11 Sep 2023 14:50:42 -0700 (PDT) Date: Tue, 12 Sep 2023 00:50:41 +0300 From: Dmitry Kozlyuk To: Ric Li Cc: dev@dpdk.org, Tyler Retzlaff Subject: Re: [PATCH v2] windows/virt2phys: fix block MDL not updated Message-ID: <20230912005041.0e09671c@sovereign> In-Reply-To: <20230911130936.1485584-1-ming3.li@intel.com> References: <20230911082229.1479773-1-ming3.li@intel.com> <20230911130936.1485584-1-ming3.li@intel.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 Hi Ric, 2023-09-11 21:09 (UTC+0800), Ric Li: > 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. I missed this case completely :( Is any if these bugs are related? If so, please mention "Bugzilla ID: xxxx" in the commit message. https://bugs.dpdk.org/show_bug.cgi?id=1201 https://bugs.dpdk.org/show_bug.cgi?id=1213 > > 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 Typo: "iova" -> "IOVA" here and below. > 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. It then needed to be deleted from the driver's > block MDL info. 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 of the same size. > > To address this, a refresh function 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 MDL within the block is released and rebuilt > to store the correct mapping. What if the size is different? Should it be updated for the refreshed block along with the MDL? [...] > +static NTSTATUS > +virt2phys_block_refresh(struct virt2phys_block *block, PVOID base, size_t size) > +{ > + NTSTATUS status; > + PMDL mdl = block->mdl; > + > + /* > + * Check if we need to refresh MDL in block. > + * 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 unlock the physical memory and lock again to > + * refresh the MDL information stored in block. > + */ > + PHYSICAL_ADDRESS block_phys, real_phys; > + > + block_phys = virt2phys_block_translate(block, base); > + real_phys = MmGetPhysicalAddress(base); > + > + if (block_phys.QuadPart == real_phys.QuadPart) > + /* No need to refresh block. */ > + return STATUS_SUCCESS; > + > + virt2phys_unlock_memory(mdl); After this call the block's MDL is a dangling pointer. If an error occurs below, the block with a dangling pointer will remain in the list and will probably cause a crash later. If a block can't be refreshed, it must be freed (it's invalid anyway). > + mdl = NULL; > + > + status = virt2phys_lock_memory(base, size, &mdl); > + if (!NT_SUCCESS(status)) > + return status; > + > + status = virt2phys_check_memory(mdl); > + if (!NT_SUCCESS(status)) { > + virt2phys_unlock_memory(mdl); > + return status; > + } > + block->mdl = mdl; > + > + TraceInfo("Block refreshed from %llx to %llx", block_phys.QuadPart, real_phys.QuadPart); Please add process ID, block VA, and block size. If refreshing fails, there is not way to tell what happened and why. What do you think about logging like this? ID=... VA=... size=... requires physical address refresh ID=... VA=... size=... physical address refreshed from ... to ... > + > + return STATUS_SUCCESS; > +} > + > 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,6 +412,11 @@ virt2phys_translate(PVOID virt, PHYSICAL_ADDRESS *phys) > > /* Don't lock the same memory twice. */ > if (block != NULL) { > + KeAcquireSpinLock(g_lock, &irql); > + status = virt2phys_block_refresh(block, base, size); > + KeReleaseSpinLock(g_lock, irql); Is it safe to do all the external calls holding this spinlock? I can't confirm from the doc that ZwQueryVirtualMemory(), for example, does not access pageable data. And virt2phys_lock_memory() raises exceptions, although it handles them. Other stuff seems safe. The rest of the code only takes the lock to access block and process lists, which are allocated from the non-paged pool. Now that I think of it, this may be insufficient because the code and the static variables are not marked as non-paged. The translation IOCTL performance is not critical, so maybe it is worth replacing the spinlock with just a global mutex, WDYT?