linux-kernel - Re: [PATCH rc 1/4] iommufd/selftest: Fix iommufd_dirty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFDMoMX8eL7azoUL@nvidia.com>
Date: Mon, 16 Jun 2025 19:02:08 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <kevin.tian@...el.com>, <shuah@...nel.org>, <joao.m.martins@...cle.com>,
	<steven.sistare@...cle.com>, <iommu@...ts.linux.dev>,
	<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<thomas.weissschuh@...utronix.de>
Subject: Re: [PATCH rc 1/4] iommufd/selftest: Fix iommufd_dirty_tracking with
 large hugepage sizes

On Mon, Jun 16, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote:
> On Sun, Jun 15, 2025 at 10:02:03PM -0700, Nicolin Chen wrote:
> >  FIXTURE_TEARDOWN(iommufd_dirty_tracking)
> >  {
> > -	munmap(self->buffer, variant->buffer_size);
> > -	munmap(self->bitmap, DIV_ROUND_UP(self->bitmap_size, BITS_PER_BYTE));
> > +	unsigned long size = variant->buffer_size;
> > +
> > +	if (variant->hugepages)
> > +		size = __ALIGN_KERNEL(variant->buffer_size, HUGEPAGE_SIZE);
> > +	munmap(self->buffer, size);
> > +	free(self->buffer);
> > +	free(self->bitmap);
> >  	teardown_iommufd(self->fd, _metadata);
> 
> munmap followed by free isn't right..

You are right. I re-checked with Copilot. It says the same thing.
I think the whole posix_memalign() + mmap() confuses me..

Yet, should the bitmap pair with free() since it's allocated by a
posix_memalign() call?

> This code is using the glibc allocator to get a bunch of pages mmap'd
> to an aligned location then replacing the pages with MAP_SHARED and
> maybe HAP_HUGETLB versions.

And I studied some use cases from Copilot. It says that, to use
the combination of posix_memalign+mmap, we should do:
	aligned_ptr = posix_memalign(pagesize, pagesize);
	unmap(aligned_ptr, pagesize);
	mapped = mmap(aligned_ptr, pagesize, PROT_READ | PROT_WRITE,
		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
	munmap(mapped, pagesize);
	// No free() after munmap().

---breakdown---
Before `posix_memalign()`:
  [ heap memory unused ]

After `posix_memalign()`:
  [ posix_memalign() memory ]  ← managed by malloc/free
  ↑ aligned_ptr

After `munmap(aligned_ptr)`:
  [ unmapped memory ]          ← allocator no longer owns it

After `mmap(aligned_ptr, ..., MAP_FIXED)`:
  [ anonymous mmap region ]    ← fully remapped, under your control
  ↑ mapped
---end---

It points out that the heap bookkeeping will be silently clobbered
without the munmap() in-between (like we are doing):
---breakdown---
After `posix_memalign()`:
  [ posix_memalign() memory ]  ← malloc thinks it owns this

Then `mmap(aligned_ptr, ..., MAP_FIXED)`:
  [ anonymous mmap region ]    ← malloc still thinks it owns this (!)
  ↑ mapped
---end---

It also gives a simpler solution for a memory that is not huge
page backed but huge page aligned (our !variant->hugepage case):
---code---
void *ptr;
size_t alignment = 2 * 1024 * 1024; // or whatever HUGEPAGE_SIZE was
size_t size = variant->buffer_size;

// Step 1: Use posix_memalign to get an aligned pointer
if (posix_memalign(&ptr, alignment, size) != 0) {
    perror("posix_memalign");
    return -1;
}

// Use the memory directly
self->buffer = ptr;

// Access/manipulate the memory as needed...

// Step 2: Clean up when done
free(self->buffer);
---end---

Also, for a huge page case, there is no need of posix_memalign():
"Hugepages are not part of the standard heap, so allocator functions
 like posix_memalign() or malloc() don't help and can even get in the
 way."

Instead, it suggests a cleaner version without posix_memalign():
---code---
void *addr = mmap(NULL, variant->buffer_size, PROT_READ | PROT_WRITE,
                  MAP_SHARED | MAP_ANONYMOUS | MAP_HUGETLB | MAP_POPULATE,
		  -1, 0);
if (addr == MAP_FAILED) { perror("mmap"); return -1; }
---end---

Should we follow?

Thanks
Nicolin