Re: [PATCH v1 00/17] selftests/nolibc: allow run with minimal kernel config

From: Zhangjin Wu
Date: Sat Jun 24 2023 - 04:40:08 EST

> Hi, Thomas
>
> On 2023-06-23 02:45:59+0800, Zhangjin Wu wrote:
> > > some general comments for the whole series.
> > >
> > > On 2023-06-21 20:52:30+0800, Zhangjin Wu wrote:
> > > > Hi, Willy
> > > >
> > > > This patchset mainly allows speed up the nolibc test with a minimal
> > > > kernel config.
> > > >

(snip)

> > Based on my local powerpc porting, I have prepared some changes like
> > this:
> >
> > # extra kernel configs by architecture
> > EXTCONFIG_powerpc = --enable SERIAL_PMACZILOG --enable CONFIG_SERIAL_PMACZILOG_CONSOLE
> > EXTCONFIG = --set-str CONFIG_INITRAMFS_SOURCE $(CURDIR)/initramfs $(EXTCONFIG_$(ARCH))
> >
> > ...
> >also
> > menuconfig:
> > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) menuconfig
> >
> > extconfig:
> > $(Q)$(srctree)/scripts/config --file $(srctree)/.config $(EXTCONFIG)
> > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) olddefconfig
> >
> > kernel: initramfs extconfig
> > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME)
> >
> >
> > 'menuconfig' is added for development, for example, find why something not work
> > and add the missing options.
> >
> > 'extconfig' is added to enable additional options (before, based on
> > defconfig) to let nolibc-test happy (for powerpc, add missing console
> > options which has been added as modules in default config).
> >
> > Based on your suggestion, this may be a good new target:
> >
> > tinyconfig:
> > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper tinyconfig prepare
> >
> > And this one, use 'allnoconfig' instead of 'olddefconfig':
> >
> > extconfig:
> > $(Q)$(srctree)/scripts/config --file $(srctree)/.config $(EXTCONFIG)
> > $(Q)$(MAKE) -C $(srctree) ARCH=$(KARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG=$(srctree)/.config allnoconfig
> >
> > So, the new 'tinyconfig' may function as the smallest test environment,
> > for faster compile and as a boundary test of nolibc-test itself.
> >
> > But again, still need time to list the minimally required options, if they are
> > few, listing them in the EXTCONFIG_<ARCH> line may be acceptable, but if the
> > options are 'huge', standalone nolibc.config may be required, let's wait for
> > one or two days.
>
> FYI there are many more tests in tools/testing/selftests/ that need
> custom configs to run. Maybe we can reuse some of their configuration
> machinery.
> (And qemu machinery maybe)
>

Yeah, thanks, these files may be very good references:

$ find tools/testing/selftests/ -name "*config" | grep qemu
tools/testing/selftests/wireguard/qemu/arch/aarch64.config
tools/testing/selftests/wireguard/qemu/arch/aarch64_be.config
tools/testing/selftests/wireguard/qemu/arch/arm.config
tools/testing/selftests/wireguard/qemu/arch/armeb.config
tools/testing/selftests/wireguard/qemu/arch/i686.config
tools/testing/selftests/wireguard/qemu/arch/m68k.config
tools/testing/selftests/wireguard/qemu/arch/mips.config
tools/testing/selftests/wireguard/qemu/arch/mips64.config
tools/testing/selftests/wireguard/qemu/arch/mips64el.config
tools/testing/selftests/wireguard/qemu/arch/mipsel.config
tools/testing/selftests/wireguard/qemu/arch/powerpc.config
tools/testing/selftests/wireguard/qemu/arch/powerpc64.config
tools/testing/selftests/wireguard/qemu/arch/powerpc64le.config
tools/testing/selftests/wireguard/qemu/arch/riscv32.config
tools/testing/selftests/wireguard/qemu/arch/riscv64.config
tools/testing/selftests/wireguard/qemu/arch/s390x.config
tools/testing/selftests/wireguard/qemu/arch/um.config
tools/testing/selftests/wireguard/qemu/arch/x86_64.config
tools/testing/selftests/wireguard/qemu/debug.config
tools/testing/selftests/wireguard/qemu/kernel.config

And I have prepared most of them, just left 2-3 architectures.

> > > And it would be interesting how much impact the enablement of procfs,
> > > tmpfs, net and memfd_create has in constrast to the minimal
> > > configuration.
> >
(snip)
> >
> > It did save 20s (~17.1%) for us, not too much, but really faster.
> >
> > > It seems unfortunate to me to complicate the testsuite to handle such
> > > uncommon scenarios.
> >
> > Yeah, such a config is not common, but as explained above, beside the compile
> > speedup improvement, it is really a good boundary test environment for
> > nolibc-test itself to make sure it work (no failure, less skips) at an
> > extremely worst-case scene, although our changes looks many, but every one is
> > as simple as CLOC ;-)
> >
> > And that also means, nolibc is able to run with a very 'tiny' kernel
> > config and users could reuse our config fragments and add their own for
> > their embedded devices.
>
> It would mean that nolibc-test is able to run on *really* trimmed down
> systems, which seems of limited use.
> If the testsuite has more dependencies it would not stop nolibc itself
> to run on them.
>
> As for the CONFIG_NET dependency, which I would guess is one of the more
> expensive configs to enable:
>
> link_cross can be easily adapted to instead use /proc/self.
>

