On Thu, Feb 27, 2025 at 04:55:06PM -0800, Jeff Xu wrote: > Hi Lorenzo, > > On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes > > > > > > 2. Tests - could you please add some tests to assert that mremap() fails > > > > for VDSO for instance? You've edited an existing test for VDSO in x86 to > > > > skip the test should this be enabled, but this is not the same as a self > > > > test. And obviously doesn't cover arm64. > > > > > > > > This should be relatively strightforward right? You already have code > > > > for finding out whether VDSO is msealed, so just use that to see if you > > > > skip, then attempt mremap(), mmap() over etc. + assert it fails. > > > > > > > > Ideally these tests would cover all the cases you've changed. > > > > > > > It is not as easy. > > > > > > The config is disabled by default. And I don't know a way to detect > > > KCONFIG from selftest itself. Without this, I can't reasonably > > > determine the test result. > > > > Please in future let's try to get this kind of response at the point of the > > request being made :) makes life much easier. > > > There might be miscommunication ? > This version is the first time you ask about adding a self test. Yeah I thought I'd been clear but this might _very well_ have been me not having been, so apologies if so. I mean 'make sure it's tested' is an overloaded term right? So fact you've tested on android, chromeos, etc. implies 'tested', but also self tests/kunit/whatever. > > IIRC, you had comments about providing the details of what tests I did, in V4. > As a follow-up, I added a test-info section in the cover letter of V5 Thanks. Appreciate that! And this really does point towards a miscommunication on my part, will try to be super explicit in future. > > Though I have thought about adding a selftest since the beginning, > there are two problems: > 1> This config is by default disabled, > 2> No good pattern to check KCONFIG from selftest. Yeah agreed, it's a TOTAL pain. I wish we had a better way of doing this. Maybe a self-volunteering exercise (*goes to write on writeboard :P*) > > > > > So I think it is easy actually. As I say here (perhaps you missed it?) you > > literally already have code you added to the x86 test to prevent test > > failure. > > > > So you essentially look for [vdso] or whatever, then you look up to see if > > it is sealed, ensure an mremap() fails. > > > This suggestion doesn't test the core change of this series, which is > to enable mseal for vdso. Right, and thinking about it, what does this test? Just that mseal works right? It's sort of implicit that, if a VMA is sealed, the seal should work (or rather, tested in mseal tests themselves, rather than mseal system settings). > > When the vdso is marked with "sl", mremap() will fail, that's part of > the mseal() business logic and belongs in mseal_test. The mseal_test > already covers the mseal, and this series doesn't change mseal. > > As for the "sl", as I responded in the "refactor mseal_test" [1] , it > will be part of the verifying step: > > [1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@xxxxxxxxxxxxxx/ OK cool thanks. I will look at this when I can, I'm just snowed under pre-LSF and have been sick so backlog, backlog as discussed off-list. But it's not been forgotten (whiteboard etc. etc.) > > > Of course this doesn't check to see if it _should_ be enabled or not. I'm > > being nice by not _insisting_ we export a way for you to determine whether > > this config option is enabled or not for the sake of a test (since I don't > > want to hold up this series). > > > > But that'd be nice! Maybe in a > > /sys/kernel/security endpoint... > > > > > > ...but I think we can potentially add this later on so we don't hold things > > up here/add yet more to the series. I suspect you and Kees alike would > > prioritise getting _this series_ in at this point :) > > > > You could, if you wanted to, check to see if /proc/config.gz exists and > > zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on > > that, but you know kinda hacky. > > Ya, it is hacky. None of the existing selftest uses this pattern, and > I'm not sure /proc/config.gz is enabled in the default kernel config. Yeah and I'm not sure I even like my hacky suggestion here, I mean let's be honest, it sucks. > > One option is to have ChromeOS or Android to maintain an out-of-tree > test, since the configuration will be enabled there. Nah haha, though of course you can do what you want. Really want something upstream. > > Option two is to create a new path: > tools/testing/selftests/sealsysmap. Then, add > CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to > this path. This seems to be the preferred way by selftest, but we need > a new dir for that. OK I like option 2 let's do this. But let's call it mseal_system_mappings (yes I"m being nitty again :) I really want to try to _explicitly_ say 'mseal' because we have other forms of sealing. Not your fault, but we overload terms like _crazy_ in mm and need to be cautious as not to confuse vs. e.g. memfd seals. > > Option three is to add a self-test when we have a process-level opt-in > solution. This would allow the test to deterministically know whether > the vdso should be sealed or not. Yeah one for future. > > > > > But anyway, FWIW I think it'd be useful to assert that mremap() or munmap() > > fails here for system mappings for the sake of it. I guess this is, in > > effect, only checking mseal-ing works right? But it at least asserts the > > existence of the thing, and that it behaves. > > > > Later on, when you add some way of observing that it's enabled or not, you > > can extend the test to check this. > > I think it is best that we don't add a test that doesn't actually > check the code change. Do you think one of the above three options > works ? maybe the second option (with a new path) ? Yeah I actually agree on reflection. And yes agreed option 2 is great, thanks! > > In any case, I think the risk is low, and the code changes are quite > simple, and fully tested. Yeah indeed, but I'd really like _something_ if possible. If option 2 is relatively quick let's get that sorted out! > > Thanks. > -Jeff. Cheers, Lorenzo
