Re: [PATCH v2 1/4] drivers/iommu: Add page_ext for IOMMU_DEBUG_PAGEALLOC

[Mostafa Saleh <smostafa@google.com>]

On Thu, Nov 13, 2025 at 10:05:19AM +0000, Will Deacon wrote:
> Hi Mostafa,
> 
> On Thu, Nov 06, 2025 at 04:39:50PM +0000, Mostafa Saleh wrote:
> > Add a new config IOMMU_DEBUG_PAGEALLOC, which registers new data to
> > page_ext.
> > This config will be used by the IOMMU API to track pages mapped in
> > the IOMMU to catch drivers trying to free kernel memory that they
> > still map in their domains, causing all types of memory corruption.
> > This behaviour is disabled by default and can be enabled using
> > kernel cmdline iommu.debug_pagealloc.
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > Tested-by: Qinxin Xia <xiaqinxin@huawei.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  6 ++++
> >  drivers/iommu/Kconfig                         | 15 +++++++++
> >  drivers/iommu/Makefile                        |  1 +
> >  drivers/iommu/iommu-debug-pagealloc.c         | 32 +++++++++++++++++++
> >  include/linux/iommu-debug-pagealloc.h         | 17 ++++++++++
> >  mm/page_ext.c                                 |  4 +++
> >  6 files changed, 75 insertions(+)
> >  create mode 100644 drivers/iommu/iommu-debug-pagealloc.c
> >  create mode 100644 include/linux/iommu-debug-pagealloc.h
> 
> This looks like a pretty handy feature to me, but I have some nits below.

Thanks for taking the time to review the patches!

> 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 6c42061ca20e..9a1c4ac8ba96 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2557,6 +2557,12 @@
> >  			1 - Bypass the IOMMU for DMA.
> >  			unset - Use value of CONFIG_IOMMU_DEFAULT_PASSTHROUGH.
> >  
> > +	iommu.debug_pagealloc=
> > +			[KNL,EARLY] When CONFIG_IOMMU_DEBUG_PAGEALLOC is set, this
> > +			parameter enables the feature at boot time. By default, it
> > +			is disabled and the system will work mostly the same as a
> > +			kernel built without CONFIG_IOMMU_DEBUG_PAGEALLOC.
> 
> Can you be more specific about "mostly the same"?

The only difference is that the static key to gate the calls, I was not sure if
saying “exactly the same” is correct, but I think it’s better avoid “mostly” as
it might be confusing and as the data in the cover letter shows no overhead,
I will re-write the whole help anyway.

> 
> > +
> >  	io7=		[HW] IO7 for Marvel-based Alpha systems
> >  			See comment before marvel_specify_io7 in
> >  			arch/alpha/kernel/core_marvel.c.
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 70d29b14d851..6b5e9a2d936a 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -383,4 +383,19 @@ config SPRD_IOMMU
> >  
> >  	  Say Y here if you want to use the multimedia devices listed above.
> >  
> > +config IOMMU_DEBUG_PAGEALLOC
> > +	bool "Debug page memory allocations against IOMMU"
> 
> Perhaps "IOMMU mappings" would make this a little clearer?

Will do.

> 
> > +	depends on DEBUG_PAGEALLOC && IOMMU_API && PAGE_EXTENSION
> > +	help
> > +	  This config checks that a page is freed(unmapped) or mapped by the
> > +	  kernel is not mapped in any IOMMU domain.
> 
> I can't really parse this sentence :/

I will re-write it.

> 
> > It can help with debugging
> > +	  use-after-free or out-of-bound maps from drivers doing DMA through
> > +	  the IOMMU API.
> > +	  This santaizer can have false-negative cases where some problems
> > +	  won't be detected.
> 
> Maybe just say "The sanitizer is best-effort and can fail to detect problems
> in the case that ...".

Makes sense, will do.

> 
> > +	  Expect overhead when enabling this and enabling the kernel command
> > +	  line iommu.debug_pagealloc.
> 
> I'd reword this to say something like "Due to the overhead of the sanitiser,
> iommu.debug_pagealloc must also be passed on the kernel command-line to
> enable this feature".

Will do.

> 
> > +
> > +	  If unsure, say N here.
> > +
> >  endif # IOMMU_SUPPORT
> > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> > index 355294fa9033..8f5130b6a671 100644
> > --- a/drivers/iommu/Makefile
> > +++ b/drivers/iommu/Makefile
> > @@ -34,3 +34,4 @@ obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
> >  obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
> >  obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
> >  obj-$(CONFIG_APPLE_DART) += apple-dart.o
> > +obj-$(CONFIG_IOMMU_DEBUG_PAGEALLOC) += iommu-debug-pagealloc.o
> > diff --git a/drivers/iommu/iommu-debug-pagealloc.c b/drivers/iommu/iommu-debug-pagealloc.c
> > new file mode 100644
> > index 000000000000..385c8bfae02b
> > --- /dev/null
> > +++ b/drivers/iommu/iommu-debug-pagealloc.c
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2025 - Google Inc
> > + * Author: Mostafa Saleh <smostafa@google.com>
> > + * IOMMU API debug page alloc sanitizer
> > + */
> > +#include <linux/atomic.h>
> > +#include <linux/iommu-debug-pagealloc.h>
> > +#include <linux/kernel.h>
> > +#include <linux/page_ext.h>
> > +
> > +static bool needed;
> > +
> > +struct iommu_debug_metadate {
> > +	atomic_t ref;
> > +};
> 
> s/metadate/metadata/

Ah, that's embarrassing, I will fix it.

Thanks,
Mostafa

> 
> Will