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