| Message ID | 20240830180237.1220027-5-jeffxu@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Increase mseal test coverage | expand |
On Sat, Sep 07, 2024 at 08:27:52PM GMT, Lorenzo Stoakes wrote: > On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote: > > On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote: > > > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote: > > > > > From: Jeff Xu <jeffxu@chromium.org> > > > > > > > > > > Add sealing test to cover mmap for > > > > > Expand/shrink across sealed vmas (MAP_FIXED) > > > > > Reuse the same address in !MAP_FIXED case. > > Hi Jeff, I really want to find a constructive way forward, but you've > basically ignored more or less everything I've said here. > > I could respond again to each of your points here, but - from my point of > view - if your response to 'what is this even testing?' is to just repeat > in effect the name of the test - we will be stuck in a loop, which will be > exited with a NACK. I don't want this. > > The majority of these tests, from a VMA/mmap point of view, appear to me to > be essentially testing 'does basic mmap functionality work correctly', > which isn't testing mseal. > > Look - I appreciate your commitment to testing (see my work on mm/vma.c - I > care passionately about testing) - but we must make sure we are actually > testing what we mean to. > > So I suggest as a constructive way forward - firstly, submit a regression > test for the change Liam wrapped into his series regarding the -EPERM > thing. > > This should go in uncontroversially, I will take the time to review it and > I don't see why that shouldn't be relatively straight forward. I will drop > the concerns about things like test macros etc. for that. > > Then after that, I suggest we have a discussion about - at a higher level - > what it is you want to test. And then between me, you, Liam and Pedro - > ahead of time, list out the tests that we want, with all of us reaching > consensus. Hi, I agree with most of the points. Sitting down here to write unofficial guidelines for mseal behavior. mseal should seal regions and mark them immutable, which means their protection and contents (ish?) (not _only_ backing mapping, but also contents in general (see madvise below)) should not be changed throughout the lifetime of the address space. For the general syscall interface, this means: 1) mprotect and munmap need to be blocked on mseal regions. 1a) munmap _cannot_ tolerate partial failure, per POSIX. 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM. 2) Most madvise calls are allowed, except for destructive operations on read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care about blocking these ops if we can do it manually with e.g memset) 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly different. We probably do want to block these on MAP_PRIVATE file mappings, as to block stuff like madvise MADV_DONTNEED on program rodata. 2b) We take into account pkeys when doing the permission checks. 3) mremap is not allowed as we'd change the "contents" of the old region. 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. We already informally allow expansion if e.g mmapping after it + mseal. 4) mlock and msync are allowed. 5) mseal is blocked. 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed. 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok. 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) should be disallowed (?) 7a) This currently does not happen, and seems like a large hole? But disallowing this would probably severely break ptrace if the ELF sealing plans come to fruition. When we say "disallowed", we usually (apart from munmap) allow for partial failure. This means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves into a corner - this is strictly "undefined behavior". The msealed regions themselves will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is also kind of a munmap-related test, and might not really be something we really want in the mseal tests) Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly does not make sense to block operations unless they affect a VMA entirely, and that would also force VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :("). Do I have everything right? Am I missing anything?
On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > Hi, > > I agree with most of the points. Sitting down here to write unofficial > guidelines for mseal behavior. > > mseal should seal regions and mark them immutable, which means their protection > and contents (ish?) (not _only_ backing mapping, but also contents in general > (see madvise below)) should not be changed throughout the lifetime of the address space. > > For the general syscall interface, this means: > 1) mprotect and munmap need to be blocked on mseal regions. > 1a) munmap _cannot_ tolerate partial failure, per POSIX. > 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM. > > 2) Most madvise calls are allowed, except for destructive operations on > read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care > about blocking these ops if we can do it manually with e.g memset) > 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly > different. We probably do want to block these on MAP_PRIVATE file mappings, as to block > stuff like madvise MADV_DONTNEED on program rodata. > 2b) We take into account pkeys when doing the permission checks. > > 3) mremap is not allowed as we'd change the "contents" of the old region. > 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. > We already informally allow expansion if e.g mmapping after it + mseal. > > 4) mlock and msync are allowed. > > 5) mseal is blocked. > > 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed. > 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok. > > 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) > should be disallowed (?) > 7a) This currently does not happen, and seems like a large hole? But disallowing this > would probably severely break ptrace if the ELF sealing plans come to fruition. > > When we say "disallowed", we usually (apart from munmap) allow for partial failure. This > means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting > of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves > into a corner - this is strictly "undefined behavior". The msealed regions themselves > will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is > also kind of a munmap-related test, and might not really be something we really want in the mseal tests) > > Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. > Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly > does not make sense to block operations unless they affect a VMA entirely, and that would also force > VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :("). > > > Do I have everything right? Am I missing anything? Small addendum: file write, truncate and hole punching might also matter for sealed file-backed regions, as these change the region's contents, but we probably want to rely on file write permissions to protect against this (as we already do). Any other solution is probably terrible and probably endlessly NAK'd by fs folks, but it does mean sealed regions aren't really immutable if you or the attacker can write to the file.
Hi Lorenzo On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote: > > On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote: > > > > On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote: > > > > > From: Jeff Xu <jeffxu@chromium.org> > > > > > > > > > > Add sealing test to cover mmap for > > > > > Expand/shrink across sealed vmas (MAP_FIXED) > > > > > Reuse the same address in !MAP_FIXED case. > > Hi Jeff, I really want to find a constructive way forward, but you've > basically ignored more or less everything I've said here. > > I could respond again to each of your points here, but - from my point of > view - if your response to 'what is this even testing?' is to just repeat > in effect the name of the test - we will be stuck in a loop, which will be > exited with a NACK. I don't want this. > > The majority of these tests, from a VMA/mmap point of view, appear to me to > be essentially testing 'does basic mmap functionality work correctly', > which isn't testing mseal. > > Look - I appreciate your commitment to testing (see my work on mm/vma.c - I > care passionately about testing) - but we must make sure we are actually > testing what we mean to. > I'm also passionate about testing :-) Maybe there is a mis-understanding ? I explained the purpose of this patch set in various responses, maybe not directly to your email though. 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. > So I suggest as a constructive way forward - firstly, submit a regression > test for the change Liam wrapped into his series regarding the -EPERM > thing. > I could work on this (to split the patch further) if this helps acceptance of the patch series. However, since the merge window is closer, everyone is busy, and it is not really urgent to get it merged. the added tests already passed in the linux-next branch, we could wait till after merge-window to review/perfect those tests. > This should go in uncontroversially, I will take the time to review it and > I don't see why that shouldn't be relatively straight forward. I will drop > the concerns about things like test macros etc. for that. > > Then after that, I suggest we have a discussion about - at a higher level - > what it is you want to test. And then between me, you, Liam and Pedro - > ahead of time, list out the tests that we want, with all of us reaching > consensus. > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > too - I may be missing something, but I cannot for the life me understand > why we have to assert negations only, and other self tests do not do this. > My most test-infra related comments comes from Muhammad Usama Anjum (added into this email), e.g. assert is not recommended.[1] , [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > I have replied to a few sample points below. > > All of us simply want to help make sure mseal works as well as it can, this > is the only motivation at play here. > > Hope you have a great weekend! > Thanks Hope a great weekend too ! -Jeff -Jeff > Cheers, Lorenzo > > > > > > > > > This commit message is woefully small. I told you on v1 to improve the > > > > commit messages. Linus has told you to do this before. > > > > > > > > Please actually respond to feedback. Thanks. > > > > > > > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > > > > --- > > > > > tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- > > > > > 1 file changed, 125 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c > > > > > index e855c8ccefc3..3516389034a7 100644 > > > > > --- a/tools/testing/selftests/mm/mseal_test.c > > > > > +++ b/tools/testing/selftests/mm/mseal_test.c > > > > > @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) > > > > > REPORT_TEST_PASS(); > > > > > } > > > > > > > > > > +static void test_seal_mmap_expand_seal_middle(bool seal) > > > > > > > > This test doesn't expand, doesn't do anything in the middle. It does mmap() > > > > though and relates to mseal, so that's something... this is compeltely > > > > misnamed and needs to be rethought. > > > > > > > > > > OK correction - it _seals_ in the middle. The remained of the criticism remains, > > > and this is rather confusing... and I continue to wonder what the purpose of > > > this is? > > > > > It expands the size (start from ptr). > > > > > > > +{ > > > > > + void *ptr; > > > > > + unsigned long page_size = getpagesize(); > > > > > + unsigned long size = 12 * page_size; > > > > > + int ret; > > > > > + void *ret2; > > > > > + int prot; > > > > > + > > > > > + setup_single_address(size, &ptr); > > > > > > > > Please replace every single instance of this with an mmap(). There's > > > > literally no reason to abstract it. And munmap() what you map. > > > > > > No, we need to abstract it. In addition to the mmap, it also > > allocates an additional two blocks before and after the allocated > > memory, to avoid auto-merging, so we can use get_vma_size. > > It doesn't? > > static void setup_single_address(int size, void **ptrOut) > { > void *ptr; > > ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); > *ptrOut = ptr; > } > > > > > > > > + FAIL_TEST_IF_FALSE(ptr != (void *)-1); > > > > > > > > Pretty sure Pedro pointed out you should be checking against MAP_FAILED > > > > here. I really don't understand why the rest of your test is full of > > > > mmap()'s but for some reason you choose to abstract this one call? What? > > > > > > > > > + /* ummap last 4 pages. */ > > > > > + ret = sys_munmap(ptr + 8 * page_size, 4 * page_size); > > > > > > > > sys_munmap()? What's wrong with munmap()? > > > > > > > > > + FAIL_TEST_IF_FALSE(!ret); > > > > > > > > Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy. > > > > > > > > Would be nice to have something human-readable like ASSERT_EQ() or > > > > ASSERT_TRUE() or ASSERT_FALSE(). > > > > > > ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The > > FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks > > related to self-test infra, such as count how many tests are failing. > > Can you please point me to where it says you should implement your own > macro that only tests the negation of an expression? > > I have found other self tests that do. > > > > > > > > + > > > > > + size = get_vma_size(ptr, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 8 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > + > > > > > + if (seal) { > > > > > + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(!ret); > > > > > + } > > > > > + > > > > > + /* use mmap to expand and overwrite (MAP_FIXED) */ > > > > > > > > You don't really need to say MAP_FIXED, it's below. > > > > > > Adding a comment here to help reviewers. > > > > > > > + ret2 = mmap(ptr, 12 * page_size, PROT_READ, > > > > > + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); > > > > > > > > Why read-only? > > > > > > > > You're not expanding you're overwriting. You're not doing anything in the > > > > middle. > > > > > > The MAP_FIXED is overwriting. It also expands the address range > > (start from ptr) from 8 to 12 pages. > > > > > > I'm again confused about what you think you're testing here. I don't think > > > > we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten > > > > VMA? > > > > > > > > You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() > > > > if that's what you want. > > > > > > > > > + if (seal) { > > > > > + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); > > > > > + FAIL_TEST_IF_FALSE(errno == EPERM); > > > > > + > > > > > + size = get_vma_size(ptr, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > + > > > > > + size = get_vma_size(ptr + 4 * page_size, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > + } else > > > > > + FAIL_TEST_IF_FALSE(ret2 == ptr); > > > > > > > > Don't do dangling else's after a big block. > > > > > > patch passed the checkpatch.pl for style check. > > > > > > > + > > > > > + REPORT_TEST_PASS(); > > > > > +} > > > > > + > > > > > +static void test_seal_mmap_shrink_seal_middle(bool seal) > > > > > > > > What's going on in the 'middle'? This test doesn't shrink, it overwrites > > > > the beginning of a sealed VMA? > > > > > > Correction - the middle is sealed. Other points remain. > > > > > The mmap attempts to shrink the address range from 12 pages to 8 pages. > > > > > > > +{ > > > > > + void *ptr; > > > > > + unsigned long page_size = getpagesize(); > > > > > + unsigned long size = 12 * page_size; > > > > > + int ret; > > > > > + void *ret2; > > > > > + int prot; > > > > > + > > > > > + setup_single_address(size, &ptr); > > > > > + FAIL_TEST_IF_FALSE(ptr != (void *)-1); > > > > > + > > > > > + if (seal) { > > > > > + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(!ret); > > > > > + } > > > > > + > > > > > + /* use mmap to shrink and overwrite (MAP_FIXED) */ > > > > > > > > What exactly are you shrinking? You're overwriting the start of the vma? > > > > > > > > What is this testing that is different from the previous test? This seems > > > > useless honestly. > > > > > > Again, as above, one test is expanding, the other test is shrinking. > > Please take a look at mmap parameters and steps before mmap call. > > > > > > > > > + ret2 = mmap(ptr, 7 * page_size, PROT_READ, > > > > > + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); > > > > > + if (seal) { > > > > > + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); > > > > > + FAIL_TEST_IF_FALSE(errno == EPERM); > > > > > + > > > > > + size = get_vma_size(ptr, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > > > > What the hell is this comparison to magic numbers? This is > > > > ridiculous. What's wrong with PROT_xxx?? > > > > > > The PROT_xxx can't be used here. > > get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different. > > > > > > > + > > > > > + size = get_vma_size(ptr + 4 * page_size, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > + > > > > > + size = get_vma_size(ptr + 4 * page_size, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == 4 * page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > > > > Err dude, you're doing this twice? > > > > > > The second get_vma_size should be (ptr + 8 * page_size) > > I will update that. > > > > > > So what are we testing here exactly? That we got a VMA split? This is > > > > err... why are we asserting this? > > > > > > I guess, that we can't overwrite a sealed bit of a VMA at the end. But again > > > this feels entirely redundant. For this kind of thing to fail would mean the > > > whole VMA machinery is broken. > > > > > The test is testing mmap(MAP_FIXED), since it can be used to overwrite > > the sealed memory range (without sealing), then there is a variant of > > expand/shrink. > > > > > > > > > > > > > + } else > > > > > + FAIL_TEST_IF_FALSE(ret2 == ptr); > > > > > + > > > > > + REPORT_TEST_PASS(); > > > > > +} > > > > > + > > > > > +static void test_seal_mmap_reuse_addr(bool seal) > > > > > > > > This is wrong, you're not reusing anything. This test is useless. > > > > > > The ptr is reused as a hint. > > > > > > > +{ > > > > > + void *ptr; > > > > > + unsigned long page_size = getpagesize(); > > > > > + unsigned long size = page_size; > > > > > + int ret; > > > > > + void *ret2; > > > > > + int prot; > > > > > + > > > > > + setup_single_address(size, &ptr); > > > > > + FAIL_TEST_IF_FALSE(ptr != (void *)-1); > > > > > + > > > > > + if (seal) { > > > > > + ret = sys_mseal(ptr, size); > > > > > + FAIL_TEST_IF_FALSE(!ret); > > > > > > > > We could avoid this horrid ret, ret2 naming if you just did: > > > > > > > > FAIL_TEST_IF_FALSE(sys_mseal(ptr, size)); > > > > > > > > > + } > > > > > + > > > > > + /* use mmap to change protection. */ > > > > > + ret2 = mmap(ptr, size, PROT_NONE, > > > > > + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); > > > > > > > > How are you using mmap to change the protection when you're providing a > > > > hint to the address to use? You're not changing any protection at all! > > > > > > It is necessary to add the this tests to make sure mseal is behave as > > it should be, which is !MAP_FIXED case, new address will be allocated, > > instead of fail of mmap() > > > > > > > > You're allocating an entirely new VMA hinting that you want it near > > > > ptr. Please read the man page for mmap(): > > > > > > > > If addr is NULL, then the kernel chooses the (page-aligned) address > > > > at which to create the mapping; this is the most portable method of > > > > creating a new mapping. If addr is not NULL, then the kernel takes > > > > it as a hint about where to place the mapping; on Linux, the kernel > > > > will pick a nearby page boundary (but always above or equal to the > > > > value specified by /proc/sys/vm/mmap_min_addr) and attempt to create > > > > the mapping there. If another mapping already exists there, the > > > > kernel picks a new address that may or may not depend on the hint. > > > > The address of the new mapping is returned as the result of the > > > > call. > > > > > > > > > + > > > > > + /* MAP_FIXED is not used, expect new addr */ > > > > > + FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED)); > > > > > > > > This is beyond horrible. You really have to add more asserts. > > > > > > Again assert is not recommended by self_test > > > > > > Also you're expecting a new address here, so again, what on earth are you > > > > asserting? That we can mmap()? > > > > > > > > > + FAIL_TEST_IF_FALSE(ret2 != ptr); > > > > > + > > > > > + size = get_vma_size(ptr, &prot); > > > > > + FAIL_TEST_IF_FALSE(size == page_size); > > > > > + FAIL_TEST_IF_FALSE(prot == 0x4); > > > > > + > > > > > + REPORT_TEST_PASS(); > > > > > +} > > > > > + > > > > > int main(int argc, char **argv) > > > > > { > > > > > bool test_seal = seal_support(); > > > > > @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) > > > > > if (!get_vma_size_supported()) > > > > > ksft_exit_skip("get_vma_size not supported\n"); > > > > > > > > > > - ksft_set_plan(91); > > > > > + ksft_set_plan(97); > > > > > > > > I'm guessing this is the number of tests, but I mean this is horrible. Is > > > > there not a better way of doing this? > > > > > > Again, this is recommended by self-test. > > > > > > > > > > > > > > > > test_seal_addseal(); > > > > > test_seal_unmapped_start(); > > > > > @@ -2357,5 +2474,12 @@ int main(int argc, char **argv) > > > > > test_munmap_free_multiple_ranges(false); > > > > > test_munmap_free_multiple_ranges(true); > > > > > > > > > > + test_seal_mmap_expand_seal_middle(false); > > > > > + test_seal_mmap_expand_seal_middle(true); > > > > > + test_seal_mmap_shrink_seal_middle(false); > > > > > + test_seal_mmap_shrink_seal_middle(true); > > > > > + test_seal_mmap_reuse_addr(false); > > > > > + test_seal_mmap_reuse_addr(true); > > > > > + > > > > > ksft_finished(); > > > > > } > > > > > -- > > > > > 2.46.0.469.g59c65b2a67-goog > > > > >
Hi Pedro On Sun, Sep 8, 2024 at 2:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > > I agree with most of the points. Sitting down here to write unofficial > guidelines for mseal behavior. > > mseal should seal regions and mark them immutable, which means their protection > and contents (ish?) (not _only_ backing mapping, but also contents in general > (see madvise below)) should not be changed throughout the lifetime of the address space. > > For the general syscall interface, this means: > 1) mprotect and munmap need to be blocked on mseal regions. > 1a) munmap _cannot_ tolerate partial failure, per POSIX. > 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM. > > 2) Most madvise calls are allowed, except for destructive operations on > read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care > about blocking these ops if we can do it manually with e.g memset) > 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly > different. We probably do want to block these on MAP_PRIVATE file mappings, as to block > stuff like madvise MADV_DONTNEED on program rodata. > 2b) We take into account pkeys when doing the permission checks. > > 3) mremap is not allowed as we'd change the "contents" of the old region. > 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. > We already informally allow expansion if e.g mmapping after it + mseal. > > 4) mlock and msync are allowed. > > 5) mseal is blocked. mseal is not blocked, i.e. seal on an already sealed memory is no-op. This is described in mseal.rst [1] [1] https://github.com/torvalds/linux/blob/master/Documentation/userspace-api/mseal.rst > > 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed. > 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok. > > 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) > should be disallowed (?) > 7a) This currently does not happen, and seems like a large hole? But disallowing this > would probably severely break ptrace if the ELF sealing plans come to fruition. > Jann Horn pointed out FOLL_FORCE during RFC [2], and this is in mseal.rst too. In short, FOLL_FORCE is not covered by mseal. On ChromeOS, FOLL_FORCE is disabled. Recently, Adrian Ratiu upstreamed that [3] [2] https://lore.kernel.org/lkml/CAG48ez3ShUYey+ZAFsU2i1RpQn0a5eOs2hzQ426FkcgnfUGLvA@mail.gmail.com/ [3] https://lore.kernel.org/lkml/20240802080225.89408-1-adrian.ratiu@collabora.com/ -Jeff > When we say "disallowed", we usually (apart from munmap) allow for partial failure. This > means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting > of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves > into a corner - this is strictly "undefined behavior". The msealed regions themselves > will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is > also kind of a munmap-related test, and might not really be something we really want in the mseal tests) > > Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. > Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly > does not make sense to block operations unless they affect a VMA entirely, and that would also force > VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :("). > > > Do I have everything right? Am I missing anything? > > -- > Pedro
Hi Pedro On Sun, Sep 8, 2024 at 2:56 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > > On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato <pedro.falcato@gmail.com> wrote: > > Hi, > > > > I agree with most of the points. Sitting down here to write unofficial > > guidelines for mseal behavior. > > > > mseal should seal regions and mark them immutable, which means their protection > > and contents (ish?) (not _only_ backing mapping, but also contents in general > > (see madvise below)) should not be changed throughout the lifetime of the address space. > > > > For the general syscall interface, this means: > > 1) mprotect and munmap need to be blocked on mseal regions. > > 1a) munmap _cannot_ tolerate partial failure, per POSIX. > > 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM. > > > > 2) Most madvise calls are allowed, except for destructive operations on > > read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care > > about blocking these ops if we can do it manually with e.g memset) > > 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly > > different. We probably do want to block these on MAP_PRIVATE file mappings, as to block > > stuff like madvise MADV_DONTNEED on program rodata. > > 2b) We take into account pkeys when doing the permission checks. > > > > 3) mremap is not allowed as we'd change the "contents" of the old region. > > 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. > > We already informally allow expansion if e.g mmapping after it + mseal. > > > > 4) mlock and msync are allowed. > > > > 5) mseal is blocked. > > > > 6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed. > > 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok. > > > > 7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) > > should be disallowed (?) > > 7a) This currently does not happen, and seems like a large hole? But disallowing this > > would probably severely break ptrace if the ELF sealing plans come to fruition. > > > > When we say "disallowed", we usually (apart from munmap) allow for partial failure. This > > means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting > > of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves > > into a corner - this is strictly "undefined behavior". The msealed regions themselves > > will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is > > also kind of a munmap-related test, and might not really be something we really want in the mseal tests) > > > > Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. > > Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly > > does not make sense to block operations unless they affect a VMA entirely, and that would also force > > VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :("). > > > > > > Do I have everything right? Am I missing anything? > > Small addendum: file write, truncate and hole punching might also > matter for sealed file-backed regions, as these change the region's > contents, but we probably > want to rely on file write permissions to protect against this (as we > already do). Any other solution is probably terrible and probably > endlessly NAK'd by fs folks, but it does > mean sealed regions aren't really immutable if you or the attacker can > write to the file. > Right. The mseal protects the control-data of VMA (e.g. prot), it doesn't do anything more than that. The file permission relies on dac/mac control. -Jeff > -- > Pedro
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.
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
Hi Lorenzo and Muhammad Reviving this thread since the merging window is closed and we have more time to review /work on this code in the next few weeks. On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > Hi Lorenzo > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > too - I may be missing something, but I cannot for the life me understand > > why we have to assert negations only, and other self tests do not do this. > > > My most test-infra related comments comes from Muhammad Usama Anjum > (added into this email), e.g. assert is not recommended.[1] , > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE Muhammad Usama Anjum doesn't want assert being used in selftest (see [1] above), and I quote: "We don't want to terminate the test if one test fails because of assert. We want the sub-tests to get executed in-dependent of other tests. ksft_test_result(condition, fmt, ...); ksft_test_result_pass(fmt, ...);" FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and replacement of assert. Please let me know if you have questions on this and Muhammad might also help to clarify the requirement if needed. Thanks -Jeff
On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > Hi Lorenzo and Muhammad > > Reviving this thread since the merging window is closed and we have > more time to review /work on this code in the next few weeks. > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > Hi Lorenzo > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > too - I may be missing something, but I cannot for the life me understand > > > why we have to assert negations only, and other self tests do not do this. > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > (added into this email), e.g. assert is not recommended.[1] , > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > [1] above), and I quote: > "We don't want to terminate the test if one test fails because of assert. We > want the sub-tests to get executed in-dependent of other tests. > > ksft_test_result(condition, fmt, ...); > ksft_test_result_pass(fmt, ...);" > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > replacement of assert. > > Please let me know if you have questions on this and Muhammad might > also help to clarify the requirement if needed. > > Thanks > -Jeff Right this is about not failing the test i.e. equivalent of an expect rather than an assert, which makes sense. What I'm saying is we should have something more like EXPECT_TRUE() EXPECT_FALSE() etc. Which would avoid these confusing FAIL_TEST_IF_FALSE(!expr) Things. Hopefully that's clear? Thanks!
On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > Hi Lorenzo and Muhammad > > > > Reviving this thread since the merging window is closed and we have > > more time to review /work on this code in the next few weeks. > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > > > Hi Lorenzo > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > > too - I may be missing something, but I cannot for the life me understand > > > > why we have to assert negations only, and other self tests do not do this. > > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > > [1] above), and I quote: > > "We don't want to terminate the test if one test fails because of assert. We > > want the sub-tests to get executed in-dependent of other tests. > > > > ksft_test_result(condition, fmt, ...); > > ksft_test_result_pass(fmt, ...);" > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > replacement of assert. > > > > Please let me know if you have questions on this and Muhammad might > > also help to clarify the requirement if needed. > > > > Thanks > > -Jeff > > Right this is about not failing the test i.e. equivalent of an expect > rather than an assert, which makes sense. > > What I'm saying is we should have something more like > > EXPECT_TRUE() > EXPECT_FALSE() > > etc. > > Which would avoid these confusing > > FAIL_TEST_IF_FALSE(!expr) FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. It is same syntax as assert(expr), e.g: man assert(expr) assert - abort the program if assertion is false FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting the program, it just reports failure in this test. Is this still confusing ? (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax of assert is well known.) > > Things. > > Hopefully that's clear? Thanks!
On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote: > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > > Hi Lorenzo and Muhammad > > > > > > Reviving this thread since the merging window is closed and we have > > > more time to review /work on this code in the next few weeks. > > > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > > > > > Hi Lorenzo > > > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > > > too - I may be missing something, but I cannot for the life me understand > > > > > why we have to assert negations only, and other self tests do not do this. > > > > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > > > [1] above), and I quote: > > > "We don't want to terminate the test if one test fails because of assert. We > > > want the sub-tests to get executed in-dependent of other tests. > > > > > > ksft_test_result(condition, fmt, ...); > > > ksft_test_result_pass(fmt, ...);" > > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > > replacement of assert. > > > > > > Please let me know if you have questions on this and Muhammad might > > > also help to clarify the requirement if needed. > > > > > > Thanks > > > -Jeff > > > > Right this is about not failing the test i.e. equivalent of an expect > > rather than an assert, which makes sense. > > > > What I'm saying is we should have something more like > > > > EXPECT_TRUE() > > EXPECT_FALSE() > > > > etc. > > > > Which would avoid these confusing > > > > FAIL_TEST_IF_FALSE(!expr) > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. But you don't only test position conditions, you also test negative ones. 'Fail test if false false thing' is really confusing and hard to read. I struggle to understand your tests as a result. I understand 'fail test if false' is expressive in a way, but it's really hard to parse. Obviously it's also misleading in that you're saying 'fail the test _later_ if false', which I hadn't even realised... It's well established in basically all normal test suites that: * assert = fail test _here_ if this fails (actually a valid thing to do if you assert something that means the test simply cannot reasonably continue if that condition is false). * expect = the test will now fail, but carry on. I mean you work for a company that does this :) [0] this is a very well established precedent. [0]:https://github.com/google/googletest > > It is same syntax as assert(expr), e.g: > > man assert(expr) > assert - abort the program if assertion is false > > FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting > the program, it just reports failure in this test. So doesn't at all do what assert does, because that _does_ terminate execution on failure... We are writing unit tests in a test framework, let's use very well established industry practices please. Also note that you don't even need to reinvent the wheel, there is a fully-featured test harness available in tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and EXPECT_xxx() helpers. I've used it extensively myself and it works well. I'd basically suggest you use that. Though moving existing tests to that would be some churn. On the other hand I really can't accept patches which are totally unreadable to me, so you'll need to fix this one way or another, and the churn is worth it as a one-time cost to be honest. > > Is this still confusing ? > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax > of assert is well known.) It's a super misleading name as it says nothing about _WHEN_ the test fails. Also the syntax of assert() may be well known but you don't call this function assert, you don't reference assert anywhere, and you don't do what assert() does so, you know, That's not a great example. The semantics of unit test frameworks are very well known, and already implemented for you, and also do not require you to do unreadable double negations for no reason, so let's use those please. > > > > > > Things. > > > > Hopefully that's clear? Thanks!
On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote: > > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > > > Hi Lorenzo and Muhammad > > > > > > > > Reviving this thread since the merging window is closed and we have > > > > more time to review /work on this code in the next few weeks. > > > > > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > > > > > > > Hi Lorenzo > > > > > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > > > > too - I may be missing something, but I cannot for the life me understand > > > > > > why we have to assert negations only, and other self tests do not do this. > > > > > > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > > > > [1] above), and I quote: > > > > "We don't want to terminate the test if one test fails because of assert. We > > > > want the sub-tests to get executed in-dependent of other tests. > > > > > > > > ksft_test_result(condition, fmt, ...); > > > > ksft_test_result_pass(fmt, ...);" > > > > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > > > replacement of assert. > > > > > > > > Please let me know if you have questions on this and Muhammad might > > > > also help to clarify the requirement if needed. > > > > > > > > Thanks > > > > -Jeff > > > > > > Right this is about not failing the test i.e. equivalent of an expect > > > rather than an assert, which makes sense. > > > > > > What I'm saying is we should have something more like > > > > > > EXPECT_TRUE() > > > EXPECT_FALSE() > > > > > > etc. > > > > > > Which would avoid these confusing > > > > > > FAIL_TEST_IF_FALSE(!expr) > > > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. > > But you don't only test position conditions, you also test negative ones. > So it is not a problem with the MACRO, but where is it used ? ret = sys_mseal(ptr, size); FAIL_TEST_IF_FALSE(!ret); Take this example, it would be assert(!ret) The syscall return usually returns 0 to indicate success, where a negative comes from, but dev should be so used to (!ret), it is a common pattern to check syscall return code. e.g assert(!ret) Or do you have specific examples of code that caused confusion ? > 'Fail test if false false thing' is really confusing and hard to read. > > I struggle to understand your tests as a result. > > I understand 'fail test if false' is expressive in a way, but it's really hard > to parse. > If you just read it as assert(), would that be easier ? (or you don't like assert() ?) > Obviously it's also misleading in that you're saying 'fail the test _later_ > if false', which I hadn't even realised... > > It's well established in basically all normal test suites that: > > * assert = fail test _here_ if this fails (actually a valid thing to do if > you assert something that means the test simply cannot > reasonably continue if that condition is false). > * expect = the test will now fail, but carry on. > > I mean you work for a company that does this :) [0] this is a very well > established precedent. > > [0]:https://github.com/google/googletest > Let's use expect as an example, let's say I create a new Macro: TEST_EXPECT_TRUE, which basically is same syntax as FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have !ret in the code. ret = sys_mseal(ptr, size); TEST_EXPECT_TRUE(!ret); Or is the FAIL_xxx_IF_FALSE pattern more un-readable than EXPECT_TURE ? maybe .. > > > > It is same syntax as assert(expr), e.g: > > > > man assert(expr) > > assert - abort the program if assertion is false > > > > FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting > > the program, it just reports failure in this test. > > So doesn't at all do what assert does, because that _does_ terminate > execution on failure... > I don't know what you mean, the test case will fail, but the next test case will run. This the point, the mseal_test continues to run all test cases to finish, even if one of the test cases is failed. > We are writing unit tests in a test framework, let's use very well > established industry practices please. > > Also note that you don't even need to reinvent the wheel, there is a > fully-featured test harness available in > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > EXPECT_xxx() helpers. > The EXPECT_xxx() doesn't take care of reporting though, or maybe it needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't spent much time on those, but on brief look, it seems it is for repeating some tests, which doesn't exactly fit into what I needed, e.g. the sealed memory won't be unmapped. It is possible that those tests can be adopted to use test fixtures, but I don't see significant gain for that. > I've used it extensively myself and it works well. > > I'd basically suggest you use that. Though moving existing tests to that > would be some churn. > > On the other hand I really can't accept patches which are totally > unreadable to me, so you'll need to fix this one way or another, and the > churn is worth it as a one-time cost to be honest. > > > > > Is this still confusing ? > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax > > of assert is well known.) > > It's a super misleading name as it says nothing about _WHEN_ the test > fails. Also the syntax of assert() may be well known but you don't call > this function assert, you don't reference assert anywhere, and you don't do what > assert() does so, you know, That's not a great example. > > The semantics of unit test frameworks are very well known, and already > implemented for you, and also do not require you to do unreadable double > negations for no reason, so let's use those please. > As stated previously, I'm not sure whether the test fixture is benefiting mseal_test at this moment. But I'm open for future conversion when I have time for this. For now, I like to get those tests in so we can properly detect possible regression for memory sealing. What will help you better review this code? Would the below help ? s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g > > > > > > > > > > Things. > > > > > > Hopefully that's clear? Thanks!
On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote: > > > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > > > > Hi Lorenzo and Muhammad > > > > > > > > > > Reviving this thread since the merging window is closed and we have > > > > > more time to review /work on this code in the next few weeks. > > > > > > > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > > > > > > > > > Hi Lorenzo > > > > > > > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > > > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > > > > > too - I may be missing something, but I cannot for the life me understand > > > > > > > why we have to assert negations only, and other self tests do not do this. > > > > > > > > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > > > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > > > > > [1] above), and I quote: > > > > > "We don't want to terminate the test if one test fails because of assert. We > > > > > want the sub-tests to get executed in-dependent of other tests. > > > > > > > > > > ksft_test_result(condition, fmt, ...); > > > > > ksft_test_result_pass(fmt, ...);" > > > > > > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > > > > replacement of assert. > > > > > > > > > > Please let me know if you have questions on this and Muhammad might > > > > > also help to clarify the requirement if needed. > > > > > > > > > > Thanks > > > > > -Jeff > > > > > > > > Right this is about not failing the test i.e. equivalent of an expect > > > > rather than an assert, which makes sense. > > > > > > > > What I'm saying is we should have something more like > > > > > > > > EXPECT_TRUE() > > > > EXPECT_FALSE() > > > > > > > > etc. > > > > > > > > Which would avoid these confusing > > > > > > > > FAIL_TEST_IF_FALSE(!expr) > > > > > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. > > > > But you don't only test position conditions, you also test negative ones. > > > So it is not a problem with the MACRO, but where is it used ? > > ret = sys_mseal(ptr, size); > FAIL_TEST_IF_FALSE(!ret); > > Take this example, it would be > assert(!ret) > > The syscall return usually returns 0 to indicate success, where a > negative comes from, but dev should be so used to (!ret), it is a > common pattern to check syscall return code. e.g assert(!ret) > > Or do you have specific examples of code that caused confusion ? > > > > 'Fail test if false false thing' is really confusing and hard to read. > > > > I struggle to understand your tests as a result. > > > > I understand 'fail test if false' is expressive in a way, but it's really hard > > to parse. > > > If you just read it as assert(), would that be easier ? (or you don't > like assert() ?) > > > Obviously it's also misleading in that you're saying 'fail the test _later_ > > if false', which I hadn't even realised... > > > > It's well established in basically all normal test suites that: > > > > * assert = fail test _here_ if this fails (actually a valid thing to do if > > you assert something that means the test simply cannot > > reasonably continue if that condition is false). > > * expect = the test will now fail, but carry on. > > > > I mean you work for a company that does this :) [0] this is a very well > > established precedent. > > > > [0]:https://github.com/google/googletest > > > Let's use expect as an example, let's say I create a new Macro: > TEST_EXPECT_TRUE, which basically is same syntax as > FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have > !ret in the code. > > ret = sys_mseal(ptr, size); > TEST_EXPECT_TRUE(!ret); > > Or is the FAIL_xxx_IF_FALSE pattern more un-readable than EXPECT_TURE > ? maybe .. > > > > > > > It is same syntax as assert(expr), e.g: > > > > > > man assert(expr) > > > assert - abort the program if assertion is false > > > > > > FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting > > > the program, it just reports failure in this test. > > > > So doesn't at all do what assert does, because that _does_ terminate > > execution on failure... > > > I don't know what you mean, the test case will fail, but the next test > case will run. This the point, the mseal_test continues to run all > test cases to finish, even if one of the test cases is failed. > > > We are writing unit tests in a test framework, let's use very well > > established industry practices please. > > > > Also note that you don't even need to reinvent the wheel, there is a > > fully-featured test harness available in > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > > EXPECT_xxx() helpers. > > > The EXPECT_xxx() doesn't take care of reporting though, or maybe it > needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't > spent much time on those, but on brief look, it seems it is for > repeating some tests, which doesn't exactly fit into what I needed, > e.g. the sealed memory won't be unmapped. > It is possible that those tests can be adopted to use test fixtures, > but I don't see significant gain for that. > > > I've used it extensively myself and it works well. > > > > I'd basically suggest you use that. Though moving existing tests to that > > would be some churn. > > > > On the other hand I really can't accept patches which are totally > > unreadable to me, so you'll need to fix this one way or another, and the > > churn is worth it as a one-time cost to be honest. > > > > > > > > Is this still confusing ? > > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax > > > of assert is well known.) > > > > It's a super misleading name as it says nothing about _WHEN_ the test > > fails. Also the syntax of assert() may be well known but you don't call > > this function assert, you don't reference assert anywhere, and you don't do what > > assert() does so, you know, That's not a great example. > > > > The semantics of unit test frameworks are very well known, and already > > implemented for you, and also do not require you to do unreadable double > > negations for no reason, so let's use those please. > > > As stated previously, I'm not sure whether the test fixture is > benefiting mseal_test at this moment. But I'm open for future > conversion when I have time for this. For now, I like to get those > tests in so we can properly detect possible regression for memory > sealing. > > What will help you better review this code? Would the below help ? > > s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g Jeff, you're falling into your usual pattern of being unreasonably argumentative for apparently no reason and I really don't have time to constantly respond inline when you're just ignoring what I tell you. You do this on nearly all code review and this just isn't working. If you want to effectively be involved in mseal you need to LISTEN. The more you do this the less patient everybody gets with you and the less likely your series will ever get merged. This is not good for mseal or anybody involved. On this issue - either use sensible macros that YOU ARE DEFINING, not assert.h, but YOU, that allow you to evaluate whether a condition is true or false - or I will not accept your unreadable test code. It's that simple and I'm done discussing this.
On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > So it is not a problem with the MACRO, but where is it used ? > ret = sys_mseal(ptr, size); > FAIL_TEST_IF_FALSE(!ret); > Take this example, it would be > assert(!ret) The problem is that the macro name is confusing and not terribly consistent with how the rest of the selftests work. The standard kselftest result reporting is ksft_test_result(bool result, char *name_format, ...); so the result of the test is a boolean flag which is passed in. This macro on the other hand sounds like a double negative so you have to stop and think what the logic is, and it's not seen anywhere else so nobody is going to be familiar with it. The main thing this is doing is burying a return statement in there, that's a bit surprising too. I'll also note that these macros are resulting in broken kselftest output, the name for a test has to be stable for automated systems to be able to associate test results between runs but these print ksft_test_result_fail("%s: line:%d\n", \ __func__, __LINE__); \ return; \ which includes the line number of the test in the name which is an obvious problem, automated systems won't be able to tell that any two failures are related to each other never mind the passing test. We should report why things failed but it's better to do that with a ksft_print_msg(), ideally one that's directly readable rather than requiring someone to go into the source code and look it up. A more standard way to write what you've got here would be to have the tests return a bool then have a runner loop which iterates over the tests: struct { char *name; bool (*func)(void); } tests[]; ... for (i = 0; i < ARRAY_SIZE(tests); i++) ksft_test_result(tests[i].test(), tests[i].name); then the tests can just have explicit return statements and don't need to worry about logging anything other than diagnostics. Depending on how much you need to share between tests you might also be able to use kselftest_harness.h which fork()s each test into a separate child and allows you to just fault to fail if that's easier. > > We are writing unit tests in a test framework, let's use very well > > established industry practices please. Plus also the fact that we have a framework here... > > Also note that you don't even need to reinvent the wheel, there is a > > fully-featured test harness available in > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > > EXPECT_xxx() helpers. > The EXPECT_xxx() doesn't take care of reporting though, or maybe it I rather think people would've noticed if the test harness was so broken that it was unable to report failures. If it is that broken we should fix it rather than open coding something else.
HHi Lorenzo On Thu, Oct 17, 2024 at 11:38 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > > On Thu, Oct 17, 2024 at 12:00 PM Lorenzo Stoakes > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > On Thu, Oct 17, 2024 at 11:47:15AM -0700, Jeff Xu wrote: > > > > On Thu, Oct 17, 2024 at 11:29 AM Lorenzo Stoakes > > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > On Thu, Oct 17, 2024 at 11:14:20AM -0700, Jeff Xu wrote: > > > > > > Hi Lorenzo and Muhammad > > > > > > > > > > > > Reviving this thread since the merging window is closed and we have > > > > > > more time to review /work on this code in the next few weeks. > > > > > > > > > > > > On Fri, Sep 13, 2024 at 3:50 PM Jeff Xu <jeffxu@chromium.org> wrote: > > > > > > > > > > > > > > Hi Lorenzo > > > > > > > > > > > > > > On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes > > > > > > > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point > > > > > > > > too - I may be missing something, but I cannot for the life me understand > > > > > > > > why we have to assert negations only, and other self tests do not do this. > > > > > > > > > > > > > > > My most test-infra related comments comes from Muhammad Usama Anjum > > > > > > > (added into this email), e.g. assert is not recommended.[1] , > > > > > > > > > > > > > > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/ > > > > > > > > > > > > > Specifically regarding Lorenzo's comments about FAIL_TEST_IF_FALSE > > > > > > > > > > > > Muhammad Usama Anjum doesn't want assert being used in selftest (see > > > > > > [1] above), and I quote: > > > > > > "We don't want to terminate the test if one test fails because of assert. We > > > > > > want the sub-tests to get executed in-dependent of other tests. > > > > > > > > > > > > ksft_test_result(condition, fmt, ...); > > > > > > ksft_test_result_pass(fmt, ...);" > > > > > > > > > > > > FAIL_TEST_IF_FALSE is a wrapper for ksft_test_result macro, and > > > > > > replacement of assert. > > > > > > > > > > > > Please let me know if you have questions on this and Muhammad might > > > > > > also help to clarify the requirement if needed. > > > > > > > > > > > > Thanks > > > > > > -Jeff > > > > > > > > > > Right this is about not failing the test i.e. equivalent of an expect > > > > > rather than an assert, which makes sense. > > > > > > > > > > What I'm saying is we should have something more like > > > > > > > > > > EXPECT_TRUE() > > > > > EXPECT_FALSE() > > > > > > > > > > etc. > > > > > > > > > > Which would avoid these confusing > > > > > > > > > > FAIL_TEST_IF_FALSE(!expr) > > > > > > > > FAIL_TEST_IF_FALSE(expr) is the right way to use this macro. > > > > > > But you don't only test position conditions, you also test negative ones. > > > > > So it is not a problem with the MACRO, but where is it used ? > > > > ret = sys_mseal(ptr, size); > > FAIL_TEST_IF_FALSE(!ret); > > > > Take this example, it would be > > assert(!ret) > > > > The syscall return usually returns 0 to indicate success, where a > > negative comes from, but dev should be so used to (!ret), it is a > > common pattern to check syscall return code. e.g assert(!ret) > > > > Or do you have specific examples of code that caused confusion ? > > > > > > > 'Fail test if false false thing' is really confusing and hard to read. > > > > > > I struggle to understand your tests as a result. > > > > > > I understand 'fail test if false' is expressive in a way, but it's really hard > > > to parse. > > > > > If you just read it as assert(), would that be easier ? (or you don't > > like assert() ?) > > > > > Obviously it's also misleading in that you're saying 'fail the test _later_ > > > if false', which I hadn't even realised... > > > > > > It's well established in basically all normal test suites that: > > > > > > * assert = fail test _here_ if this fails (actually a valid thing to do if > > > you assert something that means the test simply cannot > > > reasonably continue if that condition is false). > > > * expect = the test will now fail, but carry on. > > > > > > I mean you work for a company that does this :) [0] this is a very well > > > established precedent. > > > > > > [0]:https://github.com/google/googletest > > > > > Let's use expect as an example, let's say I create a new Macro: > > TEST_EXPECT_TRUE, which basically is same syntax as > > FAIL_TEST_IF_FALSE, I'm not sure how it is different: you still have > > !ret in the code. > > > > ret = sys_mseal(ptr, size); > > TEST_EXPECT_TRUE(!ret); > > > > Or is the FAIL_xxx_IF_FALSE pattern more un-readable than EXPECT_TURE > > ? maybe .. > > > > > > > > > > It is same syntax as assert(expr), e.g: > > > > > > > > man assert(expr) > > > > assert - abort the program if assertion is false > > > > > > > > FAIL_TEST_IF_FALSE is a replacement for assert, instead of aborting > > > > the program, it just reports failure in this test. > > > > > > So doesn't at all do what assert does, because that _does_ terminate > > > execution on failure... > > > > > I don't know what you mean, the test case will fail, but the next test > > case will run. This the point, the mseal_test continues to run all > > test cases to finish, even if one of the test cases is failed. > > > > > We are writing unit tests in a test framework, let's use very well > > > established industry practices please. > > > > > > Also note that you don't even need to reinvent the wheel, there is a > > > fully-featured test harness available in > > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > > > EXPECT_xxx() helpers. > > > > > The EXPECT_xxx() doesn't take care of reporting though, or maybe it > > needs to be combined with FIXTURE_SETUP, FIXTURE_TEARDOWN. I haven't > > spent much time on those, but on brief look, it seems it is for > > repeating some tests, which doesn't exactly fit into what I needed, > > e.g. the sealed memory won't be unmapped. > > It is possible that those tests can be adopted to use test fixtures, > > but I don't see significant gain for that. > > > > > I've used it extensively myself and it works well. > > > > > > I'd basically suggest you use that. Though moving existing tests to that > > > would be some churn. > > > > > > On the other hand I really can't accept patches which are totally > > > unreadable to me, so you'll need to fix this one way or another, and the > > > churn is worth it as a one-time cost to be honest. > > > > > > > > > > > Is this still confusing ? > > > > (The FAIL_TEST_IF_FALSE is already a descriptive name, and the syntax > > > > of assert is well known.) > > > > > > It's a super misleading name as it says nothing about _WHEN_ the test > > > fails. Also the syntax of assert() may be well known but you don't call > > > this function assert, you don't reference assert anywhere, and you don't do what > > > assert() does so, you know, That's not a great example. > > > > > > The semantics of unit test frameworks are very well known, and already > > > implemented for you, and also do not require you to do unreadable double > > > negations for no reason, so let's use those please. > > > > > As stated previously, I'm not sure whether the test fixture is > > benefiting mseal_test at this moment. But I'm open for future > > conversion when I have time for this. For now, I like to get those > > tests in so we can properly detect possible regression for memory > > sealing. > > > > What will help you better review this code? Would the below help ? > > > > s/FAIL_TEST_IF_FALSE/TEST_EXPECT_TRUE/g > > Jeff, you're falling into your usual pattern of being unreasonably > argumentative for apparently no reason and I really don't have time to > constantly respond inline when you're just ignoring what I tell you. > > You do this on nearly all code review and this just isn't working. If you > want to effectively be involved in mseal you need to LISTEN. > > The more you do this the less patient everybody gets with you and the less > likely your series will ever get merged. This is not good for mseal or > anybody involved. > > On this issue - either use sensible macros that YOU ARE DEFINING, not > assert.h, but YOU, that allow you to evaluate whether a condition is true > or false - or I will not accept your unreadable test code. > > It's that simple and I'm done discussing this. Thanks for your time on discussing this. Please, if I may say, when presenting your argument, keep it technical and avoid personal attack. Using personal attacks rather than using logic to refute your argument is “Ad Hominem Fallacy” [1] and will make it harder to get your point across. [1] https://www.txst.edu/philosophy/resources/fallacy-definitions/ad-hominem.html#:~:text=(Attacking%20the%20person)%3A%20This,who%20is%20making%20the%20argument. Additionally, The mseal_test was reviewed-by Muhammad Usama Anjum during original RFC discussion. IIUC, Muhammad Usama Anjum has domain knowledge for selftest infra, and I have relied on Muhammad’s comments and implemented all those comments. I'm not saying there is no room for improvement, but it should happen in more respectful and constructive ways. In any case, I hope we have common interest and passion to get more test coverage to avoid future regression. Given that we had 2 regressions in the past during code reviews and a pending regression to fix at this moment in memory sealing area, the benefit of additional test coverage is obvious. Specific on FAIL_TEST_IF_FALS macro, during the course of this discussion, your comments have started with, and I quote: “ Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy. Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().” “This is beyond horrible. You really have to add more asserts.” TO my response: “ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks related to self-test infra, such as counting how many tests are failing.” And your question: “why we have to assert negations only, and other self tests do not do this.” And my response: "My most test-infra related comments comes from Muhammad Usama Anjum" (added into this email), e.g. assert is not recommended.[1] , [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/" And my additional try to clarify about your question about negations: “So it is not a problem with the MACRO, but where is it used ? ret = sys_mseal(ptr, size); FAIL_TEST_IF_FALSE(!ret); Take this example, it would be assert(!ret) The syscall return usually returns 0 to indicate success, where a negative comes from, but dev should be so used to (!ret), it is a common pattern to check syscall return code. e.g assert(!ret)" And I offer an alternative approach for macro naming: "ret = sys_mseal(ptr, size); TEST_EXPECT_TRUE(!ret);" Which you didn’t respond to directly. Given the situation, I think it might be best to let domain experts from the testinfra team, such as Muhammad to suggestion a better replacement for this macro. Best regards, -Jeff
Hi Mark and Muhammad On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote: > > > So it is not a problem with the MACRO, but where is it used ? > > > ret = sys_mseal(ptr, size); > > FAIL_TEST_IF_FALSE(!ret); > > > Take this example, it would be > > assert(!ret) > > The problem is that the macro name is confusing and not terribly > consistent with how the rest of the selftests work. The standard > kselftest result reporting is > > ksft_test_result(bool result, char *name_format, ...); > > so the result of the test is a boolean flag which is passed in. This > macro on the other hand sounds like a double negative so you have to > stop and think what the logic is, and it's not seen anywhere else so > nobody is going to be familiar with it. The main thing this is doing is > burying a return statement in there, that's a bit surprising too. > Thanks for explaining the problem, naming is hard. Do you have a suggestion on a better naming? > I'll also note that these macros are resulting in broken kselftest > output, the name for a test has to be stable for automated systems to be > able to associate test results between runs but these print > > ksft_test_result_fail("%s: line:%d\n", \ > __func__, __LINE__); \ > return; \ > > which includes the line number of the test in the name which is an > obvious problem, automated systems won't be able to tell that any two > failures are related to each other never mind the passing test. We > should report why things failed but it's better to do that with a > ksft_print_msg(), ideally one that's directly readable rather than > requiring someone to go into the source code and look it up. > I don't know what the issue you described is ? Are you saying that we are missing line numbers ? it is not. here is the sample of output: Failure in the second test case from last: ok 105 test_munmap_free_multiple_ranges not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 ok 107 test_munmap_free_multiple_ranges_with_split # Planned tests != run tests (106 != 107) > A more standard way to write what you've got here would be to have the > tests return a bool then have a runner loop which iterates over the > tests: > > struct { > char *name; > bool (*func)(void); > } tests[]; > > ... > > for (i = 0; i < ARRAY_SIZE(tests); i++) > ksft_test_result(tests[i].test(), tests[i].name); > > then the tests can just have explicit return statements and don't need > to worry about logging anything other than diagnostics. > > Depending on how much you need to share between tests you might also be > able to use kselftest_harness.h which fork()s each test into a separate > child and allows you to just fault to fail if that's easier. > > > > We are writing unit tests in a test framework, let's use very well > > > established industry practices please. > > Plus also the fact that we have a framework here... > > > > Also note that you don't even need to reinvent the wheel, there is a > > > fully-featured test harness available in > > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and > > > EXPECT_xxx() helpers. > > > The EXPECT_xxx() doesn't take care of reporting though, or maybe it > > I rather think people would've noticed if the test harness was so broken > that it was unable to report failures. If it is that broken we should > fix it rather than open coding something else. In general, I agree with those comments, but I would like to rely on domain experts in test infra to recommend what to use, or is acceptable. In this case, I hope Muhammad, who reviewed this code in the first place, can make recommendations on a replacement of this macro. I would image the needs of something similar to FAIL_TEST_IF_FALSE is common in selftest writing: 1> lightweight enough so dev can pick up quickly and adapt to existing tests, instead of rewriting everything from scratch. 2> assert like syntax 3> fail the current test case, but continue running the next test case 4> take care of reporting test failures. Thanks -Jeff
On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote: > > The problem is that the macro name is confusing and not terribly > > consistent with how the rest of the selftests work. The standard > > kselftest result reporting is > > ksft_test_result(bool result, char *name_format, ...); > > > > so the result of the test is a boolean flag which is passed in. This > > macro on the other hand sounds like a double negative so you have to > > stop and think what the logic is, and it's not seen anywhere else so > > nobody is going to be familiar with it. The main thing this is doing is > > burying a return statement in there, that's a bit surprising too. > Thanks for explaining the problem, naming is hard. Do you have a > suggestion on a better naming? Honestly I'd probably deal with this by refactoring such that the macro isn't needed and the tests follow a pattern more like: if (ret != 0) { ksft_print_msg("$ACTION failed with %d\n", ret); return false; } when they encouter a failure, the pattern I sketched in my earlier message, or switch to kselftest_harness.h (like I say I don't know if the fork()ing is an issue for these tests). If I had to have a macro it'd probably be something like mseal_assert(). > > I'll also note that these macros are resulting in broken kselftest > > output, the name for a test has to be stable for automated systems to be > > able to associate test results between runs but these print .... > > which includes the line number of the test in the name which is an > > obvious problem, automated systems won't be able to tell that any two > > failures are related to each other never mind the passing test. We > > should report why things failed but it's better to do that with a > > ksft_print_msg(), ideally one that's directly readable rather than > > requiring someone to go into the source code and look it up. > I don't know what the issue you described is ? Are you saying that we > are missing line numbers ? it is not. here is the sample of output: No, I'm saying that having the line numbers is a problem. > Failure in the second test case from last: > ok 105 test_munmap_free_multiple_ranges > not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 > ok 107 test_munmap_free_multiple_ranges_with_split Test 106 here is called "test_munmap_free_multiple_ranges_with_split: line:2573" which automated systems aren't going to be able to associate with the passing "test_munmap_free_multiple_ranges_with_split", nor with any failures that occur on any other lines in the function. > I would image the needs of something similar to FAIL_TEST_IF_FALSE is > common in selftest writing: > 1> lightweight enough so dev can pick up quickly and adapt to existing > tests, instead of rewriting everything from scratch. > 2> assert like syntax > 3> fail the current test case, but continue running the next test case > 4> take care of reporting test failures. Honestly this just sounds and looks like kselftest_harness.h, it's ASSERT_ and EXPECT_ macros sound exactly like what you're looking for for asserts. The main gotchas with it are that it's not particularly elegant for test cases which need to enumerate system features (which I don't think is the case for mseal()?) and that it runs each test case in a fork()ed child which can be inconvenient for some tests. The use of fork() is because that make the overall test program much more robust against breakage in individual tests and allows things like per test timeouts.
Hi Mark On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > > On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote: > > > > The problem is that the macro name is confusing and not terribly > > > consistent with how the rest of the selftests work. The standard > > > kselftest result reporting is > > > > ksft_test_result(bool result, char *name_format, ...); > > > > > > so the result of the test is a boolean flag which is passed in. This > > > macro on the other hand sounds like a double negative so you have to > > > stop and think what the logic is, and it's not seen anywhere else so > > > nobody is going to be familiar with it. The main thing this is doing is > > > burying a return statement in there, that's a bit surprising too. > > > Thanks for explaining the problem, naming is hard. Do you have a > > suggestion on a better naming? > > Honestly I'd probably deal with this by refactoring such that the macro > isn't needed and the tests follow a pattern more like: > > if (ret != 0) { > ksft_print_msg("$ACTION failed with %d\n", ret); > return false; > } > So expanding the macro to actually code ? But this makes the meal_test quite large with lots of "if", and I would rather avoid that. > when they encouter a failure, the pattern I sketched in my earlier > message, or switch to kselftest_harness.h (like I say I don't know if > the fork()ing is an issue for these tests). If I had to have a macro > it'd probably be something like mseal_assert(). > I can go with mseal_assert, the original macro is used by mseal_test itself, and only intended as such. If changing name to mseal_assert() is acceptable, this seems to be a minimum change and I'm happy with that. > > > I'll also note that these macros are resulting in broken kselftest > > > output, the name for a test has to be stable for automated systems to be > > > able to associate test results between runs but these print > > .... > > > > which includes the line number of the test in the name which is an > > > obvious problem, automated systems won't be able to tell that any two > > > failures are related to each other never mind the passing test. We > > > should report why things failed but it's better to do that with a > > > ksft_print_msg(), ideally one that's directly readable rather than > > > requiring someone to go into the source code and look it up. > > > I don't know what the issue you described is ? Are you saying that we > > are missing line numbers ? it is not. here is the sample of output: > > No, I'm saying that having the line numbers is a problem. > > > Failure in the second test case from last: > > > ok 105 test_munmap_free_multiple_ranges > > not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 > > ok 107 test_munmap_free_multiple_ranges_with_split > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split: > line:2573" which automated systems aren't going to be able to associate > with the passing "test_munmap_free_multiple_ranges_with_split", nor with > any failures that occur on any other lines in the function. > I see. That will happen when those tests are modified and line number changes. I could see reasoning for this argument, especially when those tests are flaky and get updated often. In practice, I hope any of those kernel self-test failures should get fixed immediately, or even better, run before dev submitting the patch that affects the mm area. Having line number does help dev to go to error directly, and I'm not against filling in the "action" field, but you might also agree with me, finding unique text for each error would require some decent amount of time, especially for large tests such as mseal_test. > > I would image the needs of something similar to FAIL_TEST_IF_FALSE is > > common in selftest writing: > > > 1> lightweight enough so dev can pick up quickly and adapt to existing > > tests, instead of rewriting everything from scratch. > > 2> assert like syntax > > 3> fail the current test case, but continue running the next test case > > 4> take care of reporting test failures. > > Honestly this just sounds and looks like kselftest_harness.h, it's > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for > for asserts. The main gotchas with it are that it's not particularly > elegant for test cases which need to enumerate system features (which I > don't think is the case for mseal()?) and that it runs each test case in > a fork()ed child which can be inconvenient for some tests. The use of > fork() is because that makes the overall test program much more robust > against breakage in individual tests and allows things like per test > timeouts. OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. If I switch to test_fixture, e,g, using TEST(test_name) how do I pass the "seal" flag to it ? e.g. how do I run the same test twice, first seal = true, and second seal=false. test_seal_mmap_shrink(false); test_seal_mmap_shrink(true); The example [1], isn't clear about that. https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example Thanks
On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote: > > when they encouter a failure, the pattern I sketched in my earlier > > message, or switch to kselftest_harness.h (like I say I don't know if > > the fork()ing is an issue for these tests). If I had to have a macro > > it'd probably be something like mseal_assert(). > > > I can go with mseal_assert, the original macro is used by mseal_test > itself, and only intended as such. > > If changing name to mseal_assert() is acceptable, this seems to be a > minimum change and I'm happy with that. No.
On 10/18/24 13:52, Lorenzo Stoakes wrote: > On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote: >>> when they encouter a failure, the pattern I sketched in my earlier >>> message, or switch to kselftest_harness.h (like I say I don't know if >>> the fork()ing is an issue for these tests). If I had to have a macro >>> it'd probably be something like mseal_assert(). >>> >> I can go with mseal_assert, the original macro is used by mseal_test >> itself, and only intended as such. >> >> If changing name to mseal_assert() is acceptable, this seems to be a >> minimum change and I'm happy with that. > > No. > Jeff, Please pay attention to the feedback you have been receiving so far from Mark and others about using the existing kselftest framework for reporting results and don't reinvent wheel. We have two frameworks to choose from - they both have been in use for quiet sometime by tests. If there is a need to add new functions and fix the existing ones that should happen in kselftest_harness.h or kselftest.h. We keep fixing problem and enhancing them as needed. With my kselfest and framework maintainer hat on, I don't want to see yet another framework emerging which is buried in tests. thanks, -- Shuah
On 10/18/24 13:32, Jeff Xu wrote: > Hi Mark > > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote: >> >> On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: >>> On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@kernel.org> wrote: >> >>>> The problem is that the macro name is confusing and not terribly >>>> consistent with how the rest of the selftests work. The standard >>>> kselftest result reporting is >> >>>> ksft_test_result(bool result, char *name_format, ...); >>>> >>>> so the result of the test is a boolean flag which is passed in. This >>>> macro on the other hand sounds like a double negative so you have to >>>> stop and think what the logic is, and it's not seen anywhere else so >>>> nobody is going to be familiar with it. The main thing this is doing is >>>> burying a return statement in there, that's a bit surprising too. >> >>> Thanks for explaining the problem, naming is hard. Do you have a >>> suggestion on a better naming? >> >> Honestly I'd probably deal with this by refactoring such that the macro >> isn't needed and the tests follow a pattern more like: >> >> if (ret != 0) { >> ksft_print_msg("$ACTION failed with %d\n", ret); >> return false; >> } >> > So expanding the macro to actually code ? > But this makes the meal_test quite large with lots of "if", and I > would rather avoid that. > > >> when they encouter a failure, the pattern I sketched in my earlier >> message, or switch to kselftest_harness.h (like I say I don't know if >> the fork()ing is an issue for these tests). If I had to have a macro >> it'd probably be something like mseal_assert(). >> > I can go with mseal_assert, the original macro is used by mseal_test > itself, and only intended as such. > > If changing name to mseal_assert() is acceptable, this seems to be a > minimum change and I'm happy with that. > >>>> I'll also note that these macros are resulting in broken kselftest >>>> output, the name for a test has to be stable for automated systems to be >>>> able to associate test results between runs but these print >> >> .... >> >>>> which includes the line number of the test in the name which is an >>>> obvious problem, automated systems won't be able to tell that any two >>>> failures are related to each other never mind the passing test. We >>>> should report why things failed but it's better to do that with a >>>> ksft_print_msg(), ideally one that's directly readable rather than >>>> requiring someone to go into the source code and look it up. >> >>> I don't know what the issue you described is ? Are you saying that we >>> are missing line numbers ? it is not. here is the sample of output: >> >> No, I'm saying that having the line numbers is a problem. >> >>> Failure in the second test case from last: >> >>> ok 105 test_munmap_free_multiple_ranges >>> not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 >>> ok 107 test_munmap_free_multiple_ranges_with_split >> >> Test 106 here is called "test_munmap_free_multiple_ranges_with_split: >> line:2573" which automated systems aren't going to be able to associate >> with the passing "test_munmap_free_multiple_ranges_with_split", nor with >> any failures that occur on any other lines in the function. >> > I see. That will happen when those tests are modified and line number > changes. I could see reasoning for this argument, especially when > those tests are flaky and get updated often. > > In practice, I hope any of those kernel self-test failures should get > fixed immediately, or even better, run before dev submitting the patch > that affects the mm area. > > Having line number does help dev to go to error directly, and I'm not > against filling in the "action" field, but you might also agree with > me, finding unique text for each error would require some decent > amount of time, especially for large tests such as mseal_test. > >>> I would image the needs of something similar to FAIL_TEST_IF_FALSE is >>> common in selftest writing: >> >>> 1> lightweight enough so dev can pick up quickly and adapt to existing >>> tests, instead of rewriting everything from scratch. >>> 2> assert like syntax >>> 3> fail the current test case, but continue running the next test case >>> 4> take care of reporting test failures. >> >> Honestly this just sounds and looks like kselftest_harness.h, it's >> ASSERT_ and EXPECT_ macros sound exactly like what you're looking for >> for asserts. The main gotchas with it are that it's not particularly >> elegant for test cases which need to enumerate system features (which I >> don't think is the case for mseal()?) and that it runs each test case in >> a fork()ed child which can be inconvenient for some tests. The use of >> fork() is because that makes the overall test program much more robust >> against breakage in individual tests and allows things like per test >> timeouts. > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. > > If I switch to test_fixture, e,g, using TEST(test_name) > > how do I pass the "seal" flag to it ? Doesn't TH_LOG() work for you to pass arguments? > e.g. how do I run the same test twice, first seal = true, and second seal=false. > > test_seal_mmap_shrink(false); > test_seal_mmap_shrink(true); > > The example [1], isn't clear about that. > > https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example > > Thanks > thanks, -- Shuah
On Fri, Oct 18, 2024 at 11:01:40AM -0700, Jeff Xu wrote: > > > > Jeff, you're falling into your usual pattern of being unreasonably > > argumentative for apparently no reason and I really don't have time to > > constantly respond inline when you're just ignoring what I tell you. > > > > You do this on nearly all code review and this just isn't working. If you > > want to effectively be involved in mseal you need to LISTEN. > > > > The more you do this the less patient everybody gets with you and the less > > likely your series will ever get merged. This is not good for mseal or > > anybody involved. > > > > On this issue - either use sensible macros that YOU ARE DEFINING, not > > assert.h, but YOU, that allow you to evaluate whether a condition is true > > or false - or I will not accept your unreadable test code. > > > > It's that simple and I'm done discussing this. > > Thanks for your time on discussing this. > > Please, if I may say, when presenting your argument, keep it technical > and avoid personal attack. Using personal attacks rather than using > logic to refute your argument is “Ad Hominem Fallacy” [1] and will > make it harder to get your point across. This is not how ad hominem works, Jeff, it's a common misunderstanding of what that means. It'd be the case if in replying to a specific technical point I was to respond with something personal. That is not what is happening here. In fact it is not personal at all - code review consists of 1. technical points and 2. how well the conversation goes on review. If 2 is absolutely failing, it is absolutely fair and pertinent to bring that up. What is happening here is that we have spent several months trying to explain to you very very simple points: 1. Have macros that assert both truth and falsity so you don't have some inverted and unreadable assert. 2. Do not use arbitrary 'magic numbers'. Instead of listening, you have been needlessly difficult in a way others are not. I can't spend chunks of my working day going point-by-point knowing you will ultimately say something like 'well I just don't want to' or simply ignore points. It is a waste of both of our time, but it is what you do. > > [1] https://www.txst.edu/philosophy/resources/fallacy-definitions/ad-hominem.html#:~:text=(Attacking%20the%20person)%3A%20This,who%20is%20making%20the%20argument. > > Additionally, The mseal_test was reviewed-by Muhammad Usama Anjum > during original RFC discussion. IIUC, Muhammad Usama Anjum has domain > knowledge for selftest infra, and I have relied on Muhammad’s comments > and implemented all those comments. If I can't read the tests, I'm going to NACK the series, sorry. > > I'm not saying there is no room for improvement, but it should happen > in more respectful and constructive ways. In any case, I hope we have > common interest and passion to get more test coverage to avoid > future regression. Given that we had 2 regressions in the past during > code reviews and a pending regression to fix at this moment in memory > sealing area, the benefit of additional test coverage is obvious. You are repeatedly ignoring what you are being told by me and others. You have done this consistently to the point that you are taking up quite a bit of our time. You do this on pretty much all code reviews. There is not just 'room for improvement', you are bordering on being impossible to deal with. The benefits of additional test coverage is indeed obvious, but not if the tests are largely redundant, misleading or confused, which yours in fact are. This is why I proposed that we sit down and figure out exactly what it is you want to test ahead of time, and NACKed the series (I'm not even quite sure why we are discussing this here still). The fact you're debating about using ASSERT(), EXPECT() on the same series months later is not encouraging. > > Specific on FAIL_TEST_IF_FALS macro, during the course of this > discussion, your comments have started with, and I quote: > > “ Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy. > Would be nice to have something human-readable like ASSERT_EQ() or > ASSERT_TRUE() or ASSERT_FALSE().” > > “This is beyond horrible. You really have to add more asserts.” > > TO my response: > > “ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The > FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks > related to self-test infra, such as counting how many tests are > failing.” > > And your question: > “why we have to assert negations only, and other self tests do not do this.” > > And my response: > "My most test-infra related comments comes from Muhammad Usama Anjum" > (added into this email), e.g. assert is not recommended.[1] , > [1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.com/" This is a PERFECT example of the problem. You are excluding my response to this - that he said NO SUCH THING - but rather he was talking about having an EXPECT()-like pattern! You repeatedly do this - you ignore responses that contradict you. This is not a functional way to do code review. > > And my additional try to clarify about your question about negations: > “So it is not a problem with the MACRO, but where is it used ? > ret = sys_mseal(ptr, size); > FAIL_TEST_IF_FALSE(!ret); > Take this example, it would be > assert(!ret) > The syscall return usually returns 0 to indicate success, where a > negative comes from, but dev should be so used to (!ret), it is a > common pattern to check syscall return code. e.g assert(!ret)" > > And I offer an alternative approach for macro naming: > "ret = sys_mseal(ptr, size); > TEST_EXPECT_TRUE(!ret);" > > Which you didn’t respond to directly. Because we are going round in circles on 2 very very simple points and I don't have infinite time. Simply introduce ASSERT_TRUE(), ASSERT_FALSE(), EXPECT_TRUE(), EXPECT_FALSE() perhaps with different spellings (no idea why you would want to do that) or use the test harness. Instead you try to debate 'oh this is kind of like assert' or implying somebody said something or etc. etc. over dozens of emails. > > Given the situation, I think it might be best to let domain experts > from the testinfra team, such as Muhammad to suggestion a better > replacement for this macro. I'm not sure what kind of 'domain expertise' is required for you to not use magic numbers and to not create unreadable expressions. This is basic stuff. The 'situation' is that I've asked you for 2 extremely simple things that pretty well all other tests do and you are arguing about it 2 months later on a NACKed series. Inviting others into the thread is not going to help that. > > Best regards, > -Jeff As you know I've gone out of my way and dedicated quite a bit of time to trying to find various different solutions to this. It is not far off 10pm here and I am sat here replying to this. I would please ask you to take a step back, stop being so defensive, and just listen. Go read some other code reviews in mm and see how successful code reviews progress. Concede some points. Defer to domain expertise and to maintainers at least on occasion. The reason I keep on bringing up what you characterise as 'ad hominem' (incorrectly, though it's a very common misunderstanding of what that means), is because the problem in these reviews isn't technical - it is your unwillingness to listen and engage with reviewers. If that isn't fixed, I don't see how any of your series can progress. And in general I don't think I can sink more time into giving as detailed feedback on the review process as this. I have tried to do what I can here. Anyway, I hope that we can find a positive resolution to this as I've said before. Thanks, Lorenzo
On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote: > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split: > > line:2573" which automated systems aren't going to be able to associate > > with the passing "test_munmap_free_multiple_ranges_with_split", nor with > > any failures that occur on any other lines in the function. > I see. That will happen when those tests are modified and line number > changes. I could see reasoning for this argument, especially when > those tests are flaky and get updated often. > In practice, I hope any of those kernel self-test failures should get > fixed immediately, or even better, run before dev submitting the patch > that affects the mm area. That's not the entire issue - it is also a problem that the test name is not the same between passes and failures so automated systems can't associate the failures with the passes. When a test starts failing they will see the passing test disappear and a new test appear that has never worked. This will mean that for example if they have bisection support or UI for showing when a test started regressing those won't work. The test name needs to be stable, diagnostics identifying why or where it failed should be separate prints. Actually, prompted by the comments below about test variants I've now run the test and see that what's in -next is also broken in that it's running a lot of the tests twice with sealing enabled or disabled but not including this in the reported test name resulting in most of the tests reporting like this: ok 11 test_seal_mprotect ok 12 test_seal_mprotect which is also going to confuse automated systems, they have a hard time working out which instance is which (generally the test numbers get ignored between runs as they're not at all stable). The test names need to roll in the parameterisation: ok 11 test_seal_mprotect seal=true ok 12 test_seal_mprotect seal=false (or something, the specific format doesn't matter so long as the names are both stable and distinct). > Having line number does help dev to go to error directly, and I'm not > against filling in the "action" field, but you might also agree with > me, finding unique text for each error would require some decent > amount of time, especially for large tests such as mseal_test. In these situations if it's a typical Unix API function setting errno that failed I tend to end up writing diagnostics like: ksft_perror("open()") possibly with some of the arguments included as well, or something equivalently basic for other kinds of error. This is fairly mindless so quick and easy to do and more robust against line number slips if you're not looking at exactly the same version of the code, sometimes it's even enough you don't even need to look at the test to understand why it's upset. > > Honestly this just sounds and looks like kselftest_harness.h, it's > > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for > > for asserts. The main gotchas with it are that it's not particularly > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. > If I switch to test_fixture, e,g, using TEST(test_name) > how do I pass the "seal" flag to it ? > e.g. how do I run the same test twice, first seal = true, and second seal=false. > test_seal_mmap_shrink(false); > test_seal_mmap_shrink(true); That looks like fixture variants to me, using those with kselftest_harness.h will also fix the problem with duplicate test names being used since it generates different names for each instance of the test. Something like: FIXTURE(with_seal) { }; FIXTURE_VARIANT(with_seal) { bool seal; }; FIXTURE_VARIANT_ADD(with_seal, yes) { .seal = true, }; FIXTURE_VARIANT_ADD(with_seal, no) { .seal = false, }; FIXTURE_SETUP(with_seal) { } FIXTURE_TEARDOWN(with_seal) { } then a bunch of tests using that fixture: TEST_F(with_seal, test_seal_mmap_shrink) { if (variant->seal) { /* setup sealing */ } ... } TEST_F(with_seal, test_seal_mprotect) { if (variant->seal) { /* setup sealing */ } ... } You don't need to actually set up anything in your fixture, but you do need to have setup and teardown functions so the framework can emit required boilerplate. The gcs-locking.c test I recently added in -next is an example of a similar thing where we just need the variants, there's no actual fixture.
HI Mark On Fri, Oct 18, 2024 at 2:05 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 18, 2024 at 12:32:37PM -0700, Jeff Xu wrote: > > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown <broonie@kernel.org> wrote: > > > On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: > > > > Test 106 here is called "test_munmap_free_multiple_ranges_with_split: > > > line:2573" which automated systems aren't going to be able to associate > > > with the passing "test_munmap_free_multiple_ranges_with_split", nor with > > > any failures that occur on any other lines in the function. > > > I see. That will happen when those tests are modified and line number > > changes. I could see reasoning for this argument, especially when > > those tests are flaky and get updated often. > > > In practice, I hope any of those kernel self-test failures should get > > fixed immediately, or even better, run before dev submitting the patch > > that affects the mm area. > > That's not the entire issue - it is also a problem that the test name > is not the same between passes and failures so automated systems can't > associate the failures with the passes. I failed to understand this part. Maybe you meant the failing logging is not the same across the multiple versions of test code, by testname you meant "failing logging" >When a test starts failing they > will see the passing test disappear and a new test appears that has never > worked. > This will mean that for example if they have bisection support > or UI for showing when a test started regressing those won't work. The > test name needs to be stable, diagnostics identifying why or where it > failed should be separate prints. > If the test hasn't been changed for a while, and start failing. Then it is quite easy to run the same test on recent code changes. I think you might agree with me on this. The only thing that bisec needs to check is if the entire tests are failing or not. I haven't used the biset functionality, so I'm not sure how it works exactly, e.g. when it runs on the old version of kernel, does it use the test binary from the old kernel ? or the test binary provided by dev ? > Actually, prompted by the comments below about test variants I've now > run the test and see that what's in -next is also broken in that it's > running a lot of the tests twice with sealing enabled or disabled but > not including this in the reported test name resulting in most of the > tests reporting like this: > > ok 11 test_seal_mprotect > ok 12 test_seal_mprotect > > which is also going to confuse automated systems, they have a hard time > working out which instance is which (generally the test numbers get > ignored between runs as they're not at all stable). The test names need > to roll in the parameterisation: > > ok 11 test_seal_mprotect seal=true > ok 12 test_seal_mprotect seal=false > > (or something, the specific format doesn't matter so long as the names > are both stable and distinct). > Yes. Agree that this is a limitation of this macro. > > Having line number does help dev to go to error directly, and I'm not > > against filling in the "action" field, but you might also agree with > > me, finding unique text for each error would require some decent > > amount of time, especially for large tests such as mseal_test. > > In these situations if it's a typical Unix API function setting errno > that failed I tend to end up writing diagnostics like: > > ksft_perror("open()") > > possibly with some of the arguments included as well, or something > equivalently basic for other kinds of error. This is fairly mindless so > quick and easy to do and more robust against line number slips if you're > not looking at exactly the same version of the code, sometimes it's even > enough you don't even need to look at the test to understand why it's > upset. > I understand what you are saying, but personally, I still think line numbers are a faster and more direct way to failure. > > > Honestly this just sounds and looks like kselftest_harness.h, it's > > > ASSERT_ and EXPECT_ macros sound exactly like what you're looking for > > > for asserts. The main gotchas with it are that it's not particularly > > > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. > > > If I switch to test_fixture, e,g, using TEST(test_name) > > > how do I pass the "seal" flag to it ? > > e.g. how do I run the same test twice, first seal = true, and second seal=false. > > > test_seal_mmap_shrink(false); > > test_seal_mmap_shrink(true); > > That looks like fixture variants to me, using those with > kselftest_harness.h will also fix the problem with duplicate test names > being used since it generates different names for each instance of the > test. Something like: > > FIXTURE(with_seal) > { > }; > > FIXTURE_VARIANT(with_seal) > { > bool seal; > }; > > FIXTURE_VARIANT_ADD(with_seal, yes) > { > .seal = true, > }; > > FIXTURE_VARIANT_ADD(with_seal, no) > { > .seal = false, > }; > > FIXTURE_SETUP(with_seal) > { > } > > FIXTURE_TEARDOWN(with_seal) > { > } > > then a bunch of tests using that fixture: > > TEST_F(with_seal, test_seal_mmap_shrink) > { > if (variant->seal) { > /* setup sealing */ > } > > ... > } > > TEST_F(with_seal, test_seal_mprotect) > { > if (variant->seal) { > /* setup sealing */ > } > > ... > } > > You don't need to actually set up anything in your fixture, but you do > need to have setup and teardown functions so the framework can emit > required boilerplate. The gcs-locking.c test I recently added in -next > is an example of a similar thing where we just need the variants, > there's no actual fixture. Thanks! This is really helpful, I think the existing mseal_test can be quickly converted using this example. (A side note: if selftest documentation is updated to include this example, it will be much easier to future dev to follow) Thanks -Jeff
On Fri, Oct 18, 2024 at 05:10:01PM -0700, Jeff Xu wrote: > On Fri, Oct 18, 2024 at 2:05 PM Mark Brown <broonie@kernel.org> wrote: > > That's not the entire issue - it is also a problem that the test name > > is not the same between passes and failures so automated systems can't > > associate the failures with the passes. > I failed to understand this part. > Maybe you meant the failing logging is not the same across the > multiple versions of test code, by testname you meant "failing > logging" Tests are identified by the string given in the line reporting their result, that's not *really* a log message but rather a test name. The strings for a given test need to be the same between different runs of the test program for tooling to be able to see that it's the same test. > >When a test starts failing they > > will see the passing test disappear and a new test appears that has never > > worked. > > This will mean that for example if they have bisection support > > or UI for showing when a test started regressing those won't work. The > > test name needs to be stable, diagnostics identifying why or where it > > failed should be separate prints. > If the test hasn't been changed for a while, and start failing. Then Well, you'd hope that the tests never start failing but yet we still have tests and keep running them. > it is quite easy to run the same test on recent code changes. I think > you might agree with me on this. The only thing that bisec needs to > check is if the entire tests are failing or not. Unfortunately we're not in a position where people can reliably assume that every test program will always work everywhere so people work on individual tests, and it's certainly useful for UIs to be able to give an overview of what specifically failed. A bunch of that is tests that just don't implement feature detection/skipping properly. > I haven't used the biset functionality, so I'm not sure how it works > exactly, e.g. when it runs on the old version of kernel, does it use > the test binary from the old kernel ? or the test binary provided by > dev ? That's up to whoever is doing the testing, but I think most people run the selftests from the version of the code they're testing. Some of the subsystems aren't very enthusiastic about supporting running on older kernels. > > > how do I pass the "seal" flag to it ? > > > e.g. how do I run the same test twice, first seal = true, and second seal=false. > > > > > test_seal_mmap_shrink(false); > > > test_seal_mmap_shrink(true); > > That looks like fixture variants to me, using those with > > kselftest_harness.h will also fix the problem with duplicate test names > > being used since it generates different names for each instance of the > > test. Something like: > Thanks! This is really helpful, I think the existing mseal_test can be > quickly converted using this example. Great! > (A side note: if selftest documentation is updated to include this > example, it will be much easier to future dev to follow) Possibly send a patch adding that wherever you were looking? That was just a quick hack down of the gcs-locking program verifying that it had what I thought you needed.
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); } +static void test_seal_mmap_expand_seal_middle(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + /* ummap last 4 pages. */ + ret = sys_munmap(ptr + 8 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 8 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to expand and overwrite (MAP_FIXED) */ + ret2 = mmap(ptr, 12 * page_size, PROT_READ, + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); + if (seal) { + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(ret2 == ptr); + + REPORT_TEST_PASS(); +} + +static void test_seal_mmap_shrink_seal_middle(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to shrink and overwrite (MAP_FIXED) */ + ret2 = mmap(ptr, 7 * page_size, PROT_READ, + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); + if (seal) { + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(ret2 == ptr); + + REPORT_TEST_PASS(); +} + +static void test_seal_mmap_reuse_addr(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + if (seal) { + ret = sys_mseal(ptr, size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to change protection. */ + ret2 = mmap(ptr, size, PROT_NONE, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + + /* MAP_FIXED is not used, expect new addr */ + FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED)); + FAIL_TEST_IF_FALSE(ret2 != ptr); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + REPORT_TEST_PASS(); +} + int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n"); - ksft_set_plan(91); + ksft_set_plan(97); test_seal_addseal(); test_seal_unmapped_start(); @@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true); + test_seal_mmap_expand_seal_middle(false); + test_seal_mmap_expand_seal_middle(true); + test_seal_mmap_shrink_seal_middle(false); + test_seal_mmap_shrink_seal_middle(true); + test_seal_mmap_reuse_addr(false); + test_seal_mmap_reuse_addr(true); + ksft_finished(); }