[PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[Konstantin Khorenko]

copy_huge_pmd() reads the source PMD without the page table lock and,
for a special PMD, installs that value in the child without reading it
again under the lock. The comment says the PMD cannot change while the
mmap lock is held for write, but unmap_mapping_range() does not take
the mmap lock: it zaps under i_mmap_rwsem and the PMD lock.

dup_mmap() links the child VMA into the file's i_mmap tree right after
the parent's VMA before copying the page tables, so a zap visits the
parent first and the child second. When the zap clears the parent PMD
after fork has read it, and passes the child before fork installs the
copy, the child keeps a mapping the zap was meant to remove.

Drivers that map special PMDs with vmf_insert_pfn_pmd() rely on that
zap. vfio-pci zaps its BARs when the device memory is disabled or the
device is reset, so that userspace gets SIGBUS instead of reaching
disabled MMIO; a stale child mapping still reaches it. drm_gem_shmem
zaps the mappings of a purged object and then truncates its shmem
pages; a stale child mapping points to the freed pages.

Read the PMD again under the lock. The zap clears the parent PMD under
the same lock, so either the recheck finds it empty, or the copy is in
place before the zap reaches the child, which then removes it. If the
PMD is no longer a present special entry, return -EAGAIN: the source
mmap and VMA write locks keep faults out, so only a zap can have
cleared it, and copy_pmd_range() then skips the empty PMD. Otherwise
copy the value read under the lock. pmd_same() would be the wrong
test: threads of the parent can still set the accessed and dirty bits,
and -EAGAIN on a present huge PMD makes copy_pmd_range() clear it as a
bad PMD.

Fixes: bc02afbd4d73 ("mm/fork: accept huge pfnmap entries")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
The window is short: the zap has to clear the parent's PMD and pass
the child's VMA between fork's lockless read and its taking the page
table locks. I have not hit it without help, but a delay in that
window makes it reproducible with vfio-pci, and the patch closes it.

Setup: v7.3-rc7, KVM guest with 4 vCPUs and no IOMMU, the guest's QXL
device bound to vfio-pci in no-IOMMU mode, BAR0 of 64 MB mapped with
PMDs. The reproducer below forks while a second thread clears
PCI_COMMAND_MEMORY through the vfio config region after a random
delay; the child waits for the write to finish and reads one byte of
each 2 MB of the BAR. A child that reads anything without SIGBUS kept
a stale mapping.

  kernel                  delay in window  forks   stale children
  7.3-rc7                 none             20900   0
  7.3-rc7                 2 ms             150     128
  7.3-rc7 + this patch    none             10600   0
  7.3-rc7 + this patch    2 ms             400     0

The delay is an mdelay(2) right after pmdp_get_lockless() in the
special-PMD branch of copy_huge_pmd(), switched on by a test-only
debugfs knob. The rows without it ran on the same test kernels with
the knob at 0, except 5000 patched forks that ran on this patch alone.
The revoke started after a random delay of up to 0.2, 1 or 3 ms
(max_delay_us 200, 1000, 3000). A counter in the test-only hunk
confirmed that the forks copied the BAR as PMDs, and in the last 200
patched forks with the delay the recheck returned -EAGAIN 183 times.

panthor and mshv_vtl map special PMDs with vmf_insert_pfn_pmd() as
well.

copy_huge_pud() reads the source entry under the lock already, and
the PTE level does too, so only the PMD path needs the recheck.

Reproducer. It writes the BAR as a control and reads it with the
memory disabled, which is harmless for QXL VRAM in a guest; do not
point it at a BAR that holds device registers. Build with
"gcc -O2 -pthread -o fork_revoke fork_revoke.c", run as root with the
device bound to vfio-pci; arguments: BDF, iterations, BAR index, max
revoke delay in microseconds, e.g. "./fork_revoke 0000:00:02.0 100 0
3000".

    // SPDX-License-Identifier: GPL-2.0
    /*
     * Fork while vfio-pci revokes a BAR mapped with huge PMDs.
     *
     * The parent maps a BAR of a vfio-pci device and faults it in. A second
     * thread disables the device memory through the command register, which
     * makes vfio-pci zap all BAR mappings, while the main thread forks. Once
     * the memory is disabled, every access to the BAR must raise SIGBUS. A
     * child that can still read a BAR chunk kept a mapping the zap removed.
     *
     * usage: fork_revoke <domain:bus:dev.fn> [iterations [bar [max_delay_us]]]
     *
     * The revoke starts after a random delay of up to max_delay_us, so that it
     * lands at different points of fork.
     */
    #define _GNU_SOURCE
    #include <errno.h>
    #include <fcntl.h>
    #include <limits.h>
    #include <pthread.h>
    #include <sched.h>
    #include <setjmp.h>
    #include <signal.h>
    #include <stdint.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <unistd.h>
    #include <sys/ioctl.h>
    #include <sys/mman.h>
    #include <sys/wait.h>
    #include <linux/pci_regs.h>
    #include <linux/vfio.h>

    #define SZ_2M	(2UL << 20)

    static int dev;
    static off_t cfg_off;
    static int go_pipe[2], done_pipe[2];
    static volatile uint32_t *shared;	/* [0]: iteration whose revoke finished */
    static unsigned long iter;
    static unsigned int max_delay_us;
    static sigjmp_buf jb;

    static void die(const char *what)
    {
    	fprintf(stderr, "%s: %s\n", what, strerror(errno));
    	exit(2);
    }

    static int device_fd(const char *bdf)
    {
    	char path[PATH_MAX], link[PATH_MAX], *group_id;
    	int container, group, noiommu;
    	ssize_t len;

    	snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/iommu_group", bdf);
    	len = readlink(path, link, sizeof(link) - 1);
    	if (len < 0)
    		die("iommu_group");
    	link[len] = 0;
    	group_id = strrchr(link, '/') + 1;
    	snprintf(path, sizeof(path), "/dev/vfio/%s", group_id);
    	noiommu = access(path, F_OK);
    	if (noiommu)
    		snprintf(path, sizeof(path), "/dev/vfio/noiommu-%s", group_id);
    	container = open("/dev/vfio/vfio", O_RDWR);
    	group = open(path, O_RDWR);
    	if (container < 0 || group < 0)
    		die(path);
    	if (ioctl(group, VFIO_GROUP_SET_CONTAINER, &container) ||
    	    ioctl(container, VFIO_SET_IOMMU,
    		  noiommu ? VFIO_NOIOMMU_IOMMU : VFIO_TYPE1v2_IOMMU))
    		die("vfio container");
    	return ioctl(group, VFIO_GROUP_GET_DEVICE_FD, bdf);
    }

    static void set_memory(int on)
    {
    	uint16_t cmd;

    	if (pread(dev, &cmd, 2, cfg_off + PCI_COMMAND) != 2)
    		die("read PCI_COMMAND");
    	cmd = on ? cmd | PCI_COMMAND_MEMORY : cmd & ~PCI_COMMAND_MEMORY;
    	if (pwrite(dev, &cmd, 2, cfg_off + PCI_COMMAND) != 2)
    		die("write PCI_COMMAND");
    }

    static void *revoker(void *arg)
    {
    	unsigned long i;

    	while (read(go_pipe[0], &i, sizeof(i)) == sizeof(i)) {
    		if (max_delay_us)
    			usleep(random() % max_delay_us);
    		set_memory(0);
    		shared[0] = i;
    		if (write(done_pipe[1], &i, sizeof(i)) != sizeof(i))
    			die("done pipe");
    	}
    	return NULL;
    }

    static void sigbus(int sig)
    {
    	siglongjmp(jb, 1);
    }

    /* Number of 2MB chunks of the BAR that can be read without SIGBUS. */
    static unsigned long readable(volatile uint8_t *bar, size_t size)
    {
    	volatile unsigned long n = 0;
    	volatile size_t off;

    	for (off = 0; off < size; off += SZ_2M) {
    		if (sigsetjmp(jb, 1))
    			continue;
    		(void)bar[off];
    		n++;
    	}
    	return n;
    }

    int main(int argc, char **argv)
    {
    	unsigned long iterations = argc > 2 ? strtoul(argv[2], NULL, 0) : 1000;
    	unsigned int index = argc > 3 ? atoi(argv[3]) : 0;
    	struct vfio_region_info cfg = { .argsz = sizeof(cfg),
    					.index = VFIO_PCI_CONFIG_REGION_INDEX };
    	struct vfio_region_info bar = { .argsz = sizeof(bar), .index = index };
    	unsigned long stale_children = 0, stale_chunks = 0, n;
    	volatile uint8_t *map;
    	pthread_t thread;
    	size_t size, off;
    	uint8_t *resv;
    	int status;
    	pid_t pid;

    	if (argc < 2) {
    		fprintf(stderr, "usage: %s <bdf> [iterations [bar [max_delay_us]]]\n",
    			argv[0]);
    		return 2;
    	}
    	max_delay_us = argc > 4 ? atoi(argv[4]) : 0;
    	signal(SIGBUS, sigbus);
    	dev = device_fd(argv[1]);
    	if (dev < 0)
    		die("device fd");
    	if (ioctl(dev, VFIO_DEVICE_GET_REGION_INFO, &cfg) ||
    	    ioctl(dev, VFIO_DEVICE_GET_REGION_INFO, &bar))
    		die("region info");
    	if (!(bar.flags & VFIO_REGION_INFO_FLAG_MMAP) || bar.size < SZ_2M) {
    		fprintf(stderr, "BAR %u: no mmap or smaller than 2MB\n", index);
    		return 2;
    	}
    	cfg_off = cfg.offset;
    	size = bar.size & ~(SZ_2M - 1);

    	/* A 2MB aligned address lets vfio-pci map the BAR with huge PMDs. */
    	resv = mmap(NULL, size + SZ_2M, PROT_NONE,
    		    MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
    	if (resv == MAP_FAILED)
    		die("reserve");
    	resv = (uint8_t *)(((uintptr_t)resv + SZ_2M - 1) & ~(SZ_2M - 1));
    	map = mmap(resv, size, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
    		   dev, bar.offset);
    	if (map == MAP_FAILED)
    		die("mmap BAR");
    	shared = mmap(NULL, 4096, PROT_READ | PROT_WRITE,
    		      MAP_SHARED | MAP_ANONYMOUS, -1, 0);
    	if (shared == MAP_FAILED)
    		die("shared page");

    	/* Control: without a revoke the child reads what the parent wrote. */
    	set_memory(1);
    	for (off = 0; off < size; off += SZ_2M)
    		map[off] = (uint8_t)(off / SZ_2M) ^ 0x5a;
    	pid = fork();
    	if (!pid) {
    		for (off = 0; off < size; off += SZ_2M)
    			if (map[off] != ((uint8_t)(off / SZ_2M) ^ 0x5a))
    				_exit(1);
    		_exit(0);
    	}
    	if (pid < 0 || waitpid(pid, &status, 0) != pid ||
    	    !WIFEXITED(status) || WEXITSTATUS(status)) {
    		fprintf(stderr, "control: child does not see the parent's BAR\n");
    		return 2;
    	}
    	printf("BAR %u: %zu MB, %lu iterations, revoke delay < %u us\n",
    	       index, size >> 20, iterations, max_delay_us);

    	/* Premise: with the memory disabled the BAR cannot be read. */
    	set_memory(0);
    	n = readable(map, size);
    	set_memory(1);
    	if (n) {
    		fprintf(stderr, "memory disabled, parent still reads %lu chunks\n", n);
    		return 2;
    	}

    	if (pipe(go_pipe) || pipe(done_pipe))
    		die("pipe");
    	if (pthread_create(&thread, NULL, revoker, NULL))
    		die("thread");

    	for (iter = 1; iter <= iterations; iter++) {
    		set_memory(1);
    		for (off = 0; off < size; off += SZ_2M)
    			(void)map[off];
    		if (write(go_pipe[1], &iter, sizeof(iter)) != sizeof(iter))
    			die("go pipe");
    		pid = fork();
    		if (pid < 0)
    			die("fork");
    		if (!pid) {
    			while (shared[0] != iter)
    				sched_yield();
    			n = readable(map, size);
    			_exit(n > 255 ? 255 : n);
    		}
    		if (read(done_pipe[0], &n, sizeof(n)) != sizeof(n))
    			die("done pipe");
    		if (waitpid(pid, &status, 0) != pid || !WIFEXITED(status))
    			die("child");
    		if (WEXITSTATUS(status)) {
    			stale_children++;
    			stale_chunks += WEXITSTATUS(status);
    		}
    	}
    	set_memory(1);
    	printf("children with a stale BAR mapping: %lu of %lu (%lu 2MB chunks)\n",
    	       stale_children, iterations, stale_chunks);
    	return stale_children ? 1 : 0;
    }
---
 mm/huge_memory.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 1e5d68acf62a5..fcea98f0d8865 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		src_ptl = pmd_lockptr(src_mm, src_pmd);
 		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
 		/*
-		 * No need to recheck the pmd, it can't change with write
-		 * mmap lock held here.
-		 *
-		 * Meanwhile, making sure it's not a CoW VMA with writable
-		 * mapping, otherwise it means either the anon page wrongly
-		 * applied special bit, or we made the PRIVATE mapping be
-		 * able to wrongly write to the backend MMIO.
+		 * The mmap and VMA write locks of the source keep faults
+		 * out, but unmap_mapping_range() does not take the mmap
+		 * lock and may have cleared the pmd since the lockless
+		 * read. Let copy_pmd_range() skip it then.
+		 */
+		pmd = pmdp_get(src_pmd);
+		if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {
+			spin_unlock(src_ptl);
+			spin_unlock(dst_ptl);
+			return -EAGAIN;
+		}
+		/*
+		 * Make sure it's not a CoW VMA with writable mapping,
+		 * otherwise it means either the anon page wrongly applied
+		 * special bit, or we made the PRIVATE mapping be able to
+		 * wrongly write to the backend MMIO.
 		 */
 		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
 		goto set_pmd;

---
base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
change-id: 20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-bdf7efb612c2

Best regards,
--  
Konstantin Khorenko <khorenko@virtuozzo.com>

Re: [PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: NULL pointer dereference in zap_huge_pmd() for special PMDs on PowerPC
--

commit 5100f57130de5473973d1370273238b4778fae24
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

copy_huge_pmd() reads the source PMD without the page table lock and,
for a special PMD, installs that value in the child without reading it
again under the lock. This commit reads the PMD again under the lock to
ensure it wasn't cleared concurrently by unmap_mapping_range().

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 1e5d68acf62a5..fcea98f0d8865 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  		src_ptl = pmd_lockptr(src_mm, src_pmd);
>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>  		/*
> -		 * No need to recheck the pmd, it can't change with write
> -		 * mmap lock held here.
> -		 *
> -		 * Meanwhile, making sure it's not a CoW VMA with writable
> -		 * mapping, otherwise it means either the anon page wrongly
> -		 * applied special bit, or we made the PRIVATE mapping be
> -		 * able to wrongly write to the backend MMIO.
> +		 * The mmap and VMA write locks of the source keep faults
> +		 * out, but unmap_mapping_range() does not take the mmap
> +		 * lock and may have cleared the pmd since the lockless
> +		 * read. Let copy_pmd_range() skip it then.
> +		 */
> +		pmd = pmdp_get(src_pmd);
> +		if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {
> +			spin_unlock(src_ptl);
> +			spin_unlock(dst_ptl);
> +			return -EAGAIN;
> +		}
> +		/*
> +		 * Make sure it's not a CoW VMA with writable mapping,
> +		 * otherwise it means either the anon page wrongly applied
> +		 * special bit, or we made the PRIVATE mapping be able to
> +		 * wrongly write to the backend MMIO.
>  		 */
>  		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>  		goto set_pmd;

[Severity: High]
Does this code cause a NULL pointer dereference on PowerPC when a special
PMD is unmapped by the child?

Because the jump to set_pmd bypasses pgtable allocation and deposition
for the child process's new special PMD, no pgtable is deposited.
Later, when the child unmaps the VMA or exits, zap_pmd_range() calls
zap_huge_pmd().

On architectures like PowerPC (hash mode) where
arch_needs_pgtable_deposit() is true, this leads to:

mm/huge_memory.c:zap_huge_pmd() {
    ...
    if (has_deposit)
        zap_deposited_table(mm, pmd);
    ...
}

This will unconditionally assume a deposited table exists and try to
withdraw it. Then in zap_deposited_table():

mm/huge_memory.c:zap_deposited_table() {
    ...
    pgtable = pgtable_trans_huge_withdraw(mm, pmd);
    pte_free(mm, pgtable);
    ...
}

Since no page table was ever deposited, pgtable_trans_huge_withdraw()
returns NULL, and passing NULL to pte_free() leads to a kernel crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-v1-1-d225c60bc316@virtuozzo.com?part=1

Re: [PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[Konstantin Khorenko]

On 10/14/26 14:15, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm: NULL pointer dereference in zap_huge_pmd() for special PMDs on PowerPC
> --
> 
> commit 5100f57130de5473973d1370273238b4778fae24
> Author: Konstantin Khorenko <khorenko@virtuozzo.com>
> 
> mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()
> 
> copy_huge_pmd() reads the source PMD without the page table lock and,
> for a special PMD, installs that value in the child without reading it
> again under the lock. This commit reads the PMD again under the lock to
> ensure it wasn't cleared concurrently by unmap_mapping_range().
> 
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 1e5d68acf62a5..fcea98f0d8865 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>>  		src_ptl = pmd_lockptr(src_mm, src_pmd);
>>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>>  		/*
>> -		 * No need to recheck the pmd, it can't change with write
>> -		 * mmap lock held here.
>> -		 *
>> -		 * Meanwhile, making sure it's not a CoW VMA with writable
>> -		 * mapping, otherwise it means either the anon page wrongly
>> -		 * applied special bit, or we made the PRIVATE mapping be
>> -		 * able to wrongly write to the backend MMIO.
>> +		 * The mmap and VMA write locks of the source keep faults
>> +		 * out, but unmap_mapping_range() does not take the mmap
>> +		 * lock and may have cleared the pmd since the lockless
>> +		 * read. Let copy_pmd_range() skip it then.
>> +		 */
>> +		pmd = pmdp_get(src_pmd);
>> +		if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {
>> +			spin_unlock(src_ptl);
>> +			spin_unlock(dst_ptl);
>> +			return -EAGAIN;
>> +		}
>> +		/*
>> +		 * Make sure it's not a CoW VMA with writable mapping,
>> +		 * otherwise it means either the anon page wrongly applied
>> +		 * special bit, or we made the PRIVATE mapping be able to
>> +		 * wrongly write to the backend MMIO.
>>  		 */
>>  		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>>  		goto set_pmd;
> [Severity: High]
> Does this code cause a NULL pointer dereference on PowerPC when a special
> PMD is unmapped by the child?
> 
> Because the jump to set_pmd bypasses pgtable allocation and deposition
> for the child process's new special PMD, no pgtable is deposited.
> Later, when the child unmaps the VMA or exits, zap_pmd_range() calls
> zap_huge_pmd().
> 
> On architectures like PowerPC (hash mode) where
> arch_needs_pgtable_deposit() is true, this leads to:
> 
> mm/huge_memory.c:zap_huge_pmd() {
>     ...
>     if (has_deposit)
>         zap_deposited_table(mm, pmd);
>     ...
> }
> 
> This will unconditionally assume a deposited table exists and try to
> withdraw it. Then in zap_deposited_table():
> 
> mm/huge_memory.c:zap_deposited_table() {
>     ...
>     pgtable = pgtable_trans_huge_withdraw(mm, pmd);
>     pte_free(mm, pgtable);
>     ...
> }
> 
> Since no page table was ever deposited, pgtable_trans_huge_withdraw()
> returns NULL, and passing NULL to pte_free() leads to a kernel crash.

Thanks, that is a real bug, but it is not introduced by this patch: the
special PMD branch of copy_huge_pmd() has never deposited a page table
for the child, since commit bc02afbd4d73 ("mm/fork: accept huge pfnmap
entries"). This patch only rereads the PMD under the lock in that
branch and does not change what it installs.

It is also worse than the zap: on ppc64 in hash mode the child's first
hash fault on that PMD already dereferences the missing table, in
__hash_page_thp() through get_hpte_slot_array(), and the zap then hits
the memset() in hash__pgtable_trans_huge_withdraw() before pte_free().

I will send a v2 with a separate patch on top of this one that
allocates and deposits the table when arch_needs_pgtable_deposit() is
true, as insert_pmd() does.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

> 
> -- Sashiko AI review · https://sashiko.dev/#/patchset/20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-v1-1-d225c60bc316@virtuozzo.com?part=1
> 

Re: [PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[Lance Yang]

On 2026/10/14 19:57, Konstantin Khorenko wrote:
[...]
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 1e5d68acf62a5..fcea98f0d8865 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>   		src_ptl = pmd_lockptr(src_mm, src_pmd);
>   		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>   		/*
> -		 * No need to recheck the pmd, it can't change with write
> -		 * mmap lock held here.
> -		 *
> -		 * Meanwhile, making sure it's not a CoW VMA with writable
> -		 * mapping, otherwise it means either the anon page wrongly
> -		 * applied special bit, or we made the PRIVATE mapping be
> -		 * able to wrongly write to the backend MMIO.
> +		 * The mmap and VMA write locks of the source keep faults
> +		 * out, but unmap_mapping_range() does not take the mmap
> +		 * lock and may have cleared the pmd since the lockless
> +		 * read. Let copy_pmd_range() skip it then.
> +		 */
> +		pmd = pmdp_get(src_pmd);
> +		if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {

Why not just check pmd_none(pmd) here? We're holding the source mmap/VMA
write locks, so the PMD either stays special or gets cleared, right?

Cheers, Lance

> +			spin_unlock(src_ptl);
> +			spin_unlock(dst_ptl);
> +			return -EAGAIN;
> +		}
> +		/*
> +		 * Make sure it's not a CoW VMA with writable mapping,
> +		 * otherwise it means either the anon page wrongly applied
> +		 * special bit, or we made the PRIVATE mapping be able to
> +		 * wrongly write to the backend MMIO.
>   		 */
>   		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>   		goto set_pmd;
> 
> ---
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
> change-id: 20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-bdf7efb612c2
> 
> Best regards,
> --
> Konstantin Khorenko <khorenko@virtuozzo.com>
> 

Re: [PATCH] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

[Konstantin Khorenko]

On 10/14/26 15:19, Lance Yang wrote:
> 
> 
> On 2026/10/14 19:57, Konstantin Khorenko wrote:
> [...]
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 1e5d68acf62a5..fcea98f0d8865 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>>   		src_ptl = pmd_lockptr(src_mm, src_pmd);
>>   		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>>   		/*
>> -		 * No need to recheck the pmd, it can't change with write
>> -		 * mmap lock held here.
>> -		 *
>> -		 * Meanwhile, making sure it's not a CoW VMA with writable
>> -		 * mapping, otherwise it means either the anon page wrongly
>> -		 * applied special bit, or we made the PRIVATE mapping be
>> -		 * able to wrongly write to the backend MMIO.
>> +		 * The mmap and VMA write locks of the source keep faults
>> +		 * out, but unmap_mapping_range() does not take the mmap
>> +		 * lock and may have cleared the pmd since the lockless
>> +		 * read. Let copy_pmd_range() skip it then.
>> +		 */
>> +		pmd = pmdp_get(src_pmd);
>> +		if (unlikely(!pmd_present(pmd) || !pmd_special(pmd))) {
> 
> Why not just check pmd_none(pmd) here? We're holding the source mmap/VMA
> write locks, so the PMD either stays special or gets cleared, right?

Right: with the source mmap and VMA write locks held, only a zap can
change it, and both ways a zap takes leave the PMD none:
zap_huge_pmd() clears it, and a partial zap splits it in
__split_huge_pmd_locked(), which for a special VMA only clears it and
returns.

I tested the same condition as the lockless check that chose this
branch, so that the branch never copies anything it did not select,
but taking into account the above paragraph we can easily use pmd_none()
here, it even better matches the comment :), so I will switch to it in v3.

v2 is already out, with a second patch for the missing page table
deposit on ppc64 hash:
https://lore.kernel.org/r/20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-v2-0-e1548488fa68@virtuozzo.com

The pmd_none() change will go into v3.

Cheers, Konstantin


>> +			spin_unlock(src_ptl);
>> +			spin_unlock(dst_ptl);
>> +			return -EAGAIN;
>> +		}
>> +		/*
>> +		 * Make sure it's not a CoW VMA with writable mapping,
>> +		 * otherwise it means either the anon page wrongly applied
>> +		 * special bit, or we made the PRIVATE mapping be able to
>> +		 * wrongly write to the backend MMIO.
>>   		 */
>>   		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>>   		goto set_pmd;
>>
>> ---
>> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
>> change-id: 20261014-b4-prep-vstor-149847-copy_huge_pmd-recheck-special-pmd-bdf7efb612c2
>>
>> Best regards,
>> --
>> Konstantin Khorenko <khorenko@virtuozzo.com>
>>
>