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 prepareAnd 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 allnoconfigSo, 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 ;-)
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