Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

[Jeff Xu <jeffxu@chromium.org>]

Hi Mark

On Wed, Sep 18, 2024 at 6:18 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 13, 2024 at 03:50:00PM -0700, Jeff Xu wrote:
>
> > Even though the number of lines is large in these patches, its main
> > intention is to test Pedro's in-place change (from can_modify_mm to
> > can_modify_vma). Before this patch,  the test had a common pattern:
> > setup memory layout, seal the memory, perform a few mm-api steps, verify
> > return code (not zero).  Because of the nature of out-of-loop,  it is
> > sufficient to just verify the error code in a few cases.
>
> > With Pedro's in-loop change, the sealing check happens later in the
> > stack, thus there are more things and scenarios to verify. And there were
> > feedback to me during in-loop change that selftest should be extensive
> > enough to discover all regressions.  Even though this viewpoint is subject
> > to debate. Since none would want to do it, I thought I would just do it.
>
> > So the Patch V3 1/5 is dedicated entirely to increasing the verification
> > for existing scenarios, this including checking return code code, vma-size,
> > etc after mm api return.
>
> > Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop
> > change, we discovered a bug in unmap(), and unmap() is not atomic.
> > This leads to 4/5(mmap), 5/5(mremap), which calls munmap().
> > In addition, I add scenarios to cover cross-multiple-vma cases.
>
> > The  high-level goal of mseal test are two folds:
> > 1> make sure sealing is working correctly under different scenarios,
> > i.e. sealed mapping are not modified.
> > 2> For unsealed memory, added mseal code  doesn't regress on regular mm API.
>
> > The goal 2 is as important as 1, that is why tests usually are done in
> > two phases, one with sealing, the other without.
>
> That's vastly more detail than is in the changelogs for the actual
> patches (which are just a few lines each) or the cover letter of the
> series.  I don't have the MM knowledge to assess the detail of what
> you're saying but I can't help but think that it'd help a lot with
> review if all this detail were part of the actual submission.
Agreed, will update and give more detail in the next version of the patch.

Thanks
-Jeff