* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-19 9:17 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <41af4ffb-0383-4d00-9639-0bf16e1f5f37@redhat.com>
On Wed, Feb 19, 2025 at 10:15:47AM +0100, David Hildenbrand wrote:
> On 19.02.25 10:03, Lorenzo Stoakes wrote:
> > On Wed, Feb 19, 2025 at 12:25:51AM -0800, Kalesh Singh wrote:
> > > On Thu, Feb 13, 2025 at 10:18 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > The guard regions feature was initially implemented to support anonymous
> > > > mappings only, excluding shmem.
> > > >
> > > > This was done such as to introduce the feature carefully and incrementally
> > > > and to be conservative when considering the various caveats and corner
> > > > cases that are applicable to file-backed mappings but not to anonymous
> > > > ones.
> > > >
> > > > Now this feature has landed in 6.13, it is time to revisit this and to
> > > > extend this functionality to file-backed and shmem mappings.
> > > >
> > > > In order to make this maximally useful, and since one may map file-backed
> > > > mappings read-only (for instance ELF images), we also remove the
> > > > restriction on read-only mappings and permit the establishment of guard
> > > > regions in any non-hugetlb, non-mlock()'d mapping.
> > >
> > > Hi Lorenzo,
> > >
> > > Thank you for your work on this.
> >
> > You're welcome.
> >
> > >
> > > Have we thought about how guard regions are represented in /proc/*/[s]maps?
> >
> > This is off-topic here but... Yes, extensively. No they do not appear
> > there.
> >
> > I thought you had attended LPC and my talk where I mentioned this
> > purposefully as a drawback?
> >
> > I went out of my way to advertise this limitation at the LPC talk, in the
> > original series, etc. so it's a little disappointing that this is being
> > brought up so late, but nobody else has raised objections to this issue so
> > I think in general it's not a limitation that matters in practice.
> >
> > >
> > > In the field, I've found that many applications read the ranges from
> > > /proc/self/[s]maps to determine what they can access (usually related
> > > to obfuscation techniques). If they don't know of the guard regions it
> > > would cause them to crash; I think that we'll need similar entries to
> > > PROT_NONE (---p) for these, and generally to maintain consistency
> > > between the behavior and what is being said from /proc/*/[s]maps.
> >
> > No, we cannot have these, sorry.
> >
> > Firstly /proc/$pid/[s]maps describes VMAs. The entire purpose of this
> > feature is to avoid having to accumulate VMAs for regions which are not
> > intended to be accessible.
> >
> > Secondly, there is no practical means for this to be accomplished in
> > /proc/$pid/maps in _any_ way - as no metadata relating to a VMA indicates
> > they have guard regions.
> >
> > This is intentional, because setting such metadata is simply not practical
> > - why? Because when you try to split the VMA, how do you know which bit
> > gets the metadata and which doesn't? You can't without _reading page
> > tables_.
> >
> > /proc/$pid/smaps _does_ read page tables, but we can't start pretending
> > VMAs exist when they don't, this would be completely inaccurate, would
> > break assumptions for things like mremap (which require a single VMA) and
> > would be unworkable.
> >
> > The best that _could_ be achieved is to have a marker in /proc/$pid/smaps
> > saying 'hey this region has guard regions somewhere'.
>
> And then simply expose it in /proc/$pid/pagemap, which is a better interface
> for this pte-level information inside of VMAs. We should still have a spare
> bit for that purpose in the pagemap entries.
Ah yeah thanks David forgot about that!
This is also a possibility if that'd solve your problems Kalesh?
This bit will be fought over haha
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH 0/2 v9] reclaim file-backed pages given POSIX_FADV_NOREUSE
From: Jaegeuk Kim @ 2025-02-19 9:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, linux-f2fs-devel, linux-fsdevel, linux-mm,
linux-api
In-Reply-To: <Z7REHrJ3ImdrF476@infradead.org>
On 02/18, Christoph Hellwig wrote:
> This still has a file system sysfs HACK, you're still not Ccing the
> right list, etc.
>
> Can you pleae at least try to get it right?
I was modifying the patch having 1) declaring a static global list, 2) adding
some fields to superblock and inode structures to keep the given range in the
inode through fadvise, 3) adding hooks in evict_inode to handle the list, 4)
exploring which sysfs entry in MM to reclaim them explicitly.
But, I stopped at some point, as it looks not good at all. Moreover, I started
to be questioning why not just doing in F2FS back, given sementically I didn't
change anything on general behavior of fadvise(POSIX_FADV_NOREUSE), IIUC, which
moves pages back to LRU. In addiiton to that, I'd like to keep the range hint in
a filesystem and provide a sysfs entry to manage the hints additionally.
In addition, I don't think there's rule that filesystem cannot reclaim file-back
pages, as it just uses the exported symbol that all filesystems are using in
various different purpose. Hence, I don't get the point which is wrong.
Thanks,
>
> On Wed, Feb 12, 2025 at 02:31:55AM +0000, Jaegeuk Kim wrote:
> > This patch series does not add new API, but implements POSIX_FADV_NOREUSE where
> > it keeps the page ranges in the f2fs superblock and add a way for users to
> > reclaim the pages manually.
> >
> > Change log from v8:
> > - remove new APIs, but use fadvise(POSIX_FADV_NOREUSE)
> >
> > Jaegeuk Kim (2):
> > f2fs: keep POSIX_FADV_NOREUSE ranges
> > f2fs: add a sysfs entry to reclaim POSIX_FADV_NOREUSE pages
> >
> > Documentation/ABI/testing/sysfs-fs-f2fs | 7 ++
> > fs/f2fs/debug.c | 3 +
> > fs/f2fs/f2fs.h | 14 +++-
> > fs/f2fs/file.c | 60 +++++++++++++++--
> > fs/f2fs/inode.c | 14 ++++
> > fs/f2fs/shrinker.c | 90 +++++++++++++++++++++++++
> > fs/f2fs/super.c | 1 +
> > fs/f2fs/sysfs.c | 63 +++++++++++++++++
> > 8 files changed, 246 insertions(+), 6 deletions(-)
> >
> > --
> > 2.48.1.601.g30ceb7b040-goog
> >
> >
> ---end quoted text---
^ permalink raw reply
* Re: [PATCH] ntsync: Set the permissions to be 0666
From: Greg Kroah-Hartman @ 2025-02-19 14:22 UTC (permalink / raw)
To: Elizabeth Figura
Cc: dri-devel, Arnd Bergmann, Jonathan Corbet, Shuah Khan,
Mike Lothian, linux-kernel, linux-api, wine-devel,
André Almeida, Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra,
Andy Lutomirski, Randy Dunlap, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng
In-Reply-To: <2768333.mvXUDI8C0e@camazotz>
On Tue, Feb 18, 2025 at 05:57:31PM -0600, Elizabeth Figura wrote:
> On Friday, 14 February 2025 06:28:00 CST Mike Lothian wrote:
> > This allows ntsync to be usuable by non-root processes out of the box
> >
> > Signed-off-by: Mike Lothian <mike@fireburn.co.uk>
> > ---
> > drivers/misc/ntsync.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
> > index 055395cde42b..586b86243e1d 100644
> > --- a/drivers/misc/ntsync.c
> > +++ b/drivers/misc/ntsync.c
> > @@ -1208,6 +1208,7 @@ static struct miscdevice ntsync_misc = {
> > .minor = MISC_DYNAMIC_MINOR,
> > .name = NTSYNC_NAME,
> > .fops = &ntsync_fops,
> > + .mode = 0666, // Setting file permissions to 0666
> > };
> >
> > module_misc_device(ntsync_misc);
> >
>
> Reviewed-by: Elizabeth Figura <zfigura@codeweavers.com>
>
> --
>
> The comment seems rather redundant, but otherwise this is correct and prudent.
I agree, I'll drop the comment when I apply it, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Liam R. Howlett @ 2025-02-19 17:32 UTC (permalink / raw)
To: Kalesh Singh
Cc: Lorenzo Stoakes, Andrew Morton, Suren Baghdasaryan,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
David Hildenbrand, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <CAC_TJvfPNkJDWnG81GnJcFeMLYzN8=uM-oTrK6FKT7tD=E4TQg@mail.gmail.com>
* Kalesh Singh <kaleshsingh@google.com> [250219 03:35]:
> On Wed, Feb 19, 2025 at 12:25 AM Kalesh Singh <kaleshsingh@google.com> wrote:
> >
> > On Thu, Feb 13, 2025 at 10:18 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > The guard regions feature was initially implemented to support anonymous
> > > mappings only, excluding shmem.
> > >
> > > This was done such as to introduce the feature carefully and incrementally
> > > and to be conservative when considering the various caveats and corner
> > > cases that are applicable to file-backed mappings but not to anonymous
> > > ones.
> > >
> > > Now this feature has landed in 6.13, it is time to revisit this and to
> > > extend this functionality to file-backed and shmem mappings.
> > >
> > > In order to make this maximally useful, and since one may map file-backed
> > > mappings read-only (for instance ELF images), we also remove the
> > > restriction on read-only mappings and permit the establishment of guard
> > > regions in any non-hugetlb, non-mlock()'d mapping.
> >
> > Hi Lorenzo,
> >
> > Thank you for your work on this.
> >
> > Have we thought about how guard regions are represented in /proc/*/[s]maps?
> >
> > In the field, I've found that many applications read the ranges from
> > /proc/self/[s]maps to determine what they can access (usually related
> > to obfuscation techniques). If they don't know of the guard regions it
> > would cause them to crash; I think that we'll need similar entries to
> > PROT_NONE (---p) for these, and generally to maintain consistency
> > between the behavior and what is being said from /proc/*/[s]maps.
>
> To clarify why the applications may not be aware of their guard
> regions -- in the case of the ELF mappings these PROT_NONE (guard
> regions) would be installed by the dynamic loader; or may be inherited
> from the parent (zygote in Android's case).
The guard regions are a method to reduce vma count and speed up guard
installation and removal. This very much will change the proc interface
by its design, since it's removing information from the maps for the
advantage of speed and less resources. I thought that was pretty clear
from the start.
The proc interface is also not a very good way to programmatically get
this information, as I'm sure you are aware. I'm also sure you know
that reading that file takes the mmap read lock, and tearing can occur.
I think this implies you are taking a lot of time to get the information
you want in the way you are getting it (parsing a text file, and not
doing any mmap write work)?
If this is a common pattern, I think we need a better interface to query
this type of information. We have an ioctl going in for getting vma
information, but that will lack these new guard regions as well.
David mentioned /proc/self/pagemap - that's certainly worth exploring,
and doesn't involve text parsing.
If it's not that common, then maybe your zygote/loader can communicate
the information in a way that does not involve read locking the programs
vm area?
Are you really parsing the same library information for each application
launched to find the guards instead of asking or being told what they
are? I must be missing something..
Thanks,
Liam
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Kalesh Singh @ 2025-02-19 18:52 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <a2e12142-3eb2-48c9-b0d9-35a86cb56eec@lucifer.local>
On Wed, Feb 19, 2025 at 1:17 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Feb 19, 2025 at 10:15:47AM +0100, David Hildenbrand wrote:
> > On 19.02.25 10:03, Lorenzo Stoakes wrote:
> > > On Wed, Feb 19, 2025 at 12:25:51AM -0800, Kalesh Singh wrote:
> > > > On Thu, Feb 13, 2025 at 10:18 AM Lorenzo Stoakes
> > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > >
> > > > > The guard regions feature was initially implemented to support anonymous
> > > > > mappings only, excluding shmem.
> > > > >
> > > > > This was done such as to introduce the feature carefully and incrementally
> > > > > and to be conservative when considering the various caveats and corner
> > > > > cases that are applicable to file-backed mappings but not to anonymous
> > > > > ones.
> > > > >
> > > > > Now this feature has landed in 6.13, it is time to revisit this and to
> > > > > extend this functionality to file-backed and shmem mappings.
> > > > >
> > > > > In order to make this maximally useful, and since one may map file-backed
> > > > > mappings read-only (for instance ELF images), we also remove the
> > > > > restriction on read-only mappings and permit the establishment of guard
> > > > > regions in any non-hugetlb, non-mlock()'d mapping.
> > > >
> > > > Hi Lorenzo,
> > > >
> > > > Thank you for your work on this.
> > >
> > > You're welcome.
> > >
> > > >
> > > > Have we thought about how guard regions are represented in /proc/*/[s]maps?
> > >
> > > This is off-topic here but... Yes, extensively. No they do not appear
> > > there.
> > >
> > > I thought you had attended LPC and my talk where I mentioned this
> > > purposefully as a drawback?
> > >
> > > I went out of my way to advertise this limitation at the LPC talk, in the
> > > original series, etc. so it's a little disappointing that this is being
> > > brought up so late, but nobody else has raised objections to this issue so
> > > I think in general it's not a limitation that matters in practice.
> > >
Sorry for raising this now, yes at the time I believe we discussed
reducing the vma slab memory usage for the PROT_NONE mappings. I
didn't imagine that apps could have dependencies on the mapped ELF
ranges in /proc/self/[s]maps until recent breakages from a similar
feature. Android itself doesn't depend on this but what I've seen is
banking apps and apps that have obfuscation to prevent reverse
engineering (the particulars of such obfuscation are a black box).
> > > >
> > > > In the field, I've found that many applications read the ranges from
> > > > /proc/self/[s]maps to determine what they can access (usually related
> > > > to obfuscation techniques). If they don't know of the guard regions it
> > > > would cause them to crash; I think that we'll need similar entries to
> > > > PROT_NONE (---p) for these, and generally to maintain consistency
> > > > between the behavior and what is being said from /proc/*/[s]maps.
> > >
> > > No, we cannot have these, sorry.
> > >
> > > Firstly /proc/$pid/[s]maps describes VMAs. The entire purpose of this
> > > feature is to avoid having to accumulate VMAs for regions which are not
> > > intended to be accessible.
> > >
> > > Secondly, there is no practical means for this to be accomplished in
> > > /proc/$pid/maps in _any_ way - as no metadata relating to a VMA indicates
> > > they have guard regions.
> > >
> > > This is intentional, because setting such metadata is simply not practical
> > > - why? Because when you try to split the VMA, how do you know which bit
> > > gets the metadata and which doesn't? You can't without _reading page
> > > tables_.
Yeah the splitting becomes complicated with any vm flags for this...
meaning any attempt to expose this in /proc/*/maps have to
unconditionally walk the page tables :(
> > >
> > > /proc/$pid/smaps _does_ read page tables, but we can't start pretending
> > > VMAs exist when they don't, this would be completely inaccurate, would
> > > break assumptions for things like mremap (which require a single VMA) and
> > > would be unworkable.
> > >
> > > The best that _could_ be achieved is to have a marker in /proc/$pid/smaps
> > > saying 'hey this region has guard regions somewhere'.
> >
> > And then simply expose it in /proc/$pid/pagemap, which is a better interface
> > for this pte-level information inside of VMAs. We should still have a spare
> > bit for that purpose in the pagemap entries.
>
> Ah yeah thanks David forgot about that!
>
> This is also a possibility if that'd solve your problems Kalesh?
I'm not sure what is the correct interface to advertise these. Maybe
smaps as you suggested since we already walk the page tables there?
and pagemap bit for the exact pages as well? It won't solve this
particular issue, as 1000s of in field apps do look at this through
/proc/*/maps. But maybe we have to live with that...
We can argue that such apps are broken since they may trip on the
SIGBUS off the end of the file -- usually this isn't the case for the
ELF segment mappings.
This is still useful for other cases, I just wanted to get some ideas
if this can be extended to further use cases.
Thanks,
Kalesh
>
> This bit will be fought over haha
>
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-19 19:20 UTC (permalink / raw)
To: Kalesh Singh
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <CAC_TJvf6fOACObzR0ANFFrD+ecrP8MbXEZ_ZdzRu0Lg4RunS9g@mail.gmail.com>
On Wed, Feb 19, 2025 at 10:52:04AM -0800, Kalesh Singh wrote:
> On Wed, Feb 19, 2025 at 1:17 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Wed, Feb 19, 2025 at 10:15:47AM +0100, David Hildenbrand wrote:
> > > On 19.02.25 10:03, Lorenzo Stoakes wrote:
> > > > On Wed, Feb 19, 2025 at 12:25:51AM -0800, Kalesh Singh wrote:
> > > > > On Thu, Feb 13, 2025 at 10:18 AM Lorenzo Stoakes
> > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > >
> > > > > > The guard regions feature was initially implemented to support anonymous
> > > > > > mappings only, excluding shmem.
> > > > > >
> > > > > > This was done such as to introduce the feature carefully and incrementally
> > > > > > and to be conservative when considering the various caveats and corner
> > > > > > cases that are applicable to file-backed mappings but not to anonymous
> > > > > > ones.
> > > > > >
> > > > > > Now this feature has landed in 6.13, it is time to revisit this and to
> > > > > > extend this functionality to file-backed and shmem mappings.
> > > > > >
> > > > > > In order to make this maximally useful, and since one may map file-backed
> > > > > > mappings read-only (for instance ELF images), we also remove the
> > > > > > restriction on read-only mappings and permit the establishment of guard
> > > > > > regions in any non-hugetlb, non-mlock()'d mapping.
> > > > >
> > > > > Hi Lorenzo,
> > > > >
> > > > > Thank you for your work on this.
> > > >
> > > > You're welcome.
> > > >
> > > > >
> > > > > Have we thought about how guard regions are represented in /proc/*/[s]maps?
> > > >
> > > > This is off-topic here but... Yes, extensively. No they do not appear
> > > > there.
> > > >
> > > > I thought you had attended LPC and my talk where I mentioned this
> > > > purposefully as a drawback?
> > > >
> > > > I went out of my way to advertise this limitation at the LPC talk, in the
> > > > original series, etc. so it's a little disappointing that this is being
> > > > brought up so late, but nobody else has raised objections to this issue so
> > > > I think in general it's not a limitation that matters in practice.
> > > >
>
> Sorry for raising this now, yes at the time I believe we discussed
> reducing the vma slab memory usage for the PROT_NONE mappings. I
> didn't imagine that apps could have dependencies on the mapped ELF
> ranges in /proc/self/[s]maps until recent breakages from a similar
> feature. Android itself doesn't depend on this but what I've seen is
> banking apps and apps that have obfuscation to prevent reverse
> engineering (the particulars of such obfuscation are a black box).
Ack ok fair enough, sorry, but obviously you can understand it's
frustrating when I went to great lengths to advertise this not only at the
talk but in the original series.
Really important to have these discussions early. Not that really we can do
much about this, as inherently this feature cannot give you what you need.
Is it _only_ banking apps that do this? And do they exclusively read
/proc/$pid/maps? I mean there's nothing we can do about that, sorry. If
that's immutable, then unless you do your own very, very, very slow custom
android maps implementation (that will absolutely break the /proc/$pid/maps
scalability efforts atm) this is just a no-go.
>
> > > > >
> > > > > In the field, I've found that many applications read the ranges from
> > > > > /proc/self/[s]maps to determine what they can access (usually related
> > > > > to obfuscation techniques). If they don't know of the guard regions it
> > > > > would cause them to crash; I think that we'll need similar entries to
> > > > > PROT_NONE (---p) for these, and generally to maintain consistency
> > > > > between the behavior and what is being said from /proc/*/[s]maps.
> > > >
> > > > No, we cannot have these, sorry.
> > > >
> > > > Firstly /proc/$pid/[s]maps describes VMAs. The entire purpose of this
> > > > feature is to avoid having to accumulate VMAs for regions which are not
> > > > intended to be accessible.
> > > >
> > > > Secondly, there is no practical means for this to be accomplished in
> > > > /proc/$pid/maps in _any_ way - as no metadata relating to a VMA indicates
> > > > they have guard regions.
> > > >
> > > > This is intentional, because setting such metadata is simply not practical
> > > > - why? Because when you try to split the VMA, how do you know which bit
> > > > gets the metadata and which doesn't? You can't without _reading page
> > > > tables_.
>
> Yeah the splitting becomes complicated with any vm flags for this...
> meaning any attempt to expose this in /proc/*/maps have to
> unconditionally walk the page tables :(
It's not really complicated, it's _impossible_ unless you made literally
all VMA code walk page tables for every single operation. Which we are
emphatically not going to do :)
And no, /proc/$pid/maps is _never_ going to walk page tables. For obvious
performance reasons.
>
> > > >
> > > > /proc/$pid/smaps _does_ read page tables, but we can't start pretending
> > > > VMAs exist when they don't, this would be completely inaccurate, would
> > > > break assumptions for things like mremap (which require a single VMA) and
> > > > would be unworkable.
> > > >
> > > > The best that _could_ be achieved is to have a marker in /proc/$pid/smaps
> > > > saying 'hey this region has guard regions somewhere'.
> > >
> > > And then simply expose it in /proc/$pid/pagemap, which is a better interface
> > > for this pte-level information inside of VMAs. We should still have a spare
> > > bit for that purpose in the pagemap entries.
> >
> > Ah yeah thanks David forgot about that!
> >
> > This is also a possibility if that'd solve your problems Kalesh?
>
> I'm not sure what is the correct interface to advertise these. Maybe
> smaps as you suggested since we already walk the page tables there?
> and pagemap bit for the exact pages as well? It won't solve this
> particular issue, as 1000s of in field apps do look at this through
> /proc/*/maps. But maybe we have to live with that...
I mean why are we even considering this if you can't change this anywhere?
Confused by that.
I'm afraid upstream can't radically change interfaces to suit this
scenario.
We also can't change smaps in the way you want, it _has_ to still give
output per VMA information.
The proposed change that would be there would be a flag or something
indicating that the VMA has guard regions _SOMEWHERE_ in it.
Since this doesn't solve your problem, adds complexity, and nobody else
seems to need it, I would suggest this is not worthwhile and I'd rather not
do this.
Therefore for your needs there are literally only two choices here:
1. Add a bit to /proc/$pid/pagemap OR
2. a new interface.
I am not in favour of a new interface here, if we can just extend pagemap.
What you'd have to do is:
1. Find virtual ranges via /proc/$pid/maps
2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
Since anything that would retrieve guard region state would need to walk
page tables, any approach would be slow and I don't think this would be any
less slow than any other interface.
This way you'd be able to find all guard regions all the time.
This is just the trade-off for this feature unfortunately - its whole
design ethos is to allow modification of -faulting- behaviour without
having to modify -VMA- behaviour.
But if it's banking apps whose code you can't control (surprised you don't
lock down these interfaces), I mean is this even useful to you?
If your requirement is 'you have to change /proc/$pid/maps to show guard
regions' I mean the answer is that we can't.
>
> We can argue that such apps are broken since they may trip on the
> SIGBUS off the end of the file -- usually this isn't the case for the
> ELF segment mappings.
Or tearing of the maps interface, or things getting unmapped or or
or... It's really not a sane thing to do.
>
> This is still useful for other cases, I just wanted to get some ideas
> if this can be extended to further use cases.
Well I'm glad that you guys find it useful for _something_ ;)
Again this wasn't written only for you (it is broadly a good feature for
upstream), but I did have your use case in mind, so I'm a little
disappointed that it doesn't help, as I like to solve problems.
But I'm glad it solves at least some for you...
>
> Thanks,
> Kalesh
>
>
> >
> > This bit will be fought over haha
> >
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb
> > >
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Kalesh Singh @ 2025-02-19 20:56 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <e07dfd31-197c-49d0-92bd-12aad02daa7e@lucifer.local>
On Wed, Feb 19, 2025 at 11:20 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Feb 19, 2025 at 10:52:04AM -0800, Kalesh Singh wrote:
> > On Wed, Feb 19, 2025 at 1:17 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Wed, Feb 19, 2025 at 10:15:47AM +0100, David Hildenbrand wrote:
> > > > On 19.02.25 10:03, Lorenzo Stoakes wrote:
> > > > > On Wed, Feb 19, 2025 at 12:25:51AM -0800, Kalesh Singh wrote:
> > > > > > On Thu, Feb 13, 2025 at 10:18 AM Lorenzo Stoakes
> > > > > > <lorenzo.stoakes@oracle.com> wrote:
> > > > > > >
> > > > > > > The guard regions feature was initially implemented to support anonymous
> > > > > > > mappings only, excluding shmem.
> > > > > > >
> > > > > > > This was done such as to introduce the feature carefully and incrementally
> > > > > > > and to be conservative when considering the various caveats and corner
> > > > > > > cases that are applicable to file-backed mappings but not to anonymous
> > > > > > > ones.
> > > > > > >
> > > > > > > Now this feature has landed in 6.13, it is time to revisit this and to
> > > > > > > extend this functionality to file-backed and shmem mappings.
> > > > > > >
> > > > > > > In order to make this maximally useful, and since one may map file-backed
> > > > > > > mappings read-only (for instance ELF images), we also remove the
> > > > > > > restriction on read-only mappings and permit the establishment of guard
> > > > > > > regions in any non-hugetlb, non-mlock()'d mapping.
> > > > > >
> > > > > > Hi Lorenzo,
> > > > > >
> > > > > > Thank you for your work on this.
> > > > >
> > > > > You're welcome.
> > > > >
> > > > > >
> > > > > > Have we thought about how guard regions are represented in /proc/*/[s]maps?
> > > > >
> > > > > This is off-topic here but... Yes, extensively. No they do not appear
> > > > > there.
> > > > >
> > > > > I thought you had attended LPC and my talk where I mentioned this
> > > > > purposefully as a drawback?
> > > > >
> > > > > I went out of my way to advertise this limitation at the LPC talk, in the
> > > > > original series, etc. so it's a little disappointing that this is being
> > > > > brought up so late, but nobody else has raised objections to this issue so
> > > > > I think in general it's not a limitation that matters in practice.
> > > > >
> >
> > Sorry for raising this now, yes at the time I believe we discussed
> > reducing the vma slab memory usage for the PROT_NONE mappings. I
> > didn't imagine that apps could have dependencies on the mapped ELF
> > ranges in /proc/self/[s]maps until recent breakages from a similar
> > feature. Android itself doesn't depend on this but what I've seen is
> > banking apps and apps that have obfuscation to prevent reverse
> > engineering (the particulars of such obfuscation are a black box).
>
> Ack ok fair enough, sorry, but obviously you can understand it's
> frustrating when I went to great lengths to advertise this not only at the
> talk but in the original series.
>
> Really important to have these discussions early. Not that really we can do
> much about this, as inherently this feature cannot give you what you need.
>
> Is it _only_ banking apps that do this? And do they exclusively read
> /proc/$pid/maps? I mean there's nothing we can do about that, sorry.
Not only banking apps but that's a common category.
> If that's immutable, then unless you do your own very, very, very slow custom
> android maps implementation (that will absolutely break the /proc/$pid/maps
> scalability efforts atm) this is just a no-go.
>
Yeah unfortunately that's immutable as app versions are mostly
independent from the OS version.
We do have something that handles this by encoding the guard regions
in the vm_flags, but as you can imagine it's not generic enough for
upstream.
> >
> > > > > >
> > > > > > In the field, I've found that many applications read the ranges from
> > > > > > /proc/self/[s]maps to determine what they can access (usually related
> > > > > > to obfuscation techniques). If they don't know of the guard regions it
> > > > > > would cause them to crash; I think that we'll need similar entries to
> > > > > > PROT_NONE (---p) for these, and generally to maintain consistency
> > > > > > between the behavior and what is being said from /proc/*/[s]maps.
> > > > >
> > > > > No, we cannot have these, sorry.
> > > > >
> > > > > Firstly /proc/$pid/[s]maps describes VMAs. The entire purpose of this
> > > > > feature is to avoid having to accumulate VMAs for regions which are not
> > > > > intended to be accessible.
> > > > >
> > > > > Secondly, there is no practical means for this to be accomplished in
> > > > > /proc/$pid/maps in _any_ way - as no metadata relating to a VMA indicates
> > > > > they have guard regions.
> > > > >
> > > > > This is intentional, because setting such metadata is simply not practical
> > > > > - why? Because when you try to split the VMA, how do you know which bit
> > > > > gets the metadata and which doesn't? You can't without _reading page
> > > > > tables_.
> >
> > Yeah the splitting becomes complicated with any vm flags for this...
> > meaning any attempt to expose this in /proc/*/maps have to
> > unconditionally walk the page tables :(
>
> It's not really complicated, it's _impossible_ unless you made literally
> all VMA code walk page tables for every single operation. Which we are
> emphatically not going to do :)
>
> And no, /proc/$pid/maps is _never_ going to walk page tables. For obvious
> performance reasons.
>
> >
> > > > >
> > > > > /proc/$pid/smaps _does_ read page tables, but we can't start pretending
> > > > > VMAs exist when they don't, this would be completely inaccurate, would
> > > > > break assumptions for things like mremap (which require a single VMA) and
> > > > > would be unworkable.
> > > > >
> > > > > The best that _could_ be achieved is to have a marker in /proc/$pid/smaps
> > > > > saying 'hey this region has guard regions somewhere'.
> > > >
> > > > And then simply expose it in /proc/$pid/pagemap, which is a better interface
> > > > for this pte-level information inside of VMAs. We should still have a spare
> > > > bit for that purpose in the pagemap entries.
> > >
> > > Ah yeah thanks David forgot about that!
> > >
> > > This is also a possibility if that'd solve your problems Kalesh?
> >
> > I'm not sure what is the correct interface to advertise these. Maybe
> > smaps as you suggested since we already walk the page tables there?
> > and pagemap bit for the exact pages as well? It won't solve this
> > particular issue, as 1000s of in field apps do look at this through
> > /proc/*/maps. But maybe we have to live with that...
>
> I mean why are we even considering this if you can't change this anywhere?
> Confused by that.
>
> I'm afraid upstream can't radically change interfaces to suit this
> scenario.
>
> We also can't change smaps in the way you want, it _has_ to still give
> output per VMA information.
Sorry I wasn't suggesting to change the entries in smaps, rather
agreeing to your marker suggestion. Maybe a set of ranges for each
smaps entry that has guards? It doesn't solve the use case, but does
make these regions visible to userspace.
>
> The proposed change that would be there would be a flag or something
> indicating that the VMA has guard regions _SOMEWHERE_ in it.
>
> Since this doesn't solve your problem, adds complexity, and nobody else
> seems to need it, I would suggest this is not worthwhile and I'd rather not
> do this.
>
> Therefore for your needs there are literally only two choices here:
>
> 1. Add a bit to /proc/$pid/pagemap OR
> 2. a new interface.
>
> I am not in favour of a new interface here, if we can just extend pagemap.
>
> What you'd have to do is:
>
> 1. Find virtual ranges via /proc/$pid/maps
> 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
>
Could we also consider an smaps field like:
VmGuards: [AAA, BBB), [CCC, DDD), ...
or something of that sort?
> Since anything that would retrieve guard region state would need to walk
> page tables, any approach would be slow and I don't think this would be any
> less slow than any other interface.
>
> This way you'd be able to find all guard regions all the time.
>
> This is just the trade-off for this feature unfortunately - its whole
> design ethos is to allow modification of -faulting- behaviour without
> having to modify -VMA- behaviour.
>
> But if it's banking apps whose code you can't control (surprised you don't
> lock down these interfaces), I mean is this even useful to you?
>
> If your requirement is 'you have to change /proc/$pid/maps to show guard
> regions' I mean the answer is that we can't.
>
> >
> > We can argue that such apps are broken since they may trip on the
> > SIGBUS off the end of the file -- usually this isn't the case for the
> > ELF segment mappings.
>
> Or tearing of the maps interface, or things getting unmapped or or
> or... It's really not a sane thing to do.
>
> >
> > This is still useful for other cases, I just wanted to get some ideas
> > if this can be extended to further use cases.
>
> Well I'm glad that you guys find it useful for _something_ ;)
>
> Again this wasn't written only for you (it is broadly a good feature for
> upstream), but I did have your use case in mind, so I'm a little
> disappointed that it doesn't help, as I like to solve problems.
>
> But I'm glad it solves at least some for you...
I recall Liam had a proposal to store the guard ranges in the maple tree?
I wonder if that can be used in combination with this approach to have
a better representation of this?
>
> >
> > Thanks,
> > Kalesh
> >
> >
> > >
> > > This bit will be fought over haha
> > >
> > > >
> > > > --
> > > > Cheers,
> > > >
> > > > David / dhildenb
> > > >
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 8:51 UTC (permalink / raw)
To: Kalesh Singh
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <CAC_TJvfBvZZc=xyB0jez2VCDit-rettfQf7H4xhQbN7bYxKw-A@mail.gmail.com>
On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
> > We also can't change smaps in the way you want, it _has_ to still give
> > output per VMA information.
>
> Sorry I wasn't suggesting to change the entries in smaps, rather
> agreeing to your marker suggestion. Maybe a set of ranges for each
> smaps entry that has guards? It doesn't solve the use case, but does
> make these regions visible to userspace.
No, you are not providing a usecase for this. /proc/$pid/pagemaps does not
contaminate the smaps output, mess with efforts to make it RCU readable,
require updating the ioctl interface, etc. so it is clearly the better
choice.
>
> >
> > The proposed change that would be there would be a flag or something
> > indicating that the VMA has guard regions _SOMEWHERE_ in it.
> >
> > Since this doesn't solve your problem, adds complexity, and nobody else
> > seems to need it, I would suggest this is not worthwhile and I'd rather not
> > do this.
> >
> > Therefore for your needs there are literally only two choices here:
> >
> > 1. Add a bit to /proc/$pid/pagemap OR
> > 2. a new interface.
> >
> > I am not in favour of a new interface here, if we can just extend pagemap.
> >
> > What you'd have to do is:
> >
> > 1. Find virtual ranges via /proc/$pid/maps
> > 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
> >
>
> Could we also consider an smaps field like:
>
> VmGuards: [AAA, BBB), [CCC, DDD), ...
>
> or something of that sort?
No, absolutely, categorically not. You realise these could be thousands of
characters long right?
/proc/$pid/pagemaps resolves this without contaminating this output.
> > Well I'm glad that you guys find it useful for _something_ ;)
> >
> > Again this wasn't written only for you (it is broadly a good feature for
> > upstream), but I did have your use case in mind, so I'm a little
> > disappointed that it doesn't help, as I like to solve problems.
> >
> > But I'm glad it solves at least some for you...
>
> I recall Liam had a proposal to store the guard ranges in the maple tree?
>
> I wonder if that can be used in combination with this approach to have
> a better representation of this?
This was an alternative proposal made prior to the feature being
implemented (and you and others at Google were welcome to comment and many
were cc'd, etc.).
There is no 'in combination with'. This feature would take weeks/months to
implement, fundamentally impact the maple tree VMA implementation
and... not actually achieve anything + immediately be redundant.
Plus it'd likely be slower, have locking implications, would have kernel
memory allocation implications, a lot more complexity and probably other
problems besides (we discussed this at length at the time and a number of
issues came up, I can't recall all of them).
To be crystal clear - we are empathically NOT changing /proc/$pid/maps to
lie about VMAs regardless of underlying implementation, nor adding
thousands of characters to /proc/$pid/smaps entries.
This is independent of implementation and would have been the case had we
gone with a maple node version.
So in no world is your problem solved here, unfortunately you have
inextricably tied yourself to a VMA representation here.
I still wonder if you could find some means of abstracting this, but
/proc/$pid/pagemaps is where I am likely to expose this information for
anybody who needs it, and will likely send a series for this relatively
soon.
If you _can_ abstract this in some way, then if we provide this information
_anywhere_ you can get it.
As I said to you earlier, the _best_ we could do in smaps would be to add a
flag like 'Grd' or something to indicate some part of the VMA is
guarded. But I won't do that unless somebody has an -actual use case- for
it.
David's /proc/$pid/pagemaps suggestion is excellent, avoids all the
pitfalls, exposes guard regions to anybody who really really wants to know
and doesn't interfere with anything else, so this is what we'll go with.
Regards, Lorenzo
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-20 8:57 UTC (permalink / raw)
To: Lorenzo Stoakes, Kalesh Singh
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <6e356431-5ac9-4363-b876-78a69ae7622a@lucifer.local>
On 20.02.25 09:51, Lorenzo Stoakes wrote:
> On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
>>> We also can't change smaps in the way you want, it _has_ to still give
>>> output per VMA information.
>>
>> Sorry I wasn't suggesting to change the entries in smaps, rather
>> agreeing to your marker suggestion. Maybe a set of ranges for each
>> smaps entry that has guards? It doesn't solve the use case, but does
>> make these regions visible to userspace.
>
> No, you are not providing a usecase for this. /proc/$pid/pagemaps does not
> contaminate the smaps output, mess with efforts to make it RCU readable,
> require updating the ioctl interface, etc. so it is clearly the better
> choice.
>
>>
>>>
>>> The proposed change that would be there would be a flag or something
>>> indicating that the VMA has guard regions _SOMEWHERE_ in it.
>>>
>>> Since this doesn't solve your problem, adds complexity, and nobody else
>>> seems to need it, I would suggest this is not worthwhile and I'd rather not
>>> do this.
>>>
>>> Therefore for your needs there are literally only two choices here:
>>>
>>> 1. Add a bit to /proc/$pid/pagemap OR
>>> 2. a new interface.
>>>
>>> I am not in favour of a new interface here, if we can just extend pagemap.
>>>
>>> What you'd have to do is:
>>>
>>> 1. Find virtual ranges via /proc/$pid/maps
>>> 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
>>>
>>
>> Could we also consider an smaps field like:
>>
>> VmGuards: [AAA, BBB), [CCC, DDD), ...
>>
>> or something of that sort?
>
> No, absolutely, categorically not. You realise these could be thousands of
> characters long right?
>
> /proc/$pid/pagemaps resolves this without contaminating this output.
>
>>> Well I'm glad that you guys find it useful for _something_ ;)
>>>
>>> Again this wasn't written only for you (it is broadly a good feature for
>>> upstream), but I did have your use case in mind, so I'm a little
>>> disappointed that it doesn't help, as I like to solve problems.
>>>
>>> But I'm glad it solves at least some for you...
>>
>> I recall Liam had a proposal to store the guard ranges in the maple tree?
>>
>> I wonder if that can be used in combination with this approach to have
>> a better representation of this?
>
> This was an alternative proposal made prior to the feature being
> implemented (and you and others at Google were welcome to comment and many
> were cc'd, etc.).
>
> There is no 'in combination with'. This feature would take weeks/months to
> implement, fundamentally impact the maple tree VMA implementation
> and... not actually achieve anything + immediately be redundant.
>
> Plus it'd likely be slower, have locking implications, would have kernel
> memory allocation implications, a lot more complexity and probably other
> problems besides (we discussed this at length at the time and a number of
> issues came up, I can't recall all of them).
>
> To be crystal clear - we are empathically NOT changing /proc/$pid/maps to
> lie about VMAs regardless of underlying implementation, nor adding
> thousands of characters to /proc/$pid/smaps entries.
Yes. Calling it a "guard region" might be part of the problem
(/"misunderstanding"), because it reminds people of "virtual memory
regions".
"Guard markers" or similar might have been clearer that these operate on
individual PTEs, require page table scanning etc ... which makes them a
lot more scalable and fine-grained and provides all these benfits, with
the downside being that we don't end up with that many "virtual memory
regions" that maps/smaps operate on.
[...]
>
> As I said to you earlier, the _best_ we could do in smaps would be to add a
> flag like 'Grd' or something to indicate some part of the VMA is
> guarded. But I won't do that unless somebody has an -actual use case- for
> it.
Right, and that would limit where you have to manually scan. Something
similar is being done with uffd-wp markers IIRC.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 9:04 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <4aa97b5c-3ddc-442b-8ec9-cc43ebe9e599@redhat.com>
On Thu, Feb 20, 2025 at 09:57:37AM +0100, David Hildenbrand wrote:
> On 20.02.25 09:51, Lorenzo Stoakes wrote:
> > On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
> > > > We also can't change smaps in the way you want, it _has_ to still give
> > > > output per VMA information.
> > >
> > > Sorry I wasn't suggesting to change the entries in smaps, rather
> > > agreeing to your marker suggestion. Maybe a set of ranges for each
> > > smaps entry that has guards? It doesn't solve the use case, but does
> > > make these regions visible to userspace.
> >
> > No, you are not providing a usecase for this. /proc/$pid/pagemaps does not
> > contaminate the smaps output, mess with efforts to make it RCU readable,
> > require updating the ioctl interface, etc. so it is clearly the better
> > choice.
> >
> > >
> > > >
> > > > The proposed change that would be there would be a flag or something
> > > > indicating that the VMA has guard regions _SOMEWHERE_ in it.
> > > >
> > > > Since this doesn't solve your problem, adds complexity, and nobody else
> > > > seems to need it, I would suggest this is not worthwhile and I'd rather not
> > > > do this.
> > > >
> > > > Therefore for your needs there are literally only two choices here:
> > > >
> > > > 1. Add a bit to /proc/$pid/pagemap OR
> > > > 2. a new interface.
> > > >
> > > > I am not in favour of a new interface here, if we can just extend pagemap.
> > > >
> > > > What you'd have to do is:
> > > >
> > > > 1. Find virtual ranges via /proc/$pid/maps
> > > > 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
> > > >
> > >
> > > Could we also consider an smaps field like:
> > >
> > > VmGuards: [AAA, BBB), [CCC, DDD), ...
> > >
> > > or something of that sort?
> >
> > No, absolutely, categorically not. You realise these could be thousands of
> > characters long right?
> >
> > /proc/$pid/pagemaps resolves this without contaminating this output.
> >
> > > > Well I'm glad that you guys find it useful for _something_ ;)
> > > >
> > > > Again this wasn't written only for you (it is broadly a good feature for
> > > > upstream), but I did have your use case in mind, so I'm a little
> > > > disappointed that it doesn't help, as I like to solve problems.
> > > >
> > > > But I'm glad it solves at least some for you...
> > >
> > > I recall Liam had a proposal to store the guard ranges in the maple tree?
> > >
> > > I wonder if that can be used in combination with this approach to have
> > > a better representation of this?
> >
> > This was an alternative proposal made prior to the feature being
> > implemented (and you and others at Google were welcome to comment and many
> > were cc'd, etc.).
> >
> > There is no 'in combination with'. This feature would take weeks/months to
> > implement, fundamentally impact the maple tree VMA implementation
> > and... not actually achieve anything + immediately be redundant.
> >
> > Plus it'd likely be slower, have locking implications, would have kernel
> > memory allocation implications, a lot more complexity and probably other
> > problems besides (we discussed this at length at the time and a number of
> > issues came up, I can't recall all of them).
> >
> > To be crystal clear - we are empathically NOT changing /proc/$pid/maps to
> > lie about VMAs regardless of underlying implementation, nor adding
> > thousands of characters to /proc/$pid/smaps entries.
>
> Yes. Calling it a "guard region" might be part of the problem
> (/"misunderstanding"), because it reminds people of "virtual memory
> regions".
>
> "Guard markers" or similar might have been clearer that these operate on
> individual PTEs, require page table scanning etc ... which makes them a lot
> more scalable and fine-grained and provides all these benfits, with the
> downside being that we don't end up with that many "virtual memory regions"
> that maps/smaps operate on.
Honestly David you and the naming... :P
I disagree, sorry. Saying 'guard' anything might make people think one
thing or another. We can't account for that. I mean don't get me started on
'pinning' or any of the million other overloaded terms we use...
I _hugely_ publicly went out of my way to express the limitations, I gave a
talk, we had meetings, I mentioned it in the series.
Honestly if at that point you still don't realise, that's not a naming
problem. It's a 'did not participate with upstream' problem.
I like guard regions, as they're not pages as we previously referred to
them. People have no idea what a marker is, it doesn't sound like it spans
ranges, no don't like it sorry.
And sorry but this naming topic is closed :) I already let you change the
naming of the MADV_'s, which broke my heart, there will not be a second
heart breaking...
>
> [...]
>
> >
> > As I said to you earlier, the _best_ we could do in smaps would be to add a
> > flag like 'Grd' or something to indicate some part of the VMA is
> > guarded. But I won't do that unless somebody has an -actual use case- for
> > it.
>
> Right, and that would limit where you have to manually scan. Something
> similar is being done with uffd-wp markers IIRC.
Yeah that's a good point, but honestly if you're reading smaps that reads
the page tables, then reading /proc/$pid/pagemaps and reading page tables
TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
/proc/$pid/pagemaps and reading page tables once.
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Vlastimil Babka @ 2025-02-20 9:22 UTC (permalink / raw)
To: Lorenzo Stoakes, Kalesh Singh
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <6e356431-5ac9-4363-b876-78a69ae7622a@lucifer.local>
On 2/20/25 09:51, Lorenzo Stoakes wrote:
> On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
>
> As I said to you earlier, the _best_ we could do in smaps would be to add a
> flag like 'Grd' or something to indicate some part of the VMA is
In smaps we could say how many kB is covered by guard ptes and it would be
in line with the current output, and the fact it's already scanning page
tables, so virtually no new overhead. But it wouldn't tell you the address
ranges, of course.
> guarded. But I won't do that unless somebody has an -actual use case- for
> it.
Right, we need to hear that first. Also I'm a bit confused about what's the
issue with the existing Android apps and the proposals. Do the existing apps
expect to see particular PROT_NONE regions, which would be gone (become part
of the surrounding vma's) when using guards instead? Then clearly there's no
way to use guards for them without breaking their assumptions.
But assuming future versions of these apps for future android versions would
have to adapt to guards instead of PROT_NONE, why do we have to promise them
to represent the guards, if the point is just so they adapt to the new state
of smaps in their dubious security checking code, and not break anymore?
(but geez, if android apps are to use the android apis, which is java based
(IIRC?) why were there ever allowed to read /proc in the first place? such a
mistake, sigh)
> David's /proc/$pid/pagemaps suggestion is excellent, avoids all the
> pitfalls, exposes guard regions to anybody who really really wants to know
> and doesn't interfere with anything else, so this is what we'll go with.
>
> Regards, Lorenzo
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-20 9:23 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <b0a95f2c-093c-45fd-b4a2-2ba5cbc37e2c@lucifer.local>
On 20.02.25 10:04, Lorenzo Stoakes wrote:
> On Thu, Feb 20, 2025 at 09:57:37AM +0100, David Hildenbrand wrote:
>> On 20.02.25 09:51, Lorenzo Stoakes wrote:
>>> On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
>>>>> We also can't change smaps in the way you want, it _has_ to still give
>>>>> output per VMA information.
>>>>
>>>> Sorry I wasn't suggesting to change the entries in smaps, rather
>>>> agreeing to your marker suggestion. Maybe a set of ranges for each
>>>> smaps entry that has guards? It doesn't solve the use case, but does
>>>> make these regions visible to userspace.
>>>
>>> No, you are not providing a usecase for this. /proc/$pid/pagemaps does not
>>> contaminate the smaps output, mess with efforts to make it RCU readable,
>>> require updating the ioctl interface, etc. so it is clearly the better
>>> choice.
>>>
>>>>
>>>>>
>>>>> The proposed change that would be there would be a flag or something
>>>>> indicating that the VMA has guard regions _SOMEWHERE_ in it.
>>>>>
>>>>> Since this doesn't solve your problem, adds complexity, and nobody else
>>>>> seems to need it, I would suggest this is not worthwhile and I'd rather not
>>>>> do this.
>>>>>
>>>>> Therefore for your needs there are literally only two choices here:
>>>>>
>>>>> 1. Add a bit to /proc/$pid/pagemap OR
>>>>> 2. a new interface.
>>>>>
>>>>> I am not in favour of a new interface here, if we can just extend pagemap.
>>>>>
>>>>> What you'd have to do is:
>>>>>
>>>>> 1. Find virtual ranges via /proc/$pid/maps
>>>>> 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
>>>>>
>>>>
>>>> Could we also consider an smaps field like:
>>>>
>>>> VmGuards: [AAA, BBB), [CCC, DDD), ...
>>>>
>>>> or something of that sort?
>>>
>>> No, absolutely, categorically not. You realise these could be thousands of
>>> characters long right?
>>>
>>> /proc/$pid/pagemaps resolves this without contaminating this output.
>>>
>>>>> Well I'm glad that you guys find it useful for _something_ ;)
>>>>>
>>>>> Again this wasn't written only for you (it is broadly a good feature for
>>>>> upstream), but I did have your use case in mind, so I'm a little
>>>>> disappointed that it doesn't help, as I like to solve problems.
>>>>>
>>>>> But I'm glad it solves at least some for you...
>>>>
>>>> I recall Liam had a proposal to store the guard ranges in the maple tree?
>>>>
>>>> I wonder if that can be used in combination with this approach to have
>>>> a better representation of this?
>>>
>>> This was an alternative proposal made prior to the feature being
>>> implemented (and you and others at Google were welcome to comment and many
>>> were cc'd, etc.).
>>>
>>> There is no 'in combination with'. This feature would take weeks/months to
>>> implement, fundamentally impact the maple tree VMA implementation
>>> and... not actually achieve anything + immediately be redundant.
>>>
>>> Plus it'd likely be slower, have locking implications, would have kernel
>>> memory allocation implications, a lot more complexity and probably other
>>> problems besides (we discussed this at length at the time and a number of
>>> issues came up, I can't recall all of them).
>>>
>>> To be crystal clear - we are empathically NOT changing /proc/$pid/maps to
>>> lie about VMAs regardless of underlying implementation, nor adding
>>> thousands of characters to /proc/$pid/smaps entries.
>>
>> Yes. Calling it a "guard region" might be part of the problem
>> (/"misunderstanding"), because it reminds people of "virtual memory
>> regions".
>>
>> "Guard markers" or similar might have been clearer that these operate on
>> individual PTEs, require page table scanning etc ... which makes them a lot
>> more scalable and fine-grained and provides all these benfits, with the
>> downside being that we don't end up with that many "virtual memory regions"
>> that maps/smaps operate on.
>
> Honestly David you and the naming... :P
>
> I disagree, sorry. Saying 'guard' anything might make people think
one> thing or another. We can't account for that. I mean don't get me
started on
> 'pinning' or any of the million other overloaded terms we use...
>
> I _hugely_ publicly went out of my way to express the limitations, I
gave a> talk, we had meetings, I mentioned it in the series.
>
> Honestly if at that point you still don't realise, that's not a naming
> problem. It's a 'did not participate with upstream' problem.
>
> I like guard regions, as they're not pages as we previously referred to
> them. People have no idea what a marker is, it doesn't sound like it spans
> ranges, no don't like it sorry.
>
> And sorry but this naming topic is closed :) I already let you change the
> naming of the MADV_'s, which broke my heart, there will not be a second
> heart breaking...
Lorenzo, I was not pushing for it to be changed *now*, that ship has
sailed, and personally, I *don't* find it confusing because I know how
it works under the hood.
I was trying to find a reason *why* people would thing that it would
show up in smaps in the first place. For example, just when reading the
MAN page *today*.
Doesn't really matter now, it is named the way it is, and all we can do
is try making documentation clearer if it keeps confusing people.
Your conclusion is 'did not participate with upstream'; I don't agree
with that. But maybe you and Kalesh have a history on that that let's
you react on his questions IMHO more emotionally than it should have been.
>
>>
>> [...]
>>
>>>
>>> As I said to you earlier, the _best_ we could do in smaps would be to add a
>>> flag like 'Grd' or something to indicate some part of the VMA is
>>> guarded. But I won't do that unless somebody has an -actual use case- for
>>> it.
>>
>> Right, and that would limit where you have to manually scan. Something
>> similar is being done with uffd-wp markers IIRC.
>
> Yeah that's a good point, but honestly if you're reading smaps that reads
> the page tables, then reading /proc/$pid/pagemaps and reading page tables
> TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> /proc/$pid/pagemaps and reading page tables once.
Right; I recently wished that we would have an interface to obtain more
VMA flags without having to go through smaps
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 9:47 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <387f3516-99f2-41e9-967e-4b051a8d21b8@redhat.com>
On Thu, Feb 20, 2025 at 10:23:57AM +0100, David Hildenbrand wrote:
> On 20.02.25 10:04, Lorenzo Stoakes wrote:
> > On Thu, Feb 20, 2025 at 09:57:37AM +0100, David Hildenbrand wrote:
> > > On 20.02.25 09:51, Lorenzo Stoakes wrote:
> > > > On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
> > > > > > We also can't change smaps in the way you want, it _has_ to still give
> > > > > > output per VMA information.
> > > > >
> > > > > Sorry I wasn't suggesting to change the entries in smaps, rather
> > > > > agreeing to your marker suggestion. Maybe a set of ranges for each
> > > > > smaps entry that has guards? It doesn't solve the use case, but does
> > > > > make these regions visible to userspace.
> > > >
> > > > No, you are not providing a usecase for this. /proc/$pid/pagemaps does not
> > > > contaminate the smaps output, mess with efforts to make it RCU readable,
> > > > require updating the ioctl interface, etc. so it is clearly the better
> > > > choice.
> > > >
> > > > >
> > > > > >
> > > > > > The proposed change that would be there would be a flag or something
> > > > > > indicating that the VMA has guard regions _SOMEWHERE_ in it.
> > > > > >
> > > > > > Since this doesn't solve your problem, adds complexity, and nobody else
> > > > > > seems to need it, I would suggest this is not worthwhile and I'd rather not
> > > > > > do this.
> > > > > >
> > > > > > Therefore for your needs there are literally only two choices here:
> > > > > >
> > > > > > 1. Add a bit to /proc/$pid/pagemap OR
> > > > > > 2. a new interface.
> > > > > >
> > > > > > I am not in favour of a new interface here, if we can just extend pagemap.
> > > > > >
> > > > > > What you'd have to do is:
> > > > > >
> > > > > > 1. Find virtual ranges via /proc/$pid/maps
> > > > > > 2. iterate through /proc/$pid/pagemaps to retrieve state for all ranges.
> > > > > >
> > > > >
> > > > > Could we also consider an smaps field like:
> > > > >
> > > > > VmGuards: [AAA, BBB), [CCC, DDD), ...
> > > > >
> > > > > or something of that sort?
> > > >
> > > > No, absolutely, categorically not. You realise these could be thousands of
> > > > characters long right?
> > > >
> > > > /proc/$pid/pagemaps resolves this without contaminating this output.
> > > >
> > > > > > Well I'm glad that you guys find it useful for _something_ ;)
> > > > > >
> > > > > > Again this wasn't written only for you (it is broadly a good feature for
> > > > > > upstream), but I did have your use case in mind, so I'm a little
> > > > > > disappointed that it doesn't help, as I like to solve problems.
> > > > > >
> > > > > > But I'm glad it solves at least some for you...
> > > > >
> > > > > I recall Liam had a proposal to store the guard ranges in the maple tree?
> > > > >
> > > > > I wonder if that can be used in combination with this approach to have
> > > > > a better representation of this?
> > > >
> > > > This was an alternative proposal made prior to the feature being
> > > > implemented (and you and others at Google were welcome to comment and many
> > > > were cc'd, etc.).
> > > >
> > > > There is no 'in combination with'. This feature would take weeks/months to
> > > > implement, fundamentally impact the maple tree VMA implementation
> > > > and... not actually achieve anything + immediately be redundant.
> > > >
> > > > Plus it'd likely be slower, have locking implications, would have kernel
> > > > memory allocation implications, a lot more complexity and probably other
> > > > problems besides (we discussed this at length at the time and a number of
> > > > issues came up, I can't recall all of them).
> > > >
> > > > To be crystal clear - we are empathically NOT changing /proc/$pid/maps to
> > > > lie about VMAs regardless of underlying implementation, nor adding
> > > > thousands of characters to /proc/$pid/smaps entries.
> > >
> > > Yes. Calling it a "guard region" might be part of the problem
> > > (/"misunderstanding"), because it reminds people of "virtual memory
> > > regions".
> > >
> > > "Guard markers" or similar might have been clearer that these operate on
> > > individual PTEs, require page table scanning etc ... which makes them a lot
> > > more scalable and fine-grained and provides all these benfits, with the
> > > downside being that we don't end up with that many "virtual memory regions"
> > > that maps/smaps operate on.
> >
> > Honestly David you and the naming... :P
> >
> > I disagree, sorry. Saying 'guard' anything might make people think one>
> thing or another. We can't account for that. I mean don't get me started on
> > 'pinning' or any of the million other overloaded terms we use...
> >
> > I _hugely_ publicly went out of my way to express the limitations, I gave
> a> talk, we had meetings, I mentioned it in the series.
> >
> > Honestly if at that point you still don't realise, that's not a naming
> > problem. It's a 'did not participate with upstream' problem.
> >
> > I like guard regions, as they're not pages as we previously referred to
> > them. People have no idea what a marker is, it doesn't sound like it spans
> > ranges, no don't like it sorry.
> >
> > And sorry but this naming topic is closed :) I already let you change the
> > naming of the MADV_'s, which broke my heart, there will not be a second
> > heart breaking...
>
> Lorenzo, I was not pushing for it to be changed *now*, that ship has sailed,
> and personally, I *don't* find it confusing because I know how it works
> under the hood.
>
> I was trying to find a reason *why* people would thing that it would show up
> in smaps in the first place. For example, just when reading the MAN page
> *today*.
>
> Doesn't really matter now, it is named the way it is, and all we can do is
> try making documentation clearer if it keeps confusing people.
Right, but I disagree with your analysis. In case as you say, it doesn't
really matter at this stage.
>
> Your conclusion is 'did not participate with upstream'; I don't agree with
> that. But maybe you and Kalesh have a history on that that let's you react
> on his questions IMHO more emotionally than it should have been.
This is wholly unfair, I have been very reasonable in response to this
thread. I have offered to find solutions, I have tried to understand the
problem in spite of having gone to great lengths to try to discuss the
limitations of the proposed approach in every venue I possibly could.
I go out of my way to deal professionally and objectively with what is
presented. Nothing here is emotional. So I'd ask that you please abstain
from making commentary like this which has no basis.
My point about not participating with upstream is that Kalesh is now asking
for an -entirely different- approach to guard regions than was implemented,
months after it was merged.
This is after many discussion were had including with people at Google
among other organisations, an RFC which clearly delineated the limitations,
a talk at LPC also.
I feel I communite these things better than many people actually, I go out
of my way to document, add extensive self-documenting tests, etc. etc. and
to participate, engage and take onboard feedback from others.
So I'm suggesting that clearly - something broke down here. There was a
miscommunication, or there was a lack of awareness of a key requirement.
I mean a large motivator for file-backed support here came from the LPC
talk give by Kalesh and Juan re: ELF guard regions. So obviously that this
breakdown in communication with upstream occurred is very unfortunate.
I am not blaming anybody or being 'emotional'. I am simply stating what
seems to me to be a clear fact.
I genuinely don't understand how it could be seen any other way? How can
requesting that an entirely different alternative approach months after the
fact be anything other than some failure to engage with upstream?
I am emphatically _not_ blaming Kalesh by the way, whom I respect and with
whom I have no problem, in any way whatsoever. I apologised when I realised
that he simply was not aware of this limitation at LPC if you look through
the thread, having politely suggested disappointment at this not having
been brought up then.
I am suggesting that within Google clearly there has been some form of
miscommunication or failure for one aspect of things (this limitation) to
be expressed at the right time to engage with upstream.
Sorry if you misinterpreted that as something else, this is the _only_
point I am making here.
And again, I am going out of my way to find practical and helpful ways
forward - though I cannot see how we can possibly fulfil Kalesh's needs
here.
>
>
> >
> > >
> > > [...]
> > >
> > > >
> > > > As I said to you earlier, the _best_ we could do in smaps would be to add a
> > > > flag like 'Grd' or something to indicate some part of the VMA is
> > > > guarded. But I won't do that unless somebody has an -actual use case- for
> > > > it.
> > >
> > > Right, and that would limit where you have to manually scan. Something
> > > similar is being done with uffd-wp markers IIRC.
> >
> > Yeah that's a good point, but honestly if you're reading smaps that reads
> > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > /proc/$pid/pagemaps and reading page tables once.
>
> Right; I recently wished that we would have an interface to obtain more VMA
> flags without having to go through smaps
Well maybe that lends itself to the idea of adding a whole new interface in
general...
>
> --
> Cheers,
>
> David / dhildenb
>
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 9:53 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Kalesh Singh, David Hildenbrand, Andrew Morton,
Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <5fe78422-f348-414c-9ff2-78812ba9a949@suse.cz>
On Thu, Feb 20, 2025 at 10:22:29AM +0100, Vlastimil Babka wrote:
> On 2/20/25 09:51, Lorenzo Stoakes wrote:
> > On Wed, Feb 19, 2025 at 12:56:31PM -0800, Kalesh Singh wrote:
> >
> > As I said to you earlier, the _best_ we could do in smaps would be to add a
> > flag like 'Grd' or something to indicate some part of the VMA is
>
> In smaps we could say how many kB is covered by guard ptes and it would be
> in line with the current output, and the fact it's already scanning page
> tables, so virtually no new overhead. But it wouldn't tell you the address
> ranges, of course.
Yes right. Sorry, I didn't think to suggest this, the fundamental point
being that there is essentially a 'flag' or 'entry' saying 'there are
guard regions here'. And indeed that could include ranges.
But it doesn't tell you the actual address ranges which is what Kalesh
appears to require.
You could equally well find these, and the sizes (albeit less conveniently)
with a combination of /proc/$pid/maps and adding a guard bit to
/proc/$pid/pagemap.
>
> > guarded. But I won't do that unless somebody has an -actual use case- for
> > it.
>
> Right, we need to hear that first. Also I'm a bit confused about what's the
> issue with the existing Android apps and the proposals. Do the existing apps
> expect to see particular PROT_NONE regions, which would be gone (become part
> of the surrounding vma's) when using guards instead? Then clearly there's no
> way to use guards for them without breaking their assumptions.
>
> But assuming future versions of these apps for future android versions would
> have to adapt to guards instead of PROT_NONE, why do we have to promise them
> to represent the guards, if the point is just so they adapt to the new state
> of smaps in their dubious security checking code, and not break anymore?
>
> (but geez, if android apps are to use the android apis, which is java based
> (IIRC?) why were there ever allowed to read /proc in the first place? such a
> mistake, sigh)
>
> > David's /proc/$pid/pagemaps suggestion is excellent, avoids all the
> > pitfalls, exposes guard regions to anybody who really really wants to know
> > and doesn't interfere with anything else, so this is what we'll go with.
> >
> > Regards, Lorenzo
>
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-20 10:03 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <72e044ba-64af-49c0-8b87-ead508654fb7@lucifer.local>
>> Your conclusion is 'did not participate with upstream'; I don't agree with
>> that. But maybe you and Kalesh have a history on that that let's you react
>> on his questions IMHO more emotionally than it should have been.
>
> This is wholly unfair, I have been very reasonable in response to this
> thread. I have offered to find solutions, I have tried to understand the
> problem in spite of having gone to great lengths to try to discuss the
> limitations of the proposed approach in every venue I possibly could.
>
> I go out of my way to deal professionally and objectively with what is
> presented. Nothing here is emotional. So I'd ask that you please abstain
> from making commentary like this which has no basis.
I appreciate everything you write below. But this request is just
impossible. I will keep raising my opinion and misunderstandings will
happen.
Note that the whole "Honestly David you and the naming. .." thing could
have been written as "I don't think it's a naming problem."
>>
>>
>>>
>>>>
>>>> [...]
>>>>
>>>>>
>>>>> As I said to you earlier, the _best_ we could do in smaps would be to add a
>>>>> flag like 'Grd' or something to indicate some part of the VMA is
>>>>> guarded. But I won't do that unless somebody has an -actual use case- for
>>>>> it.
>>>>
>>>> Right, and that would limit where you have to manually scan. Something
>>>> similar is being done with uffd-wp markers IIRC.
>>>
>>> Yeah that's a good point, but honestly if you're reading smaps that reads
>>> the page tables, then reading /proc/$pid/pagemaps and reading page tables
>>> TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
>>> /proc/$pid/pagemaps and reading page tables once.
>>
>> Right; I recently wished that we would have an interface to obtain more VMA
>> flags without having to go through smaps
>
> Well maybe that lends itself to the idea of adding a whole new interface in
> general...
An extended "maps" interface might be reasonable, that allows for
exposing more things without walking the page tables. (e.g., flags)
Maybe one could have an indicator that says "ever had guard regions in
this mapping" without actually walking the page tables.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 10:15 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <4f5a9c19-9bdd-47eb-bb14-205e3dcced90@redhat.com>
On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > that. But maybe you and Kalesh have a history on that that let's you react
> > > on his questions IMHO more emotionally than it should have been.
> >
> > This is wholly unfair, I have been very reasonable in response to this
> > thread. I have offered to find solutions, I have tried to understand the
> > problem in spite of having gone to great lengths to try to discuss the
> > limitations of the proposed approach in every venue I possibly could.
> >
> > I go out of my way to deal professionally and objectively with what is
> > presented. Nothing here is emotional. So I'd ask that you please abstain
> > from making commentary like this which has no basis.
>
> I appreciate everything you write below. But this request is just
> impossible. I will keep raising my opinion and misunderstandings will
> happen.
Well I wouldn't ask you not to express your opinion David, you know I respect
and like you, and by all means push back hard or call out what you think is bad
behaviour :)
I just meant to say, in my view, that there was no basis, but I appreciate
miscommunications happen.
So apologies if I came off as being difficult or rude, it actually wasn't
intended. And to re-emphasise - I have zero personal issue with anybody in this
thread whatsoever!
I just want to find the best way forward, technically and am willing to do
whatever work is required to make the guard region implementation as good as it
possibly can be.
>
> Note that the whole "Honestly David you and the naming. .." thing could have
> been written as "I don't think it's a naming problem."
I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
style, which is what this was meant to be... so I think herein lies the basis of
the miscommunication :)
I apologise, the household is ill, which maybe affects my judgment in how I
write these, but in general text is a very poor medium. It was meant to be said
in a jolly tone with a wink...
I think maybe I should learn my lesson with these things, I thought the ':p'
would make this clear but yeah, text, poor medium.
Anyway apologies if this seemed disrespectful.
>
> > >
> > >
> > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > >
> > > > > > As I said to you earlier, the _best_ we could do in smaps would be to add a
> > > > > > flag like 'Grd' or something to indicate some part of the VMA is
> > > > > > guarded. But I won't do that unless somebody has an -actual use case- for
> > > > > > it.
> > > > >
> > > > > Right, and that would limit where you have to manually scan. Something
> > > > > similar is being done with uffd-wp markers IIRC.
> > > >
> > > > Yeah that's a good point, but honestly if you're reading smaps that reads
> > > > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > > > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > > > /proc/$pid/pagemaps and reading page tables once.
> > >
> > > Right; I recently wished that we would have an interface to obtain more VMA
> > > flags without having to go through smaps
> >
> > Well maybe that lends itself to the idea of adding a whole new interface in
> > general...
>
> An extended "maps" interface might be reasonable, that allows for exposing
> more things without walking the page tables. (e.g., flags)
>
> Maybe one could have an indicator that says "ever had guard regions in this
> mapping" without actually walking the page tables.
Yeah this is something we've discussed before, but it's a little fraught. Let's
say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
impact merging (easy enough) to account for splits/merges.
The problem comes in that we would then need to acquire the VMA write lock to do
it, something we don't currently require on application of guard regions.
We'd also have to make sure nothing else makes any assumptions about VMA flags
implying differences in VMAs in this one instance (though we do already do this
for VM_SOFTDIRTY).
I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
>
> --
> Cheers,
>
> David / dhildenb
>
>
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-20 12:44 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <1e959451-2534-44b7-bf62-bc75305048fe@lucifer.local>
On 20.02.25 11:15, Lorenzo Stoakes wrote:
> On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
>>>> Your conclusion is 'did not participate with upstream'; I don't agree with
>>>> that. But maybe you and Kalesh have a history on that that let's you react
>>>> on his questions IMHO more emotionally than it should have been.
>>>
>>> This is wholly unfair, I have been very reasonable in response to this
>>> thread. I have offered to find solutions, I have tried to understand the
>>> problem in spite of having gone to great lengths to try to discuss the
>>> limitations of the proposed approach in every venue I possibly could.
>>>
>>> I go out of my way to deal professionally and objectively with what is
>>> presented. Nothing here is emotional. So I'd ask that you please abstain
>>> from making commentary like this which has no basis.
>>
>> I appreciate everything you write below. But this request is just
>> impossible. I will keep raising my opinion and misunderstandings will
>> happen.
>
> Well I wouldn't ask you not to express your opinion David, you know I respect
> and like you, and by all means push back hard or call out what you think is bad
> behaviour :)
>
> I just meant to say, in my view, that there was no basis, but I appreciate
> miscommunications happen.
> > So apologies if I came off as being difficult or rude, it actually
wasn't
> intended. And to re-emphasise - I have zero personal issue with anybody in this
> thread whatsoever!
It sounded to me like you were trying to defend your work (again, IMHO
too emotionally, and I might have completely misinterpreted that) and
slowly switching to "friendly fire" (towards me). Apologies from my side
if I completely misunderstood/misinterpreted that.
To recap: what we have upstream is great; you did a great job. Yes, the
mechanism has its drawbacks, but that's just part of the design.
Some people maybe have wrong expectations, maybe there were
misunderstandings, or maybe there are requirements that only now pop up;
it's sometimes unavoidable, and that's ok.
We can try to document it better (and I was trying to find clues why
people might be mislead), and see if/how we could sort out these
requirements. But we can likely not make it perfect in any possible way
(I'm sure there are plenty of use cases where what we currently have is
more than sufficient).
> > I just want to find the best way forward, technically and am
willing to do
> whatever work is required to make the guard region implementation as good as it
> possibly can be.
>
>>
>> Note that the whole "Honestly David you and the naming. .." thing could have
>> been written as "I don't think it's a naming problem."
>
> I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
> style, which is what this was meant to be... so I think herein lies the basis of
> the miscommunication :)
>
> I apologise, the household is ill, which maybe affects my judgment in how I
> write these, but in general text is a very poor medium. It was meant to be said
> in a jolly tone with a wink...
>
> I think maybe I should learn my lesson with these things, I thought the ':p'
> would make this clear but yeah, text, poor medium.
>
> Anyway apologies if this seemed disrespectful.
No worries, it's hard to really make me angry, and I appreciate your
openness and your apology (well, and you and your work, obviously).
I'll note, though, if my memory serves me right, that nobody so far ever
criticized the way I communicate upstream, or even told me to abstain
from certain communication.
That probably hurt most, now that a couple of hours passed. Nothing that
a couple of beers and a bit of self-reflection on my communication style
can't fix ;)
[...]
>>>>> Yeah that's a good point, but honestly if you're reading smaps that reads
>>>>> the page tables, then reading /proc/$pid/pagemaps and reading page tables
>>>>> TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
>>>>> /proc/$pid/pagemaps and reading page tables once.
>>>>
>>>> Right; I recently wished that we would have an interface to obtain more VMA
>>>> flags without having to go through smaps
>>>
>>> Well maybe that lends itself to the idea of adding a whole new interface in
>>> general...
>>
>> An extended "maps" interface might be reasonable, that allows for exposing
>> more things without walking the page tables. (e.g., flags)
>>
>> Maybe one could have an indicator that says "ever had guard regions in this
>> mapping" without actually walking the page tables.
>
> Yeah this is something we've discussed before, but it's a little fraught. Let's
> say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
> impact merging (easy enough) to account for splits/merges.
> > The problem comes in that we would then need to acquire the VMA
write lock to do
> it, something we don't currently require on application of guard regions.
Right, and we shouldn't write-lock the mmap. We'd need some way to just
atomically set such an indicator on a VMA.
I'll also note that it might be helpful for smallish region, but
especially for large ones (including when they are split and the
indicator is wrong), it's less helpful. I don't have to tell you about
the VMA merging implications, probably it would be like VM_SOFTDIRTY
handling :)
>
> We'd also have to make sure nothing else makes any assumptions about VMA flags
> implying differences in VMAs in this one instance (though we do already do this
> for VM_SOFTDIRTY).
>
> I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
Yes.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-20 13:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kalesh Singh, Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <bd4597b5-2da2-484c-9410-384e04336a9d@redhat.com>
On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > on his questions IMHO more emotionally than it should have been.
> > > >
> > > > This is wholly unfair, I have been very reasonable in response to this
> > > > thread. I have offered to find solutions, I have tried to understand the
> > > > problem in spite of having gone to great lengths to try to discuss the
> > > > limitations of the proposed approach in every venue I possibly could.
> > > >
> > > > I go out of my way to deal professionally and objectively with what is
> > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > from making commentary like this which has no basis.
> > >
> > > I appreciate everything you write below. But this request is just
> > > impossible. I will keep raising my opinion and misunderstandings will
> > > happen.
> >
> > Well I wouldn't ask you not to express your opinion David, you know I respect
> > and like you, and by all means push back hard or call out what you think is bad
> > behaviour :)
> >
> > I just meant to say, in my view, that there was no basis, but I appreciate
> > miscommunications happen.
> > > So apologies if I came off as being difficult or rude, it actually
> wasn't
> > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > thread whatsoever!
>
> It sounded to me like you were trying to defend your work (again, IMHO too
> emotionally, and I might have completely misinterpreted that) and slowly
> switching to "friendly fire" (towards me). Apologies from my side if I
> completely misunderstood/misinterpreted that.
Right this was not at all my intent, sorry if it seemed that way. I may well
have communicated terribly, so apologies on my side too.
>
> To recap: what we have upstream is great; you did a great job. Yes, the
> mechanism has its drawbacks, but that's just part of the design.
Thanks :)
>
> Some people maybe have wrong expectations, maybe there were
> misunderstandings, or maybe there are requirements that only now pop up;
> it's sometimes unavoidable, and that's ok.
>
> We can try to document it better (and I was trying to find clues why people
> might be mislead), and see if/how we could sort out these requirements. But
> we can likely not make it perfect in any possible way (I'm sure there are
> plenty of use cases where what we currently have is more than sufficient).
Sure and I"m very open to adding a documentation page for guard regions, in
fact was considering this very thing recently. I already added man pages
but be good to be able to go into more depth.
>
> > > I just want to find the best way forward, technically and am willing to
> do
> > whatever work is required to make the guard region implementation as good as it
> > possibly can be.
> >
> > >
> > > Note that the whole "Honestly David you and the naming. .." thing could have
> > > been written as "I don't think it's a naming problem."
> >
> > I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
> > style, which is what this was meant to be... so I think herein lies the basis of
> > the miscommunication :)
> >
> > I apologise, the household is ill, which maybe affects my judgment in how I
> > write these, but in general text is a very poor medium. It was meant to be said
> > in a jolly tone with a wink...
> >
> > I think maybe I should learn my lesson with these things, I thought the ':p'
> > would make this clear but yeah, text, poor medium.
> >
> > Anyway apologies if this seemed disrespectful.
>
> No worries, it's hard to really make me angry, and I appreciate your
> openness and your apology (well, and you and your work, obviously).
>
> I'll note, though, if my memory serves me right, that nobody so far ever
> criticized the way I communicate upstream, or even told me to abstain from
> certain communication.
I wish I could say the same haha, so perhaps this was a problem on my side
honestly. I do have a habit of being 'tongue in cheek' and failing to
communicate that which I did say the last time I wouldn't repeat. It is not
intended, I promise.
As the abstain, was more a British turn of phrase, meaning to say - I
dispute the claim that this is an emotional thing and please don't say this
if it isn't so.
But I understand that of course, you may have interpreted it as so, due to
my having failed to communicate it well.
Again, I must say, text remains replete with possibilities for
miscommunication, misunderstanding and it can so often be difficult to
communicate one's intent.
But again of course, I apologise if I overstepped the line in any way!
>
> That probably hurt most, now that a couple of hours passed. Nothing that a
> couple of beers and a bit of self-reflection on my communication style can't
> fix ;)
Ugh sorry, man. Not my intent. And it seems - I literally OWE YOU pints
now. :) we will fix this at lsf...
Perhaps owe Kalesh some too should he be there... will budget
accordingly... :P
>
> [...]
>
> > > > > > Yeah that's a good point, but honestly if you're reading smaps that reads
> > > > > > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > > > > > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > > > > > /proc/$pid/pagemaps and reading page tables once.
> > > > >
> > > > > Right; I recently wished that we would have an interface to obtain more VMA
> > > > > flags without having to go through smaps
> > > >
> > > > Well maybe that lends itself to the idea of adding a whole new interface in
> > > > general...
> > >
> > > An extended "maps" interface might be reasonable, that allows for exposing
> > > more things without walking the page tables. (e.g., flags)
> > >
> > > Maybe one could have an indicator that says "ever had guard regions in this
> > > mapping" without actually walking the page tables.
> >
> > Yeah this is something we've discussed before, but it's a little fraught. Let's
> > say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
> > impact merging (easy enough) to account for splits/merges.
> > > The problem comes in that we would then need to acquire the VMA write
> lock to do
> > it, something we don't currently require on application of guard regions.
>
> Right, and we shouldn't write-lock the mmap. We'd need some way to just
> atomically set such an indicator on a VMA.
Hm yeah, could be tricky, we definitely can't manage a new field in
vm_area_struct, this is a very sensitive subject at the moment really with
Suren's work with VMAs allocated via SLAB_TYPESAFE_BY_RCU, putting the lock
into the VMA and the alignment requirements.
Not sure what precedent we'd have with atomic setting of a VMA flag for
this... could be tricky.
>
> I'll also note that it might be helpful for smallish region, but especially
> for large ones (including when they are split and the indicator is wrong),
> it's less helpful. I don't have to tell you about the VMA merging
> implications, probably it would be like VM_SOFTDIRTY handling :)
Yeah indeed now we've simplified merging a lot of possibilities emerge,
this is one!
>
> >
> > We'd also have to make sure nothing else makes any assumptions about VMA flags
> > implying differences in VMAs in this one instance (though we do already do this
> > for VM_SOFTDIRTY).
> >
> > I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
>
> Yes.
>
> --
> Cheers,
>
> David / dhildenb
>
Best, Lorenzo
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Suren Baghdasaryan @ 2025-02-20 16:21 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: David Hildenbrand, Kalesh Singh, Andrew Morton, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas
In-Reply-To: <31a007c0-884f-495d-ba27-08e3e0dd767d@lucifer.local>
On Thu, Feb 20, 2025 at 5:18 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> > On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > > on his questions IMHO more emotionally than it should have been.
> > > > >
> > > > > This is wholly unfair, I have been very reasonable in response to this
> > > > > thread. I have offered to find solutions, I have tried to understand the
> > > > > problem in spite of having gone to great lengths to try to discuss the
> > > > > limitations of the proposed approach in every venue I possibly could.
> > > > >
> > > > > I go out of my way to deal professionally and objectively with what is
> > > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > > from making commentary like this which has no basis.
> > > >
> > > > I appreciate everything you write below. But this request is just
> > > > impossible. I will keep raising my opinion and misunderstandings will
> > > > happen.
> > >
> > > Well I wouldn't ask you not to express your opinion David, you know I respect
> > > and like you, and by all means push back hard or call out what you think is bad
> > > behaviour :)
> > >
> > > I just meant to say, in my view, that there was no basis, but I appreciate
> > > miscommunications happen.
> > > > So apologies if I came off as being difficult or rude, it actually
> > wasn't
> > > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > > thread whatsoever!
> >
> > It sounded to me like you were trying to defend your work (again, IMHO too
> > emotionally, and I might have completely misinterpreted that) and slowly
> > switching to "friendly fire" (towards me). Apologies from my side if I
> > completely misunderstood/misinterpreted that.
>
> Right this was not at all my intent, sorry if it seemed that way. I may well
> have communicated terribly, so apologies on my side too.
Sorry for being late to the party. Was sick for a couple of days.
Lorenzo is right, there was a breakdown in communication at Google and
he has all the rights to be upset. The issue with obfuscators should
have been communicated once it was discovered. I was in regular
discussions with Lorenzo but wasn't directly involved with this
particular project and wasn't aware or did not realize that the
obfuscator issue renders guards unusable for this usecase. My
apologies, I should have asked more questions about it. I suspect
Lorenzo would have implemented this anyway...
To make guard regions work for this usecase, first we (Android) need
to abstract /proc/pid/maps accesses. Only then we can use additional
interfaces like /proc/pid/pagemaps to obtain guard region information.
I'll start figuring out what it takes to insert such an abstraction.
Thanks,
Suren.
>
> >
> > To recap: what we have upstream is great; you did a great job. Yes, the
> > mechanism has its drawbacks, but that's just part of the design.
>
> Thanks :)
>
> >
> > Some people maybe have wrong expectations, maybe there were
> > misunderstandings, or maybe there are requirements that only now pop up;
> > it's sometimes unavoidable, and that's ok.
> >
> > We can try to document it better (and I was trying to find clues why people
> > might be mislead), and see if/how we could sort out these requirements. But
> > we can likely not make it perfect in any possible way (I'm sure there are
> > plenty of use cases where what we currently have is more than sufficient).
>
> Sure and I"m very open to adding a documentation page for guard regions, in
> fact was considering this very thing recently. I already added man pages
> but be good to be able to go into more depth.
>
> >
> > > > I just want to find the best way forward, technically and am willing to
> > do
> > > whatever work is required to make the guard region implementation as good as it
> > > possibly can be.
> > >
> > > >
> > > > Note that the whole "Honestly David you and the naming. .." thing could have
> > > > been written as "I don't think it's a naming problem."
> > >
> > > I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
> > > style, which is what this was meant to be... so I think herein lies the basis of
> > > the miscommunication :)
> > >
> > > I apologise, the household is ill, which maybe affects my judgment in how I
> > > write these, but in general text is a very poor medium. It was meant to be said
> > > in a jolly tone with a wink...
> > >
> > > I think maybe I should learn my lesson with these things, I thought the ':p'
> > > would make this clear but yeah, text, poor medium.
> > >
> > > Anyway apologies if this seemed disrespectful.
> >
> > No worries, it's hard to really make me angry, and I appreciate your
> > openness and your apology (well, and you and your work, obviously).
> >
> > I'll note, though, if my memory serves me right, that nobody so far ever
> > criticized the way I communicate upstream, or even told me to abstain from
> > certain communication.
>
> I wish I could say the same haha, so perhaps this was a problem on my side
> honestly. I do have a habit of being 'tongue in cheek' and failing to
> communicate that which I did say the last time I wouldn't repeat. It is not
> intended, I promise.
>
> As the abstain, was more a British turn of phrase, meaning to say - I
> dispute the claim that this is an emotional thing and please don't say this
> if it isn't so.
>
> But I understand that of course, you may have interpreted it as so, due to
> my having failed to communicate it well.
>
> Again, I must say, text remains replete with possibilities for
> miscommunication, misunderstanding and it can so often be difficult to
> communicate one's intent.
>
> But again of course, I apologise if I overstepped the line in any way!
>
> >
> > That probably hurt most, now that a couple of hours passed. Nothing that a
> > couple of beers and a bit of self-reflection on my communication style can't
> > fix ;)
>
> Ugh sorry, man. Not my intent. And it seems - I literally OWE YOU pints
> now. :) we will fix this at lsf...
>
> Perhaps owe Kalesh some too should he be there... will budget
> accordingly... :P
>
> >
> > [...]
> >
> > > > > > > Yeah that's a good point, but honestly if you're reading smaps that reads
> > > > > > > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > > > > > > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > > > > > > /proc/$pid/pagemaps and reading page tables once.
> > > > > >
> > > > > > Right; I recently wished that we would have an interface to obtain more VMA
> > > > > > flags without having to go through smaps
> > > > >
> > > > > Well maybe that lends itself to the idea of adding a whole new interface in
> > > > > general...
> > > >
> > > > An extended "maps" interface might be reasonable, that allows for exposing
> > > > more things without walking the page tables. (e.g., flags)
> > > >
> > > > Maybe one could have an indicator that says "ever had guard regions in this
> > > > mapping" without actually walking the page tables.
> > >
> > > Yeah this is something we've discussed before, but it's a little fraught. Let's
> > > say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
> > > impact merging (easy enough) to account for splits/merges.
> > > > The problem comes in that we would then need to acquire the VMA write
> > lock to do
> > > it, something we don't currently require on application of guard regions.
> >
> > Right, and we shouldn't write-lock the mmap. We'd need some way to just
> > atomically set such an indicator on a VMA.
>
> Hm yeah, could be tricky, we definitely can't manage a new field in
> vm_area_struct, this is a very sensitive subject at the moment really with
> Suren's work with VMAs allocated via SLAB_TYPESAFE_BY_RCU, putting the lock
> into the VMA and the alignment requirements.
>
> Not sure what precedent we'd have with atomic setting of a VMA flag for
> this... could be tricky.
>
> >
> > I'll also note that it might be helpful for smallish region, but especially
> > for large ones (including when they are split and the indicator is wrong),
> > it's less helpful. I don't have to tell you about the VMA merging
> > implications, probably it would be like VM_SOFTDIRTY handling :)
>
> Yeah indeed now we've simplified merging a lot of possibilities emerge,
> this is one!
>
> >
> > >
> > > We'd also have to make sure nothing else makes any assumptions about VMA flags
> > > implying differences in VMAs in this one instance (though we do already do this
> > > for VM_SOFTDIRTY).
> > >
> > > I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
> >
> > Yes.
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
>
> Best, Lorenzo
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Kalesh Singh @ 2025-02-20 18:08 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Lorenzo Stoakes, David Hildenbrand, Andrew Morton,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <CAJuCfpHpchh0CzEgh5CKmRLwpscBLx32A-mGi4eudpir1wm=cQ@mail.gmail.com>
On Thu, Feb 20, 2025 at 8:22 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Thu, Feb 20, 2025 at 5:18 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> > > On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > > > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > > > on his questions IMHO more emotionally than it should have been.
> > > > > >
> > > > > > This is wholly unfair, I have been very reasonable in response to this
> > > > > > thread. I have offered to find solutions, I have tried to understand the
> > > > > > problem in spite of having gone to great lengths to try to discuss the
> > > > > > limitations of the proposed approach in every venue I possibly could.
> > > > > >
> > > > > > I go out of my way to deal professionally and objectively with what is
> > > > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > > > from making commentary like this which has no basis.
> > > > >
> > > > > I appreciate everything you write below. But this request is just
> > > > > impossible. I will keep raising my opinion and misunderstandings will
> > > > > happen.
> > > >
> > > > Well I wouldn't ask you not to express your opinion David, you know I respect
> > > > and like you, and by all means push back hard or call out what you think is bad
> > > > behaviour :)
> > > >
> > > > I just meant to say, in my view, that there was no basis, but I appreciate
> > > > miscommunications happen.
> > > > > So apologies if I came off as being difficult or rude, it actually
> > > wasn't
> > > > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > > > thread whatsoever!
> > >
> > > It sounded to me like you were trying to defend your work (again, IMHO too
> > > emotionally, and I might have completely misinterpreted that) and slowly
> > > switching to "friendly fire" (towards me). Apologies from my side if I
> > > completely misunderstood/misinterpreted that.
> >
> > Right this was not at all my intent, sorry if it seemed that way. I may well
> > have communicated terribly, so apologies on my side too.
Hi everyone,
Thank you for all the discussion.
I don't find any personal issues with the communication in this
thread, but I appreciate David being the object voice of reason.
I understand it can be frustrating since you have made many efforts to
communicate these tradeoffs. Unfortunately these issues were not known
for the file-backed ELF guard regions for my particular use case.
>
> Sorry for being late to the party. Was sick for a couple of days.
> Lorenzo is right, there was a breakdown in communication at Google and
> he has all the rights to be upset. The issue with obfuscators should
> have been communicated once it was discovered. I was in regular
> discussions with Lorenzo but wasn't directly involved with this
> particular project and wasn't aware or did not realize that the
> obfuscator issue renders guards unusable for this usecase. My
> apologies, I should have asked more questions about it. I suspect
> Lorenzo would have implemented this anyway...
>
Suren's use case is different from mine and this design fits perfectly
for anon guard regions from the allocator. :)
So I think in conclusion, these aren't VMAs and shouldn't be treated
as such; we will advertise them from pagemap for those who need to
know.
-- Kalesh
> To make guard regions work for this usecase, first we (Android) need
> to abstract /proc/pid/maps accesses. Only then we can use additional
> interfaces like /proc/pid/pagemaps to obtain guard region information.
> I'll start figuring out what it takes to insert such an abstraction.
> Thanks,
> Suren.
>
>
> >
> > >
> > > To recap: what we have upstream is great; you did a great job. Yes, the
> > > mechanism has its drawbacks, but that's just part of the design.
> >
> > Thanks :)
> >
> > >
> > > Some people maybe have wrong expectations, maybe there were
> > > misunderstandings, or maybe there are requirements that only now pop up;
> > > it's sometimes unavoidable, and that's ok.
> > >
> > > We can try to document it better (and I was trying to find clues why people
> > > might be mislead), and see if/how we could sort out these requirements. But
> > > we can likely not make it perfect in any possible way (I'm sure there are
> > > plenty of use cases where what we currently have is more than sufficient).
> >
> > Sure and I"m very open to adding a documentation page for guard regions, in
> > fact was considering this very thing recently. I already added man pages
> > but be good to be able to go into more depth.
> >
> > >
> > > > > I just want to find the best way forward, technically and am willing to
> > > do
> > > > whatever work is required to make the guard region implementation as good as it
> > > > possibly can be.
> > > >
> > > > >
> > > > > Note that the whole "Honestly David you and the naming. .." thing could have
> > > > > been written as "I don't think it's a naming problem."
> > > >
> > > > I feel like I _always_ get in trouble when I try to write in a 'tongue-in-cheek'
> > > > style, which is what this was meant to be... so I think herein lies the basis of
> > > > the miscommunication :)
> > > >
> > > > I apologise, the household is ill, which maybe affects my judgment in how I
> > > > write these, but in general text is a very poor medium. It was meant to be said
> > > > in a jolly tone with a wink...
> > > >
> > > > I think maybe I should learn my lesson with these things, I thought the ':p'
> > > > would make this clear but yeah, text, poor medium.
> > > >
> > > > Anyway apologies if this seemed disrespectful.
> > >
> > > No worries, it's hard to really make me angry, and I appreciate your
> > > openness and your apology (well, and you and your work, obviously).
> > >
> > > I'll note, though, if my memory serves me right, that nobody so far ever
> > > criticized the way I communicate upstream, or even told me to abstain from
> > > certain communication.
> >
> > I wish I could say the same haha, so perhaps this was a problem on my side
> > honestly. I do have a habit of being 'tongue in cheek' and failing to
> > communicate that which I did say the last time I wouldn't repeat. It is not
> > intended, I promise.
> >
> > As the abstain, was more a British turn of phrase, meaning to say - I
> > dispute the claim that this is an emotional thing and please don't say this
> > if it isn't so.
> >
> > But I understand that of course, you may have interpreted it as so, due to
> > my having failed to communicate it well.
> >
> > Again, I must say, text remains replete with possibilities for
> > miscommunication, misunderstanding and it can so often be difficult to
> > communicate one's intent.
> >
> > But again of course, I apologise if I overstepped the line in any way!
> >
> > >
> > > That probably hurt most, now that a couple of hours passed. Nothing that a
> > > couple of beers and a bit of self-reflection on my communication style can't
> > > fix ;)
> >
> > Ugh sorry, man. Not my intent. And it seems - I literally OWE YOU pints
> > now. :) we will fix this at lsf...
> >
> > Perhaps owe Kalesh some too should he be there... will budget
> > accordingly... :P
> >
> > >
> > > [...]
> > >
> > > > > > > > Yeah that's a good point, but honestly if you're reading smaps that reads
> > > > > > > > the page tables, then reading /proc/$pid/pagemaps and reading page tables
> > > > > > > > TWICE that seems inefficient vs. just reading /proc/$pid/maps, then reading
> > > > > > > > /proc/$pid/pagemaps and reading page tables once.
> > > > > > >
> > > > > > > Right; I recently wished that we would have an interface to obtain more VMA
> > > > > > > flags without having to go through smaps
> > > > > >
> > > > > > Well maybe that lends itself to the idea of adding a whole new interface in
> > > > > > general...
> > > > >
> > > > > An extended "maps" interface might be reasonable, that allows for exposing
> > > > > more things without walking the page tables. (e.g., flags)
> > > > >
> > > > > Maybe one could have an indicator that says "ever had guard regions in this
> > > > > mapping" without actually walking the page tables.
> > > >
> > > > Yeah this is something we've discussed before, but it's a little fraught. Let's
> > > > say it was a VMA flag, in this case we'd have to make this flag 'sticky' and not
> > > > impact merging (easy enough) to account for splits/merges.
> > > > > The problem comes in that we would then need to acquire the VMA write
> > > lock to do
> > > > it, something we don't currently require on application of guard regions.
> > >
> > > Right, and we shouldn't write-lock the mmap. We'd need some way to just
> > > atomically set such an indicator on a VMA.
> >
> > Hm yeah, could be tricky, we definitely can't manage a new field in
> > vm_area_struct, this is a very sensitive subject at the moment really with
> > Suren's work with VMAs allocated via SLAB_TYPESAFE_BY_RCU, putting the lock
> > into the VMA and the alignment requirements.
> >
> > Not sure what precedent we'd have with atomic setting of a VMA flag for
> > this... could be tricky.
> >
> > >
> > > I'll also note that it might be helpful for smallish region, but especially
> > > for large ones (including when they are split and the indicator is wrong),
> > > it's less helpful. I don't have to tell you about the VMA merging
> > > implications, probably it would be like VM_SOFTDIRTY handling :)
> >
> > Yeah indeed now we've simplified merging a lot of possibilities emerge,
> > this is one!
> >
> > >
> > > >
> > > > We'd also have to make sure nothing else makes any assumptions about VMA flags
> > > > implying differences in VMAs in this one instance (though we do already do this
> > > > for VM_SOFTDIRTY).
> > > >
> > > > I saw this as possibly something like VM_MAYBE_GUARD_REGIONS or something.
> > >
> > > Yes.
> > >
> > > --
> > > Cheers,
> > >
> > > David / dhildenb
> > >
> >
> > Best, Lorenzo
^ permalink raw reply
* [PATCH v2] ntsync: Check wait count based on byte size.
From: Elizabeth Figura @ 2025-02-20 19:23 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-kernel, linux-api, wine-devel, Elizabeth Figura,
kernel test robot
GCC versions below 13 incorrectly detect the copy size as being static and too
small to fit in the "fds" array. Work around this by explicitly calculating the
size and returning EINVAL based on that, instead of based on the object count.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp@intel.com/
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
--
Suggested-by as per Arnd's request, but the only thing I changed was preserving
array_size() [as noted by Geert in the linked thread]. I tested and found no
regressions.
v2: Add missing sign-off
---
drivers/misc/ntsync.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 586b86243e1d..24cb2f71186a 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -873,6 +873,7 @@ static int setup_wait(struct ntsync_device *dev,
{
int fds[NTSYNC_MAX_WAIT_COUNT + 1];
const __u32 count = args->count;
+ size_t size = array_size(count, sizeof(fds[0]));
struct ntsync_q *q;
__u32 total_count;
__u32 i, j;
@@ -880,15 +881,14 @@ static int setup_wait(struct ntsync_device *dev,
if (args->pad || (args->flags & ~NTSYNC_WAIT_REALTIME))
return -EINVAL;
- if (args->count > NTSYNC_MAX_WAIT_COUNT)
+ if (size >= sizeof(fds))
return -EINVAL;
total_count = count;
if (args->alert)
total_count++;
- if (copy_from_user(fds, u64_to_user_ptr(args->objs),
- array_size(count, sizeof(*fds))))
+ if (copy_from_user(fds, u64_to_user_ptr(args->objs), size))
return -EFAULT;
if (args->alert)
fds[count] = args->alert;
--
2.47.2
^ permalink raw reply related
* Re: [PATCH v2] ntsync: Check wait count based on byte size.
From: Elizabeth Figura @ 2025-02-20 19:29 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-kernel, linux-api, wine-devel, kernel test robot
In-Reply-To: <20250220192334.549167-1-zfigura@codeweavers.com>
On Thursday, 20 February 2025 13:23:34 CST Elizabeth Figura wrote:
> GCC versions below 13 incorrectly detect the copy size as being static and too
> small to fit in the "fds" array. Work around this by explicitly calculating the
> size and returning EINVAL based on that, instead of based on the object count.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp@intel.com/
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
>
Oops, I forgot to include a reference to v1:
https://lore.kernel.org/all/20250219001920.210847-1-zfigura@codeweavers.com/
^ permalink raw reply
* Re: [PATCH v2] ntsync: Check wait count based on byte size.
From: Greg Kroah-Hartman @ 2025-02-21 5:53 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, linux-kernel, linux-api, wine-devel,
kernel test robot
In-Reply-To: <3342044.44csPzL39Z@camazotz>
On Thu, Feb 20, 2025 at 01:29:25PM -0600, Elizabeth Figura wrote:
> On Thursday, 20 February 2025 13:23:34 CST Elizabeth Figura wrote:
> > GCC versions below 13 incorrectly detect the copy size as being static and too
> > small to fit in the "fds" array. Work around this by explicitly calculating the
> > size and returning EINVAL based on that, instead of based on the object count.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp@intel.com/
> > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
> >
>
> Oops, I forgot to include a reference to v1:
>
> https://lore.kernel.org/all/20250219001920.210847-1-zfigura@codeweavers.com/
No need for the reference, that's easy enough to find if I needed it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-21 11:04 UTC (permalink / raw)
To: Kalesh Singh
Cc: Suren Baghdasaryan, David Hildenbrand, Andrew Morton,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <CAC_TJvd2Y-EnavZkt5_nQUXmRpjo8AYMu6rND7eMUwXn27ab0A@mail.gmail.com>
On Thu, Feb 20, 2025 at 10:08:36AM -0800, Kalesh Singh wrote:
> On Thu, Feb 20, 2025 at 8:22 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 5:18 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> > > > On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > > > > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > > > > on his questions IMHO more emotionally than it should have been.
> > > > > > >
> > > > > > > This is wholly unfair, I have been very reasonable in response to this
> > > > > > > thread. I have offered to find solutions, I have tried to understand the
> > > > > > > problem in spite of having gone to great lengths to try to discuss the
> > > > > > > limitations of the proposed approach in every venue I possibly could.
> > > > > > >
> > > > > > > I go out of my way to deal professionally and objectively with what is
> > > > > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > > > > from making commentary like this which has no basis.
> > > > > >
> > > > > > I appreciate everything you write below. But this request is just
> > > > > > impossible. I will keep raising my opinion and misunderstandings will
> > > > > > happen.
> > > > >
> > > > > Well I wouldn't ask you not to express your opinion David, you know I respect
> > > > > and like you, and by all means push back hard or call out what you think is bad
> > > > > behaviour :)
> > > > >
> > > > > I just meant to say, in my view, that there was no basis, but I appreciate
> > > > > miscommunications happen.
> > > > > > So apologies if I came off as being difficult or rude, it actually
> > > > wasn't
> > > > > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > > > > thread whatsoever!
> > > >
> > > > It sounded to me like you were trying to defend your work (again, IMHO too
> > > > emotionally, and I might have completely misinterpreted that) and slowly
> > > > switching to "friendly fire" (towards me). Apologies from my side if I
> > > > completely misunderstood/misinterpreted that.
> > >
> > > Right this was not at all my intent, sorry if it seemed that way. I may well
> > > have communicated terribly, so apologies on my side too.
>
> Hi everyone,
>
> Thank you for all the discussion.
>
> I don't find any personal issues with the communication in this
> thread, but I appreciate David being the object voice of reason.
>
> I understand it can be frustrating since you have made many efforts to
> communicate these tradeoffs. Unfortunately these issues were not known
> for the file-backed ELF guard regions for my particular use case.
>
> >
> > Sorry for being late to the party. Was sick for a couple of days.
> > Lorenzo is right, there was a breakdown in communication at Google and
> > he has all the rights to be upset. The issue with obfuscators should
> > have been communicated once it was discovered. I was in regular
> > discussions with Lorenzo but wasn't directly involved with this
> > particular project and wasn't aware or did not realize that the
> > obfuscator issue renders guards unusable for this usecase. My
> > apologies, I should have asked more questions about it. I suspect
> > Lorenzo would have implemented this anyway...
> >
>
> Suren's use case is different from mine and this design fits perfectly
> for anon guard regions from the allocator. :)
>
> So I think in conclusion, these aren't VMAs and shouldn't be treated
> as such; we will advertise them from pagemap for those who need to
> know.
>
Thanks Kalesh, glad there were no issues here and we have found
constructive common ground! :)
It turns out implementing the pagemap side of things is _really_
straightforward, so I'll be sending a series for that shortly. Hopefully
this provides some basis for whichever use cases need this information, as
it is the best and least invasive place for this information at this stage.
Cheers, Lorenzo
> -- Kalesh
^ permalink raw reply
* [PATCH 0/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-21 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Kalesh Singh, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
Juan Yescas, linux-mm, linux-doc, linux-kernel, linux-fsdevel,
linux-kselftest, linux-api
Currently there is no means of determining whether a give page in a mapping
range is designated a guard region (as installed via madvise() using the
MADV_GUARD_INSTALL flag).
This is generally not an issue, but in some instances users may wish to
determine whether this is the case.
This series adds this ability via /proc/$pid/pagemap, updates the
documentation and adds a self test to assert that this functions correctly.
Lorenzo Stoakes (2):
fs/proc/task_mmu: add guard region bit to pagemap
tools/selftests: add guard region test for /proc/$pid/pagemap
Documentation/admin-guide/mm/pagemap.rst | 3 +-
fs/proc/task_mmu.c | 6 ++-
tools/testing/selftests/mm/guard-regions.c | 47 ++++++++++++++++++++++
tools/testing/selftests/mm/vm_util.h | 1 +
4 files changed, 55 insertions(+), 2 deletions(-)
--
2.48.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox