Re: [PATCH 00/15] mm/rmap: index MAP_PRIVATE file-backed folios by virt pgoff

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

Dealing with the sashiko feedback ([0]):

Since there's not been review of this yet, I'll send a quick respin with the
valid bits of the below addressed.

Many duplicated variants of:
> vma_start_virt_pgoff() is wrong for zero-initialised VMAs prior to
> their page offset being set up

<incorrect>

Sashiko seems to be using a crystal ball to predict that I won't set this
field at the same time vma->vm_pgoff is set, which is incorrect.

Nor does it quite understand 'this feature is not switched on yet' :)

Also in the sensitive case where asserts might fail due to chicken-and-egg
'assigning to a fresh VMA', I use e.g. __vma_set_range() which doesn't perform
the assert.

Note that vma_set_range(), which is typically how the pgoff field is set, now
also sets virtual page offset.

Many duplicated variants of:
> vma->vm_file is an incorrect check for anon because of 'special' VMAs such as
> VDSO, VVAR etc. not setting this, so don't assert !vma->vm_file and assume
> it's anon.

<incorrect in practice, but dubiously valid>

These are only called in instances where a virtual variant of vma_start_pgoff(),
linear_page_index(), etc. are called, that is places where a 'special' VMA
should not have that called on it.

However for belts + braces will update the asserts to also test
vma_is_anonymous() (the 'special' VMAs have vma->vm_ops set so will be excluded
by this).

Also noticed linear_virt_page_index() was not updated to account for
MAP_PRIVATE-/dev/zero being made into pure anon, so will fix that too.

(will send fixes for this on respin)

> Does this patch miss updating the VMA merge criteria to check anon_pgoff
> contiguity?...

<incorrect>

Another case of sashiko not understanding the meaning of 'this feature
isn't enabled yet'.

I make the relevant change _when I enable the feature_ to avoid bisection
hazards.

>Can this result in a NULL pointer dereference when CONFIG_DEBUG_VM is enabled?

<correct but already addressed>

Yup :) this is valid but already reported by syzbot and already fixed, both
inline with a note to Andrew and also locally to be sent in any respin.

> Are there additional core anonymous folio creation and validation paths
> that also need this update to prevent breaking reverse mapping lookups
> when the underlying behavior changes?

<incorrect>

More of sashiko not understanding the concept of bisection hazards or the
fact the feature is not switched on yet.

> Does this change break NUMA node interleaving for mixed MAP_PRIVATE
> mappings? (9/15)

<incorrect assessment, valid that it's something that changes>

It doesn't 'break' anything, but it does affect it.

Commit 88c91dc58582 ("mempolicy: migration attempt to match interleave
nodes") already makes it clear that this is best effort and may cross VMA
boundaries, so this was already fuzzy in the sense that it's best effort
and designed to endure a varying 'base'.

So CoW'd folios of MAP_PRIVATE-mapped file-backed mappings will simply be
equivalent to the base varying.

It's also very unlikely that anything in the wild relying upon this
interleaving behaviour will be doing significant amounts of MAP_PRIVATE
CoW'ing, as in practice this is largely used for things like ELF image
relocations, etc.

I will update the commit message to mention it.

> Does this code miss asserting virt_pgoff for adjacent VMAs modified during
> the merge? (10/15)

<correct>

Trivial VMA userland test change, fixed up will send on respin.

> Does this code unintentionally overwrite the anonymous state of
> MAP_PRIVATE /dev/zero mappings? (12/15)

<correct>

Yeah it does... I deal with this correctly when I make
MAP_PRIVATE-/dev/zero mappings pure anon, but this is a bisection hazard,
so have fixed this for respin by gating on !map_is_anon() when calling
set_vma_user_defined_fields().

> Does this missing device type check in map_is_dev_zero() allow block devices
> sharing the same major/minor numbers to falsely match and lose their mapping
> data here?

<correct>

Yes :)) I added an `S_ISCHR(inode->i_mode)` check locally. I was not aware
that char and block devices have entirely separately major/minor
namespaces...

Cheers, Lorenzo

[0]:https://sashiko.dev/#/patchset/20260717-b4-scalable-cow-virt-pgoff-v1-0-cf24910ef094%40kernel.org