Yes, we can simply use /proc/self or proc/self/cmdline.

> chmod_net relies on /proc/$PID/net accepting chmod().
> It is the only file in /proc/$PID/ that works that way.
>

Something like /tmp/xxx has been used in our patchset, to get a chmodable file,
tmpfs is a good choice, even when the TMPFS (and SHMEM) option is disabled, a
ramfs based tmpfs will be provided by kernel.

> Maybe its a kernel bug anyways and we shouldn't rely on it anyways?
> I'm taking a look.
>

Good catch, I have been also interested in such a curious difference,
because all of the other proc interfaces are not chmodable, not sure if
it is 'intentional', let's discuss in your patch thread, perhaps we
should cc the /proc/self/net maintainers/authors ;-)

> >
> > (snipped)
> >
> > > > It is able to build and run nolibc-test with musl libc now, but there
> > > > are some failures/skips due to the musl its own issues/requirements:
> > > >
> > > > $ sudo ./nolibc-test | grep -E 'FAIL|SKIP'
> > > > 8 sbrk = 1 ENOMEM [FAIL]
> > > > 9 brk = -1 ENOMEM [FAIL]
> > > > 46 limit_int_fast16_min = -2147483648 [FAIL]
> > > > 47 limit_int_fast16_max = 2147483647 [FAIL]
> > > > 49 limit_int_fast32_min = -2147483648 [FAIL]
> > > > 50 limit_int_fast32_max = 2147483647 [FAIL]
> > > > 0 -fstackprotector not supported [SKIPPED]
> > > >
> > > > musl disabled sbrk and brk for some conflicts with its malloc and the
> > > > fast version of int types are defined in 32bit, which differs from nolibc
> > > > and glibc. musl reserved the sbrk(0) to allow get current brk, we
> > > > added a test for this in the v4 __sysret() helper series [2].
> > >
> > > We could add new macros
> > >
> > > #define UINT_MAX(t) (~(t)0)
> > > #define SINT_MAX(t) (((t)1 << (sizeof(t) * 8 - 2)) - (t)1 + ((t)1 << (sizeof(t) * 8 - 2)))
> > >
> > > to get whatever is appropriate for the respective type.
> > >
> >
> > They work perfectly, thanks:
> >
> > /* for fast int test cases in stdlib test, musl use 32bit fast int */
> > #undef UINT_MAX
> > #define UINT_MAX(t) (~(t)0)
> > #undef SINT_MAX
> > #define SINT_MAX(t) (((t)1 << (sizeof(t) * 8 - 2)) - (t)1 + ((t)1 << (sizeof(t) * 8 - 2)))
> > #undef SINT_MIN
> > #define SINT_MIN(t) (-SINT_MAX(t) - 1)
> >
> > ...
> >
> > CASE_TEST(limit_int_fast16_min); EXPECT_EQ(1, INT_FAST16_MIN, (int_fast16_t) SINT_MIN(int_fast16_t)); break;
> > CASE_TEST(limit_int_fast16_max); EXPECT_EQ(1, INT_FAST16_MAX, (int_fast16_t) SINT_MAX(int_fast16_t)); break;
> > CASE_TEST(limit_uint_fast16_max); EXPECT_EQ(1, UINT_FAST16_MAX, (uint_fast16_t) UINT_MAX(uint_fast16_t)); break;
> > CASE_TEST(limit_int_fast32_min); EXPECT_EQ(1, INT_FAST32_MIN, (int_fast32_t) SINT_MIN(int_fast32_t)); break;
> > CASE_TEST(limit_int_fast32_max); EXPECT_EQ(1, INT_FAST32_MAX, (int_fast32_t) SINT_MAX(int_fast32_t)); break;
> > CASE_TEST(limit_uint_fast32_max); EXPECT_EQ(1, UINT_FAST32_MAX, (uint_fast32_t) UINT_MAX(uint_fast32_t)); break;
> >
> > To avoid overriding the existing macros, perhaps we should add something
> > like UINT_TYPE_MAX(t), SINT_TYPE_MAX(t) and SINT_TYPE_MIN(t) ?
>
> They should only be visible inside nolibc-test.c I think.

