On Fri, Aug 30, 2024 at 06:02:32PM GMT, jeffxu@xxxxxxxxxxxx wrote: > From: Jeff Xu <jeffxu@xxxxxxxxxxxx> > > 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@xxxxxxxxxxxx/ > - 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@xxxxxxxxxxxx/ > > > 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
