Re: [PATCH 3/3] selftests/mseal_system_mappings: Test vdso unmap and remap

[Lorenzo Stoakes (ARM)]

On Wed, Oct 28, 2026 at 12:40:32AM +0800, Chris Down wrote:
> Lorenzo Stoakes (ARM) writes:
> > Sorry, in mm we require commit messages. Even for tests :)
>
> Sure thing, I'll add one.
>
> > On Tue, Oct 27, 2026 at 07:43:25PM +0800, Chris Down wrote:
> > > Signed-off-by: Chris Down <chris@xxxxxxxxxxxxxx>
> > > ---
> > >  .../mseal_system_mappings/sysmap_is_sealed.c  | 92 +++++++++++++++++++
> > >  1 file changed, 92 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c b/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c
> > > index ea75ad537620..83b4fadfbfe7 100644
> > > --- a/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c
> > > +++ b/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c
> >
> > Why on earth does 'mseal_system_mappings' have its own directory...??
> >
> > Ugh I don't know how I let this stuff go on the original series :/
> >
> > Please, for the love of all that's sacred, move these tests over to
> > tools/selftests/mm/. This is an mm feature, for our sins...
> >
> > You can do the move as a prerequisite patch in the series.
>
> Will do, I'll make the move the first patch.
>
> > (And, again, I really do insist on your change doing some cleanup here as
> > well rather than extending this stuff.)
>
> Yep, see my reply to 1/3.
>
> > > @@ -10,6 +10,7 @@
> > >  #include <unistd.h>
> > >  #include <string.h>
> > >  #include <stdbool.h>
> > > +#include <sys/mman.h>
> > >  #include <sys/utsname.h>
> > >  #include <zlib.h>
> > >
> > > @@ -232,4 +233,95 @@ TEST_F(basic, check_sealed)
> > >  	EXPECT_EQ(expected, sealed);
> > >  };
> > >
> > > +static bool find_vdso(unsigned long *start, unsigned long *end)
> > > +{
> > > +	char line[MAX_LINE_LEN];
> > > +	bool found = false;
> > > +	FILE *maps;
> > > +
> > > +	maps = fopen("/proc/self/maps", "r");
> > > +	if (!maps)
> > > +		return false;
> > > +
> > > +	while (fgets(line, sizeof(line), maps)) {
> > > +		if (strstr(line, "[vdso]") &&
> > > +		    sscanf(line, "%lx-%lx", start, end) == 2) {
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	fclose(maps);
> > > +	return found;
> > > +}
> >
> > There's shared code for this kind of thing, please don't duplicate a
> > /proc/self/maps thing if unnecessary. Also there's a nicer ioctl interface for
> > all this.
> >
> > See vm_utils.[ch].
>
> Oh thank you! I will use that.
>
> > I mean arguably this is an mremap test that belongs with mremap? I guess
> > debatable...
>
> Hmm, I'd lean towards keeping it with the sysmap test, since it's really
> testing that the seal is enforced on these mappings rather than anything
> about mremap() itself, but I don't feel strongly if you insist, just let me
> know.

Actually let's keep them together :) It's always a bit blurry...

>
> > This all just seems like work that should have been done in the past when this
> > feature was proposed, and doesn't depend on your changes at all AFAICT? Maybe I
> > missed it though.
> >
> > Valuable in itself, but probably worth sending separately as a result? Otherwise
> > it's a little disconnected from the actual change you're making.
>
> Yeah, it doesn't depend on it. I can send it separately on top of the move
> if you like, I just don't like touching things without tests where possible.

Well that's a good instinct for sure :)

I mean alternatively you could send as patch 1 and explicitly say you're
providing a tested foundation for the other changes.

>
> Thanks for the review!

No worries!

--
Cheers, Lorenzo