Message-ID: <CAC_TJvd2Y-EnavZkt5_nQUXmRpjo8AYMu6rND7eMUwXn27ab0A@mail.gmail.com>
Date: Thu, 20 Feb 2025 10:08:36 -0800
From: Kalesh Singh <kaleshsingh@...gle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, David Hildenbrand <david@...hat.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, "Liam R . Howlett" <Liam.Howlett@...cle.com>, 
	Matthew Wilcox <willy@...radead.org>, Vlastimil Babka <vbabka@...e.cz>, 
	"Paul E . McKenney" <paulmck@...nel.org>, Jann Horn <jannh@...gle.com>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, Shuah Khan <shuah@...nel.org>, 
	linux-kselftest@...r.kernel.org, linux-api@...r.kernel.org, 
	John Hubbard <jhubbard@...dia.com>, Juan Yescas <jyescas@...gle.com>
Subject: Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings

On Thu, Feb 20, 2025 at 8:22 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Thu, Feb 20, 2025 at 5:18 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.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

Powered by blists - more mailing lists