Yeah, I did so.

> But yes the UINT_MAX naming is bad.
>
> Also when going away from testing constants maybe we can get back some
> test strength by validating the sizeof() of the datatypes.
>

It seems easier.

> <snip>
>
> > > >
> > > > * selftests/nolibc: vfprintf: silence memfd_create() warning
> > > > selftests/nolibc: vfprintf: skip if neither tmpfs nor hugetlbfs
> > > > selftests/nolibc: vfprintf: support tmpfs and hugetlbfs
> > > >
> > > > memfd_create from kernel >= v6.2 forcely warn on missing
> > > > MFD_NOEXEC_SEAL flag, the first one silence it with such flag, for
> > > > older kernels, use 0 flag as before.
> > >
> > > Given this is only a problem when nolibc-test is PID1 and printing to
> > > the system console, we could also just disable warnings on the system
> > > console through syscall() or /proc/sys/kernel/printk.
> >
> > Ok, I did think about disabling console for this call, but I was worried about
> > the requirement of root (euid0) to do so, limiting it under PID1 may solve the
> > root permission issue, but still need to find the right syscall to avoid the
> > dependency of /proc/sys/kernel/printk, otherwise, to avoid failure for !procfs,
> > the whole vfprintf will be skipped for such a warning, to be honest, it looks
> > not a good direction.
>
> This should work:
>
> syslog(__NR_syslog, 6 /* SYSLOG_ACTION_CONSOLE_OFF */);
>

Thanks, will try it like this:

syslog(__NR_syslog, 6 /* SYSLOG_ACTION_CONSOLE_OFF */);
memfd_create()
syslog(__NR_syslog, 7 /* SYSLOG_ACTION_CONSOLE_ON */);

Putting it in prepare() may hide potential issues reported by kernel.

> > >
> > > It would also avoid cluttering the tests for limited gain.
> > >
> >
> > Hmm, if consider the more code lines about disabling/enabling console and the
> > dependency of /proc/sys/kernel/printk, I do prefer current change.
>
> It should really only be the single line above.
>
> > But I'm also interested in how the other applications developers to treat this
> > warning, from the new kernel version side, we should use the latest non
> > executable flags for security, but to let applications work with old kernels,
> > we must support old flags, checking the kernel versions may be another choice.
>
> I know that systemd does it the same way as you proposed it, with
> non-negligible code overhead.
>
> But for nolibc-test I really don't see any security issue.
>

Yes, but at least as a good reference when people want to reuse some
codes from nolibc-test.c ;-)

> > Perhaps it's time for us to add the 'uname()' for nolibc, but the
> > version comparing may be not that easy when we are in c context ;-)
> >
> > https://www.man7.org/linux/man-pages/man2/uname.2.html
>
> Please no :-)
>
> > So, the current method may be still a 'balanced' solution, it tries supported
> > flags from new kernel to old kernel to get a better and working memfd_create()
> > without the version checking, is this cleaner?
> >
> > int i;
> > /* kernel >= v6.2 require MFD_NOEXEC_SEAL (0x0008U), but older ones not support this flag */
>
> It is not required, only desired. The functionality still works as
> expected. I don't think the "old" way can ever stop working as it would
> break userspace ABI.
>
> > unsigned int flags[2] = {0x0008U, 0};
> >
> > for (i = 0; i < 2; i ++) {
>
> Loops like this should use ARRAY_SIZE() to calculate the termination
> condition.

Yeah, it should be:

unsigned int flags[] = {0x0008U, 0};

for (i = 0; i < sizeof(flags)/sizeof(flags[0]); i ++)
...

Best regards,
Zhangjin

>
> > /* try supported flags from new kernels to old kernels */
> > fd = memfd_create("vfprintf", flags[i]);
> > if (fd != -1)
> > break;
> > }
> >
> > if (fd == -1) {
> > ...
> > }
>
> <snip>
>
>
> Thomas