Re: [PATCH v8 0/4] Introduce mseal

["Liam R. Howlett" <Liam.Howlett@Oracle.com>]

* 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.

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