Re: [PATCH v2 0/6] mm: Optimize mseal checks

[Jeff Xu <jeffxu@chromium.org>]

Hi Andrew, Pedro

On Thu, Aug 8, 2024 at 6:03 PM Jeff Xu <jeffxu@google.com> wrote:
>
> On Thu, Aug 8, 2024 at 5:34 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
> >
> > On Fri, Aug 9, 2024 at 12:12 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed,  7 Aug 2024 22:13:03 +0100 Pedro Falcato <pedro.falcato@gmail.com> wrote:
> > >
> > > > This series also depends on (and will eventually very slightly conflict with)
> > > > the powerpc series that removes arch_unmap[2].
> > >
> > > That's awkward.  Please describe the dependency?
> >
> > One of the transformations done in this patch series (patch 2) assumes
> > that arch_unmap either doesn't exist or does nothing.
> > PPC is the only architecture with an arch_unmap implementation, and
> > through the series I linked they're going to make it work via
> > ->close().
> >
> > What's the easiest way to deal with this? Can the PPC series go
> > through the mm tree?
> >
> This patch can't be merged until arch_unmap() is all removed (ppc change)
>
> Also I'm still doing a test/reviewing for this patch,  perhaps it is
> better to wait till my test is done.
>
Sorry that I'm late for updating this thread.

With removing arch_unmap() change landed , there is no dependency for
the patch. However: I have other comments:

1. Testing
Testing is 90% of work when I modify kernel code and send a patch.  So
I'm a little disappointed that this patch doesn't have any test
updates or add new tests. Which makes me less confident about the
regression risk on mseal itself, i.e. a sealed mapping being
overwritten by mprotect/mmap/mremap/munmap.  I have posted the comment
in  [1], and I would like to repeat it to stress my point.

The V2 series doesn't have selftest change which indicates lack of
testing. The out-of-loop check is positioned nearer to the API entry
point and separated from internal business logic, thereby minimizing
the testing requirements. However, as we move the sealing check
further inward and intertwine it with business logic, greater test
coverage becomes necessary to ensure  the correctness of  sealing
is preserved.

Yes. I promised to run some tests, which I did, with the existing self
test (that passed),  also I added more tests in the mremap selftest.
However I'm bound by the time that I can spend on this  (my other
duties and deliverables), I can't test it as much as I like to for
in-loop change (in a time frame demanded by a dev in this ml). Because
this patch is not getting tested as it should be, my confidence for
the V2 patch is low .

2 perf testing
stress-ng is not stable in my test with Chromebook, and I'm requesting
 Oliver to take more samples [2] . This due diligence assures that
this patch accurately fulfills its purpose. The in-loop approach adds
complexity to the code, i.e. future dev is harder to understand the
sealing logic. Additionally, it sacrifices a security feature that
makes it harder for an attacker to modify mapping (currently if an
attacker uses munmap with a large address range, if one of the
addresses is sealed, the entire range is not modified. In the in-loop
approach,  memory will be unmapped till it hits the sealed memory).
Therefore, I would like to ascertain the gain.

3 mremap refactor work.
I posted mremap refactor work [3] , which is aligned with the
direction that we want to do in-loop change, and it also (imo)  a
better version of handling error cases for mremap across multiple vma
boundaries. That patch set can be applied on its own, and the test
cases added also enhance the existing selftest. I hope my patch can be
reviewed, and if passing perf test/approved, applied to mm first.

Thanks
Best regards,
-Jeff

[1] https://lore.kernel.org/linux-mm/20240814071424.2655666-2-jeffxu@chromium.org/
[2] https://lore.kernel.org/linux-mm/CABi2SkXtZLojx3AQU4C=41NtBPGjVB2+fv_KWziOqyXRQ8P7Bg@mail.gmail.com/
[3] https://lore.kernel.org/linux-mm/20240814071424.2655666-1-jeffxu@chromium.org/