Message-ID: <9f2f751e-2acb-4a27-93cb-767628b38236@lucifer.local>
Date: Fri, 30 Aug 2024 20:17:06 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: jeffxu@...omium.org
Cc: akpm@...ux-foundation.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org, pedro.falcato@...il.com,
        willy@...radead.org, broonie@...nel.org, vbabka@...e.cz,
        Liam.Howlett@...cle.com, rientjes@...gle.com, keescook@...omium.org
Subject: Re: [PATCH v3 0/5] Increase mseal test coverage

On Fri, Aug 30, 2024 at 06:02:32PM GMT, jeffxu@...omium.org wrote:
> From: Jeff Xu <jeffxu@...omium.org>
>
> This series increase the test coverage of mseal_test by:
>
> Add check for vma_size, prot, and error code for existing tests.
> Add more testcases for madvise, munmap, mmap and mremap to cover
> sealing in different scenarios.
>
> The increase test coverage hopefully help to prevent future regression.
> It doesn't change any existing mm api's semantics, i.e. it will pass on
> linux main and 6.10 branch.

This is a big improvement in detail, thanks.

>
> Note: in order to pass this test in mm-unstable, mm-unstable must have
> Liam's fix on mmap [1]
>
> [1] https://lore.kernel.org/linux-kselftest/vyllxuh5xbqmaoyl2mselebij5ox7cseekjcvl5gmzoxxwd2he@hxi4mpjanxzt/#t

This is already in mm-unstable so this is redundant, and as Liam explained,
his new v8 series will contain this fix too, and his old version will be
unwound before new applied, so at no time will this be relevant.

>
> History:
> V3:
> - no-functional change, incooperate feedback from Pedro Falcato
>
> V2:
> - https://lore.kernel.org/linux-kselftest/20240829214352.963001-1-jeffxu@chromium.org/
> - remove the mmap fix (Liam R. Howlett will fix it separately)
> - Add cover letter (Lorenzo Stoakes)

I think I asked for more than this :)

> - split the testcase for ease of review (Mark Brown)
>
> V1:
> - https://lore.kernel.org/linux-kselftest/20240828225522.684774-1-jeffxu@chromium.org/
>
>
> Jeff Xu (5):
>   selftests/mseal_test: Check vma_size, prot, error code.
>   selftests/mseal: add sealed madvise type
>   selftests/mseal: munmap across multiple vma ranges.
>   selftests/mseal: add more tests for mmap
>   selftests/mseal: add more tests for mremap
>
>  tools/testing/selftests/mm/mseal_test.c | 830 ++++++++++++++++++++++--
>  1 file changed, 763 insertions(+), 67 deletions(-)
>
> --
> 2.46.0.469.g59c65b2a67-goog
>

Overall having checked one patch in the series, I am quite concerned that
these tests are not testing what they suggest they do, are redundant, and I
have found numerous problems line-by-line.

I've also encountered copy/pasted blocks, comparing PROT_xxx fields to
magic numbers, and it generally feels really rushed.

I feel like it might be worth taking some time on the next respin to really
think carefully about how these functions work, checking man pages and
source, and getting some understanding of what it is we really need to
assert about mseal() behaviour.

We're here to help you and want to be collaborative and help get mseal()
into a good state, so I'd like to suggest taking your time on the next
respin to really improve the quality and think carefully in each instance
_what_ you are testing and _why_.

And don't be afraid to ask questions and discuss these things with
us. We're happy to help!

Anyway, I am now off on a long weekend before my birthday, I wish you a
great weekend and hope we can find a way to move forward constructively
with this! :)

Thanks, Lorenzo

Powered by blists - more mailing lists