Re: [PATCH v8 0/4] Introduce mseal

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Feb 2, 2024 at 8:46 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> [240202 18:36]:
> > On Fri, 2 Feb 2024 at 13:18, Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > >
> > > There will be a larger performance cost to checking up front without
> > > allowing the partial completion.
> >
> > I suspect that for mseal(), the only half-way common case will be
> > sealing an area that is entirely contained within one vma.
>
> Agreed.
>
> >
> > So the cost will be the vma splitting (if it's not the whole vma), and
> > very unlikely to be any kind of "walk the vma's to check that they can
> > all be sealed" loop up-front.
>
> That's the cost of calling mseal(), and I think that will be totally
> reasonable.
>
> I'm more concerned with the other calls that do affect more than one vma
> that will now have to ensure there is not an mseal'ed vma among the
> affected area.
>
> As you pointed out, we don't do atomic updates and so we have to add a
> loop at the beginning to check this new special case, which is what this
> patch set does today.  That means we're going to be looping through
> twice for any call that could fail if one is mseal'ed. This includes
> munmap() and mprotect().
>
> The impact will vary based on how many vma's are handled. I'd like some
> numbers on this so we can see if it is a concern, which Jeff has agreed
> to provide in the future - Thank you, Jeff.

Yes please. The additional walk Liam points to seems to be happening
even if we don't use mseal at all. Android apps often create thousands
of VMAs, so a small regression to a syscall like mprotect might cause
a very visible regression to app launch times (one of the key metrics
for Android). Having performance impact numbers here would be very
helpful.

>
> It also means we're modifying the behaviour of those calls so they could
> fail before anything changes (regardless of where the failure would
> occur), and we could still fail later when another aspect of a vma would
> cause a failure as we do today.  We are paying the price for a more
> atomic update, but we aren't trying very hard to be atomic with our
> updates - we don't have many (virtually no) vma checks before
> modifications start.
>
> For instance, we could move the mprotect check for map_deny_write_exec()
> to the pre-update loop to make it more atomic in nature.  This one seems
> somewhat related to mseal, so it would be better if they were both
> checked atomic(ish) together.  Although, I wonder if the user visible
> changes would be acceptable and worth the risk.
>
> We will have two classes of updates to vma's: the more atomic view and
> the legacy view.  The question of what happens when the two mix, or
> where a specific check should go will get (more) confusing.
>
> Thanks,
> Liam
>