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 734264257C; Tue, 12 Sep 2023 13:14:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 48DF3402E2; Tue, 12 Sep 2023 13:14:02 +0200 (CEST) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id 9C43D402AC for ; Tue, 12 Sep 2023 13:13:59 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1694517240; x=1726053240; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=kgtwZgAb8cKJNk6Kl7De0kowqsfm3Pb2NFa32MawsE0=; b=PWMnJNYZKV3TqebL6/z6GWS2cu/Kk09wE3dZYm7wx1pwhzEmaowqK/NI GQaIEAscS/LjtxH6ZzDbpz7uasAUY1X4GZENuQXqbhhNRVFMQkYYM4bCB OX7Vx8qAAgIZO7XhyS1J8lMJP4GZ8psw3skHNn1VIKdhJI8K6iir5bxtP LTfsrnoe1OLKw343ZBojBmQ0zLagC2xFITpcQ5Rtwb/giEfE2APNQ7w4u BGURjklfJzUs3KJM2fMBfwoJqJ9RmLDdLYK/EXKXaQEHTSSmyi8FLzveO yyPtY4XSCpcI2ECmJcBWd/xO0FhefooSzSHY8mbHGwsm3BAw/+OYEY2Br Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10830"; a="464713337" X-IronPort-AV: E=Sophos;i="6.02,139,1688454000"; d="scan'208";a="464713337" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Sep 2023 04:13:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10830"; a="990469480" X-IronPort-AV: E=Sophos;i="6.02,139,1688454000"; d="scan'208";a="990469480" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmsmga006.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 12 Sep 2023 04:13:53 -0700 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.32; Tue, 12 Sep 2023 04:13:53 -0700 Received: from orsmsx603.amr.corp.intel.com (10.22.229.16) by ORSMSX611.amr.corp.intel.com (10.22.229.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.32; Tue, 12 Sep 2023 04:13:52 -0700 Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) by orsmsx603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.32 via Frontend Transport; Tue, 12 Sep 2023 04:13:52 -0700 Received: from NAM10-DM6-obe.outbound.protection.outlook.com (104.47.58.106) by edgegateway.intel.com (134.134.137.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.32; Tue, 12 Sep 2023 04:13:52 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IQe9cT9aitbHGW1nFY5MvstZ5dkADQbrVLbj51sHFvF9dy3MJ3P6djx5d0XLnLEXfqQtOumSpXWHiu44XH9+VIbdm/0lCHneY71D/JvkHITscpX3RGkvg/rZxxKZRT0mgu0yen3hQoxsjB4HMnilGFM1kIVVYavNppJNlyxijrmU36Ia1kaWVG6ksekwDbMBFq0Gc6lR41hVR/z1uRMrsB634peMNaMikc7IOzV7/yz+t7sXZkfD7R8woZtgk+rpCYmbEpkSM+iRhZXDCcOmEzmZmUB918fsxnI1N0hKr9YfnA1QiMyrZRCpskixE4nm+HdpATJnt9WyInZq81LB5g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=LXd63k0hErK98LMjRs6pWCmlJwdBztLRPoF6Pq6JU4o=; b=GpL73VfwuZYWGBcIAU84K2P5gnqsMCQP2CDUpPpk8/FxQiRBKx6R7aWVSSyL9duS98JxdZe32E6ztLPY46+BPud4J0weuynBzYI/B3FrgCmn3b/EOcTzrLCcusCxIv9KUZItOu2X/WzhgWcSg/g/3qhLlvyleaGYQzOD4tnssBEqZDHTDlkJtDZrD5Pce3CrpVjUKOLvjunDS0JezdVfxNi4HB2qwO2qBfs/vUVSQgCMHme6dS9jVPq1Sc6HHbdCjkOFysE6Dk/wGKXt6RvIlk5T4GYy0aWpjsUFlVBJmlghAPinf3q0wd6ira8Tma3mDOtNa16jn5dboog2X0pZ3A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Received: from SJ0PR11MB5814.namprd11.prod.outlook.com (2603:10b6:a03:423::11) by IA1PR11MB7198.namprd11.prod.outlook.com (2603:10b6:208:419::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6768.30; Tue, 12 Sep 2023 11:13:50 +0000 Received: from SJ0PR11MB5814.namprd11.prod.outlook.com ([fe80::cc2c:be39:32f:2398]) by SJ0PR11MB5814.namprd11.prod.outlook.com ([fe80::cc2c:be39:32f:2398%7]) with mapi id 15.20.6768.029; Tue, 12 Sep 2023 11:13:50 +0000 From: "Li, Ming3" To: Dmitry Kozlyuk CC: "dev@dpdk.org" , Tyler Retzlaff Subject: RE: [PATCH v2] windows/virt2phys: fix block MDL not updated Thread-Topic: [PATCH v2] windows/virt2phys: fix block MDL not updated Thread-Index: AQHZ5LFBQIpnVcW5LEeti176i93+eLAWKqGAgACm+FA= Date: Tue, 12 Sep 2023 11:13:50 +0000 Message-ID: References: <20230911082229.1479773-1-ming3.li@intel.com> <20230911130936.1485584-1-ming3.li@intel.com> <20230912005041.0e09671c@sovereign> In-Reply-To: <20230912005041.0e09671c@sovereign> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: SJ0PR11MB5814:EE_|IA1PR11MB7198:EE_ x-ms-office365-filtering-correlation-id: 6026f088-88c3-411a-0adf-08dbb3815774 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 4lrKXeDJfPQcl4CdAy/WEqPQ32/T0I3l3kRy+69CRCdUE+d6ipogy1HEgPlBEQWJcm7wZKUuAv7cEO35AYxqtL+pT80eWfEb11AJ+Q7Hs8LkhJoy0I3oMYqi5+OetfM3URlFkqyM59mkdNagbHsoTf4rTbCiF2w6oML239xonBeKO76C+jaaLvf6H93A43ZU56NSRWzvisnIeiRsgMcPqXTBYlOdneOLeej0YVQdOhM1ZGtbMdDuj5T2XaDdIm07dN4NtzjF3orH729krV1uTdAUUhXWZWLhdwJR0qNjREU3gswuGjRCDlIOvwZjlNt0HuKqff+Ok8glxxSv4kZ8gNRqG/Q5rPwjnQpka344sfp5Ez3T5vIqqhjPWDKKjQT5G2GdQXKrq1a+cJ5ZE2M04Af+/kdOE91FLXY7i1tK3X1ptPw+gIDwVqadwToHwpI3DnW4oALLvEimz/sBz01HrcbczKInpRMsC2EdUwJkDzsMRH7UTqNOSJQ7f+8+CebG55CLg6hSVdGckZD67kbI0h4LrQNROI9wgYTiQkZdU8KnhP8SWUqwb2UnSq0C0sJxk+Dc+rOcfdS8JXdSD/PbvRCnI+FJjUs4QZCD1TVMiQvOEmpFoLuFvFhR1//p8VwCgIUilGA15kSW+qofC7SisQ== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SJ0PR11MB5814.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230031)(346002)(136003)(39860400002)(366004)(376002)(396003)(1800799009)(186009)(451199024)(82960400001)(38100700002)(122000001)(38070700005)(55016003)(33656002)(86362001)(478600001)(9686003)(2906002)(966005)(71200400001)(53546011)(6916009)(6506007)(5660300002)(8936002)(52536014)(4326008)(76116006)(66476007)(316002)(54906003)(15650500001)(66556008)(66946007)(7696005)(66446008)(41300700001)(64756008)(83380400001)(8676002)(26005); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?yi2t2XjwF7jGgTovT1246ugE3+oCb44pJnzLA0z+IQwYCJN6aSi5dd0BsScd?= =?us-ascii?Q?YV3wWUTMuBtToIQTmByXERQ5Wf/2Hv5lYOmsFGE6Ja4hCcVVOl3DpqkY0aHg?= =?us-ascii?Q?zJOLXUiKnwShbmqjM/Cg9nCdz0IRYjQERbizQITTG7ClLuKOh7fefXdi/SO0?= =?us-ascii?Q?yBUFJKz4UW1zVejgTske+sMWl1WHEDBTT4WyZ+FpJhge9u9oRZn5aOD6YDpK?= =?us-ascii?Q?gbYBAKWXao5//Z+KEPLlLde9AxSyr7RBQfEMOR0bacRFE2mBTjPczkI60sV3?= =?us-ascii?Q?arPSgCXPjznin8DesrG67zWW2WSMlKHYwdx41kyaun8EZsgSBRCE2djtxEzl?= =?us-ascii?Q?xN/VJEy+g6tV4LawO6CExOi16e+7E0vkeppV5VoDr1WPaBZhWE2u7Wm2C/bl?= =?us-ascii?Q?zf/VY52RKAFv7HHUprl76GOBLoxjHxoeiUc60O/6j05glw+Nxjui4U4eyWxh?= =?us-ascii?Q?4I1pC2D1DIo+XL/qf3miRngf200CG7tumdDjnZsMqBhtnxXzJ4eZfWW0lDHU?= =?us-ascii?Q?cR6i8sQaZnGdZyNz3AQf8bCy/3NnkozDT8mj4qtPhUagC85x1AejLowEH/o4?= =?us-ascii?Q?JPnHy5QyhymESCms0SM8Bd1bLbtUHSO5VIRleP1Y444wEJrPYqUPXczNuY3q?= =?us-ascii?Q?4Hw/ml07Ibg4aNr/wi9HlKGz4v/zCZeCFZNJbLITLFhF0GirivePMu0/nw5B?= =?us-ascii?Q?GDDoXngw9T+x2mTe49Malj2Wv9s8+LESr3xxM+nXiGkVv/6cuLsy3IuOrz90?= =?us-ascii?Q?JuEbeXC3KHdtaPHsxWFEy8p6QfV3wx2qFqm3yrfowymnIBcNcdAP3JT2jW5s?= =?us-ascii?Q?BPb7LulFRq6Ikp+cYA9lTAVj/pUWOKazFM7QNLm+ow2LMjcd/PbL0Uc82sPD?= =?us-ascii?Q?nGVmkyhkyu6a1wW3UVy4f824KpA6j4YLN5Wl5mcQcg5RtuyotWaUBHs+ecy4?= =?us-ascii?Q?KMh5mEcF2SV1MvOc/OFeeu8OVrYk9Nj4XwLEwhdIIZoGnwSEnXkaHld20Cde?= =?us-ascii?Q?pTapwLlTOHCDYgPDZpvXlFbP2BILa9VCqUoMj7T4jEtfbA/VvqH1RzTE7iqA?= =?us-ascii?Q?jKKCHZ7S+1pu0skw844iU+bJIrNyA2XHN7j5xRNYnbo7GF5L5eq0gQeXd5Zx?= =?us-ascii?Q?dScuE6YGXKLnrBMz96BPmDBtL/mOImJZF9uhvQ2FXHIPgYYu9nE+YDqA2nzO?= =?us-ascii?Q?GjuVX/SG+zc72sh/8UfigYpysoiSZnr27YloWHZCqiKZoomdeEDdfKYa6+Av?= =?us-ascii?Q?giaC/QQeulu5QbHKi2TUy+6UFMWc2aWwSps6DSvLGIvP01PJor/dBSYbJNuQ?= =?us-ascii?Q?7Hj7XabUF3+0sWZGtKDTGie3hmOQVEGiuc0DsNU1HJuDUuaKM/fKIvzYI1ty?= =?us-ascii?Q?COKupZNCmndlFUXQfVOP9plv9VNjV0SLa50AaHjota9czL8YATeTHkqs5xJ1?= =?us-ascii?Q?+2/L7SIL5Sl8I6zRFIeX/JwpNhnbxGmYkKQthKAy1DR3aMvhhOwK+guMMxxf?= =?us-ascii?Q?1vLx+b24IvpPsuGPND51obdccPmGIxnfZMN3GRIaxbEfUFZCHsw6FNDKvYgO?= =?us-ascii?Q?c736ayIhfE233QDnY3Y=3D?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5814.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 6026f088-88c3-411a-0adf-08dbb3815774 X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Sep 2023 11:13:50.1907 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ESvpEtUWekreTtrOmklnv0DEk0ttLy6ycf3L/mq4gMrva9K1pVEBqx0wr9HI4whnPagBNpBlMmDxxKVckp4QcA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7198 X-OriginatorOrg: intel.com 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 Dmitry, Thanks for the review, I'll send the next version patch. Please see my comments below. > -----Original Message----- > From: Dmitry Kozlyuk > Sent: Tuesday, September 12, 2023 5:51 AM > To: Li, Ming3 > Cc: dev@dpdk.org; Tyler Retzlaff > Subject: Re: [PATCH v2] windows/virt2phys: fix block MDL not updated >=20 > Hi Ric, >=20 > 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. >=20 > I missed this case completely :( >=20 > Is any if these bugs are related? > If so, please mention "Bugzilla ID: xxxx" in the commit message. >=20 > https://bugs.dpdk.org/show_bug.cgi?id=3D1201 > https://bugs.dpdk.org/show_bug.cgi?id=3D1213 >=20 Sure, will do. I cannot reproduce them in my environment, but from the message, they both mentioned that some pages not unlocked after exit. So they can be= related. For example, in Bug 1201, it only exists on Windows 2019, may it be caused = by the OS limitation so that some memory segment got freed and allocated same virt= ual address again? Maybe someone can use this patch to check if there is 'refresh' behavior fr= om TraceView logs. > > > > 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 >=20 > Typo: "iova" -> "IOVA" here and below. >=20 Noted, will fix in v3. > > 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. >=20 > What if the size is different? > Should it be updated for the refreshed block along with the MDL? >=20 The size of single MDL is always 2MB since it describes a hugepage here.=20 (at least from my observation :)) For allocated buffer larger than 2MB, it = has serval mem segs (related to serval MDLs), most buggy mem segs are those possess a whole hugepage, these segments are freed along with the buffer, so their MDLs become invalid. Since the block is just wrapper for MDL and list entry, the refresh action should be applied to the whole block. > [...] > > +static NTSTATUS > > +virt2phys_block_refresh(struct virt2phys_block *block, PVOID base, > > +size_t size) { > > + NTSTATUS status; > > + PMDL mdl =3D 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 agai= n, > > + * 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 =3D virt2phys_block_translate(block, base); > > + real_phys =3D MmGetPhysicalAddress(base); > > + > > + if (block_phys.QuadPart =3D=3D real_phys.QuadPart) > > + /* No need to refresh block. */ > > + return STATUS_SUCCESS; > > + > > + virt2phys_unlock_memory(mdl); >=20 > After this call the block's MDL is a dangling pointer. > If an error occurs below, the block with a dangling pointer will remain i= n the list > and will probably cause a crash later. > If a block can't be refreshed, it must be freed (it's invalid anyway). >=20 I will change the refresh logic here to just check the PA, and if it doesn'= t match, the block will be removed from process's blocks list(after the check functi= on). To make it easy for block removal, the single linked list will be replaced = with a double linked list. > > + mdl =3D NULL; > > + > > + status =3D virt2phys_lock_memory(base, size, &mdl); > > + if (!NT_SUCCESS(status)) > > + return status; > > + > > + status =3D virt2phys_check_memory(mdl); > > + if (!NT_SUCCESS(status)) { > > + virt2phys_unlock_memory(mdl); > > + return status; > > + } > > + block->mdl =3D mdl; > > + > > + TraceInfo("Block refreshed from %llx to %llx", block_phys.QuadPart, > > +real_phys.QuadPart); >=20 > 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? >=20 > ID=3D... VA=3D... size=3D... requires physical address refresh > ID=3D... VA=3D... size=3D... physical address refreshed from ... to ... >=20 > > + > > + 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 !=3D NULL) { > > + KeAcquireSpinLock(g_lock, &irql); > > + status =3D virt2phys_block_refresh(block, base, size); > > + KeReleaseSpinLock(g_lock, irql); >=20 > Is it safe to do all the external calls holding this spinlock? > I can't confirm from the doc that ZwQueryVirtualMemory(), for example, do= es > not access pageable data. > And virt2phys_lock_memory() raises exceptions, although it handles them. > Other stuff seems safe. >=20 > The rest of the code only takes the lock to access block and process list= s, 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. >=20 > The translation IOCTL performance is not critical, so maybe it is worth r= eplacing > the spinlock with just a global mutex, WDYT? In the upcoming v3 patch, the lock will be used for block removal which won= 't fail. I'm relatively new to Windows driver development. From my perspective, the = use of a spinlock seems appropriate in this driver. Maybe a read-write lock can= be more effective here?