Received-SPF: pass (google.com: domain of linux-kernel+bounces-79181-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Michael Kelley <mhklinux@outlook.com>
To: Nicolin Chen <nicolinc@nvidia.com>, Will Deacon <will@kernel.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kernel-team@android.com" <kernel-team@android.com>, "iommu@lists.linux.dev"
	<iommu@lists.linux.dev>, Christoph Hellwig <hch@lst.de>, Marek Szyprowski
	<m.szyprowski@samsung.com>, Robin Murphy <robin.murphy@arm.com>, Petr Tesarik
	<petr.tesarik1@huawei-partners.com>, Dexuan Cui <decui@microsoft.com>
Subject: RE: [PATCH v4 5/5] iommu/dma: Force swiotlb_max_mapping_size on an
 untrusted device
Thread-Topic: [PATCH v4 5/5] iommu/dma: Force swiotlb_max_mapping_size on an
 untrusted device
Thread-Index: AQHaZLoRyKgMK9jLMUWPn0GpqcfvobEVbBTggALvr4CAAAjwsA==
Date: Fri, 23 Feb 2024 21:10:32 +0000
Message-ID:
 <SN6PR02MB41574C0F51BEF3AC474C2EC1D4552@SN6PR02MB4157.namprd02.prod.outlook.com>
References: <20240221113504.7161-1-will@kernel.org>
 <20240221113504.7161-6-will@kernel.org>
 <SN6PR02MB4157828120FB7D3408CEC991D4572@SN6PR02MB4157.namprd02.prod.outlook.com>
 <Zdj4y7nqF2LHq2IH@Asurada-Nvidia>
In-Reply-To: <Zdj4y7nqF2LHq2IH@Asurada-Nvidia>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?CaWyl/XV6cBTbWB4es0rYT3xWPNv3tINhp4+W9RCGc9PWNSZwIsRqsgdTpaS?=
 =?us-ascii?Q?9AGObyqfC4bSVacC3SLPPPMk+PTuO5GV1Tpvbewc0+ajy5iGmlmhKFyHQTE4?=
 =?us-ascii?Q?fUV3vHQNKu74LGUBNaORUzi5iSvk+HGiecMVUO0nS99H18cdnigMn+SW0fko?=
 =?us-ascii?Q?yGclD13/gtXEEdliBgSnCq2NdKR5jMj7rR413WZv2qcK8wBocooK3V9fpNoI?=
 =?us-ascii?Q?2xDR73Sgy48cVqIoq/ZizfO+LjMQjp0D5WdsOkoQyo47xHZaBM9didIxe7qo?=
 =?us-ascii?Q?ENMr2BRV/AYsUJHqK4muTHI2bsenM/yCgmy28jLo+FGh+/ara6hx8eYtUlNu?=
 =?us-ascii?Q?7h3NBwYlryqDJl0qB/mrWUMmgcUxMLIv+8zPtJkk6hMCexnt/xXTudfsRa04?=
 =?us-ascii?Q?s03+3hxuaPuCJPGxYotfXYqhzSpwlfKXiep8vLgccR6VcVZetXcnir0Wwpma?=
 =?us-ascii?Q?yZV4U5A/Erpg/WBXi655xFXBMe/U+6X6yZHAe1OAVaA1Fsa+Nf/KYaSoaeKJ?=
 =?us-ascii?Q?qztfU3Fn7m75+6MYF3jDm9cXWVC1t5PYG1NgCizaTjXPCBDnoPrs98MZYElC?=
 =?us-ascii?Q?gNnsTjgabPqUNz5SQCVw0bbl3RtkThnUFvIjBSUJY0WdDgKNgrElNMVzJx3D?=
 =?us-ascii?Q?+Al7IfINUgpUfjHzX42Jt+G8O0pRnpRDi+yhNy3KfmMswRbQo6XE8/XwOI+1?=
 =?us-ascii?Q?rOH5UOaiClmbrju78Y4ZWy44YpEosPQZTg1g8LESBTnL2T6mqdBpPbLA5O6x?=
 =?us-ascii?Q?Bsbr2ALgMal7xbfu4G2MInrQf1FuyvmdWcxS/7DVjhDoG7ZPHOJ4GrgxoSg0?=
 =?us-ascii?Q?duWNML5hLXgjqVZQt48nVsf7elN1Tk9Jd09fo7EJMYQ3XUWk1eGXkxouF6m+?=
 =?us-ascii?Q?YAStq3xrLcStkT8YTQMFzh3LOMQz9J116VXPzuLL2qutGs3D9SeIzLiVE7zH?=
 =?us-ascii?Q?agVKVASaWqTV5SpiqwWCbXbK2CH5XLTjPLPHTmui1mkl5wFDZgbaDyjDXGNO?=
 =?us-ascii?Q?h9a6Zzxrbm2+spkU5EzCYMHZsn5JBKPwBMoDMhzi2CYuoZaFrimfRJnwKQeS?=
 =?us-ascii?Q?T8uWyPfYI4oESTxg/aMxM2MzfHUgr8XRYYjF1QExAQ3vLexAE2IMbgeC09vf?=
 =?us-ascii?Q?WA2OAHRuWeX4mJ66DM0FECxQQb8TZ0ML5E1eg0uPitlj5VXDrztZAIBC0dtQ?=
 =?us-ascii?Q?c4oIZf3jV3xutQpuzy2NQThlA2xv2NclZLgQ1w=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4157.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-Network-Message-Id: 4924402e-63b7-43a5-a1c0-08dc34b3df5a
X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Feb 2024 21:10:33.0319
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR02MB8394

From: Nicolin Chen <nicolinc@nvidia.com> Sent: Friday, February 23, 2024 11=
:58 AM
>=20
> On Wed, Feb 21, 2024 at 11:39:29PM +0000, Michael Kelley wrote:
> > From: Will Deacon <will@kernel.org> Sent: Wednesday, February 21, 2024
> 3:35 AM
> > > +static size_t iommu_dma_max_mapping_size(struct device *dev)
> > > +{
> > > +     if (is_swiotlb_active(dev) && dev_is_untrusted(dev))
> > > +             return swiotlb_max_mapping_size(dev);
> > > +     return SIZE_MAX;
> > > +}
> > > +
> >
> > In this [1] email, Nicolin had a version of this function that incorpor=
ated
> > the IOMMU granule.  For granules bigger than 4K, I think that's needed
> > so that when IOMMU code sets the swiotlb alloc_align_mask to the
> > IOMMU granule - 1, the larger offset plus the size won't exceed the
> > max number of slots.  swiotlb_max_mapping_size() by itself may
> > return a value that's too big when alloc_align_mask is used.
> >
> > Michael
> >
> > [1] https://lore.kernel.org/linux-iommu/SN6PR02MB415727E61B5295C259CCB2=
68D4512@SN6PR02MB4157.namprd02.prod.outlook.com/T/#m14dd359c5a4dd13e6cb0c35=
cf94f8d746257ae48
>=20
> Yea, I just confirmed that with 64KB PAGE_SIZE the alloc_size
> can be over swiotlb_max_mapping_size, i.e. 256KB > 252KB. Yet,
> the max size corresponding to the max number of slots should
> be 256KB. So, I feel that this is marginally safe?

Yes, in the specific case you tested.  But I don't think the general
case is safe.  In your specific case, the "size" argument to
iommu_dma_map_page() is presumably 252 Kbytes because that's
what your new iommu_dma_max_mapping_size() returns.=20
iommu_dma_map_page() calls iova_align() to round up the 252K
to 256K.  Also in your specific case, the "offset" argument to=20
iommu_dma_map_page() is 0, so the "phys" variable (which embeds
the offset) passed to swiotlb_tbl_map_single() is aligned on a
64 Kbyte page boundary.   swiotlb_tbl_map_single() then
computes an offset in the orig_addr (i.e., "phys") based on the
DMA min_align_mask (4 Kbytes), and that value is 0 in your specific
case.  swiotlb_tbl_map_single() then calls swiotlb_find_slots()
specifying a size that is 256K bytes plus an offset of 0, so everything
works.

But what if the original offset passed to iommu_dma_map_page()
is 3 Kbytes (for example)?   swiotlb_tbl_map_single() will have an
orig_addr that is offset from a page boundary by 3 Kbytes.  The
value passed to swiotlb_find_slots() will be 256 Kbytes plus an
offset of 3 Kbytes, which is too big.

>=20
> With that being said, there seems to be a 4KB size waste, due
> to aligning with the iommu_domain granule, in this particular
> alloc_size=3D256KB case?
>=20
> On the other hand, if swiotlb_max_mapping_size was subtracted
> by 64KB (granule=3D64KB), i.e. alloc_size=3D192KB, which seems to
> generate more swiotlb fragments?

dma_max_mapping_size() returns a fixed value for a particular
device, so the fixed value must be conversative enough to handle
dma_map_page() calls with a non-zero offset (or the similar
dma_map_sg() where the scatter/gather list has non-zero
offsets).  So yes, the higher layers must limit I/O size, which
can produce more separate I/Os and swiotlb fragments to
fulfill an original request that is 256 Kbytes or larger.  The
same thing happens with 4K page size in that a 256K I/O
gets broken into a 252K I/O and a 4K I/O if the swiotlb is
being used.

I'm trying to think through if there's a way to make things
work with iommu_max_mapping_size() being less
conversative than subtracting the full granule size.  I'm
doubtful that there is.

Michael