[LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API
Ricardo B. Marlière
rbm@suse.com
Thu Apr 3 18:43:16 CEST 2025
Hello Petr!
On Thu Apr 3, 2025 at 10:59 AM -03, Petr Vorel wrote:
> Hi Ricardo,
>
>> Signed-off-by: Ricardo B. Marlière <rbm@suse.com>
>> ---
>> Changes in v2:
>> - Used tst_test.test_all instead of .test
>> - Removed VALUE_AND_STRING macro
>> - Added local wrappers to the syscalls
>> - Fixed the test description
>> - Added a comment quote from the set_thread_area manual to add context
>> - Link to v1: https://lore.kernel.org/all/20250327-conversions-modify_ldt-v2-6-2907d4d3f6c0@suse.com
> From Message-Id: generated by b4 it's obvious it is v2, not v1. I also tried to
> find v1, but IMHO you haven't sent it.
>
Yes I did a `b4 prep --force-revision 2` and manually added that link
because I considered that patch to be the first try on achieving the
same goal which is that of refactoring set_thread_area test :)
>> ---
>> .../syscalls/set_thread_area/set_thread_area.h | 31 -----
>> .../syscalls/set_thread_area/set_thread_area01.c | 133 +++++++--------------
>> 2 files changed, 43 insertions(+), 121 deletions(-)
>
> ...
>> +++ b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
> ...
>> +#ifdef __i386__
>
>> -#if defined(HAVE_ASM_LDT_H) && defined(HAVE_STRUCT_USER_DESC)
> BTW original test had #if defined(HAVE_ASM_LDT_H) && defined(HAVE_STRUCT_USER_DESC),
> suggesting it was not i386 specific.
>
Indeed, this made me a bit confused at first.
> AC_CHECK_TYPES([struct user_desc, struct modify_ldt_ldt_s],[],[],[#include <asm/ldt.h>])
>
> And there is fallback for struct modify_ldt_ldt_t if neither struct user_desc
> nor struct modify_ldt_ldt_s is available. That's also suggest other archs.
>
> I was able to test it only on x86_64, where it works only when 32 bit
> compatibility is set. That indeed works for #ifdef __i386__ or it could be guarded by:
>
> .supported_archs = (const char *const []){"x86", NULL},
I was not aware of this, thank you.
> This way it would appear in the test catalog doc.
>
> Trying to run it on regular x86_64, it TCONF due tst_syscalls() guard:
>
> set_thread_area01.c:39: TCONF: syscall(205) __NR_set_thread_area not supported on your arch
As Andrea mentioned, the manual page says it returns ENOSYS for 64-bit
variant. And since asm/ldt.h is only present in x86, it's safe to assume
the test is limited to i386. But using .supported_archs is a better way,
indeed.
>
> Which is strange, because I see in arch/x86/kernel/tls.c in do_set_thread_area()
> (the implementation) part of the code with #ifdef CONFIG_X86_64.
>
> If it's really for 32 bit (I don't think so) but supported on more archs, it
> could be guarded by .needs_abi_bits = 32 (again metadata doc).
>
> Just investigating, looking at man set_thread_area(2) it mentions:
>
> At the moment, set_thread_area() is
> available on m68k, MIPS, C-SKY, and x86 (both 32-bit and 64-bit
> variants); get_thread_area() is available on m68k and x86.
>
> [2] https://man7.org/linux/man-pages/man2/set_thread_area.2.html
>
> Looking at kernel code it also shows that other archs are included,
> IMHO the relevant nowadays (besides x86*) are only e.g. arm, arm64/aarch64.
>
> $ git grep -l set_thread_area arch/
> arch/arm/tools/syscall.tbl
> arch/arm64/tools/syscall_32.tbl
> arch/csky/include/asm/syscalls.h
> arch/csky/kernel/syscall.c
> arch/m68k/include/asm/syscalls.h
> arch/m68k/kernel/sys_m68k.c
> arch/m68k/kernel/syscalls/syscall.tbl
> arch/microblaze/kernel/syscalls/syscall.tbl
> arch/mips/kernel/syscall.c
> arch/mips/kernel/syscalls/syscall_n32.tbl
> arch/mips/kernel/syscalls/syscall_n64.tbl
> arch/mips/kernel/syscalls/syscall_o32.tbl
> arch/parisc/kernel/syscalls/syscall.tbl
> arch/sh/kernel/syscalls/syscall.tbl
> arch/um/kernel/ptrace.c
> arch/x86/entry/syscalls/syscall_32.tbl
> arch/x86/entry/syscalls/syscall_64.tbl
> arch/x86/include/asm/ptrace.h
> arch/x86/include/uapi/asm/sigcontext.h
> arch/x86/kernel/process.c
> arch/x86/kernel/ptrace.c
> arch/x86/kernel/tls.c
> arch/x86/um/asm/ptrace.h
> arch/x86/um/os-Linux/tls.c
> arch/x86/um/shared/sysdep/tls.h
> arch/x86/um/tls_32.c
> arch/xtensa/kernel/syscalls/syscall.tbl
>
> But that means nothing. Checking at a real set_thread_area() definition I see:
>
> $ git grep -l 'SYSCALL_DEFINE.*(set_thread_area' arch/
> arch/csky/kernel/syscall.c
> arch/mips/kernel/syscall.c
> arch/x86/kernel/tls.c
> arch/x86/um/tls_32.c
>
> => it probably does not work on arm and arm64/aarch64.
I haven't tested other architectures.
>
> But maybe just not specify anything? Because tst_syscall() will quit with TCONF
> in case kernel does not support it? That case we don't block anybody from mips,
> csky folks to do testing if they want. But...
>
>> +#include "lapi/ldt.h"
>
> ... given it requires 'struct user_desc' from <asm/ldt.h>, which is only for
> x86/i386 and x86_64, I would chose:
>
> .supported_archs = (const char *const []){"x86", NULL},
>
> Ah, the dependency [1] is at the end in b4 tag:
>> prerequisite-message-id: <20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
>
> Later I'll check, because it's obvious that it needs include/lapi/ldt.h from
> another patchset, but I was not sure whether v6 should be used (anyway, it's the
Yes, that's the message-id that should be used :)
> newest). For me it'd be quickest if I could just see working branch on the
> remote fork (+ have CI run for free). BTW if we one day implement automatic CI
> for sending patches, it will not work if some of depending commits is separated.
>
> [1] https://patchwork.ozlabs.org/project/ltp/patch/20250402-conversions-modify_ldt-v6-1-2e4b0e27870e@suse.com/
>
>> +/* When set_thread_area() is passed an entry_number of -1, it searches
>> + * for a free TLS entry. If set_thread_area() finds a free TLS entry,
>> + * the value of u_info->entry_number is set upon return to show which
>> + * entry was changed.
>> + */
>
> ...
>> +void run(void)
>> {
>> - int lc;
>> - unsigned i;
>> -
>> - tst_parse_opts(argc, argv, NULL, NULL);
>> -
>> - setup();
>> -
>> - for (lc = 0; TEST_LOOPING(lc); lc++) {
>> - for (i = 0; i < sizeof(tests) / sizeof(struct test); i++) {
>> - TEST(tst_syscall(tests[i].syscall, tests[i].u_info));
>> + TST_EXP_PASS(set_thread_area(&entry));
>> + TST_EXP_PASS(get_thread_area(&entry));
>
>> - if (TEST_RETURN != tests[i].exp_ret) {
>> - tst_resm(TFAIL, "%s returned %li expected %i",
>> - tests[i].syscall_name,
>> - TEST_RETURN, tests[i].exp_ret);
>> - continue;
>> - }
>> + TST_EXP_FAIL(set_thread_area(&invalid_entry), EINVAL);
>> + TST_EXP_FAIL(get_thread_area(&invalid_entry), EINVAL);
>
>> - if (TEST_ERRNO != tests[i].exp_errno) {
>> - tst_resm(TFAIL,
>> - "%s failed with %i (%s) expected %i (%s)",
>> - tests[i].syscall_name, TEST_ERRNO,
>> - strerror(TEST_ERRNO),
>> - tests[i].exp_errno,
>> - strerror(tests[i].exp_errno));
>> - continue;
>> - }
>> + TST_EXP_FAIL(set_thread_area((void *)-9), EFAULT);
>> + TST_EXP_FAIL(get_thread_area((void *)-9), EFAULT);
>
> This will be a problem for portablity outside of i386 (x86).
> We have tst_get_bad_addr(NULL), and it works for me on x86_64 with 32 bit
> compatibility.
I sent a v3, can you elaborate if that's still a problem in it?
Thanks for reviewing,
- Ricardo.
>
> Kind regards,
> Petr
>
>> ---
>> base-commit: 898cc14ad412fb521867b43fed5c4e067b76f809
>> change-id: 20250403-conversions-set_thread_area-07a90e0cd449
>> prerequisite-message-id: <20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
>> prerequisite-patch-id: 490a3e6bc4004db5234224b6fd6d4bf5030b219d
>> prerequisite-patch-id: 962bb815444eb2de9756dd2659e097567b6d6fe8
>> prerequisite-patch-id: a8357fb870a8d7278e206b4fc65f1d9450fee802
>
>> Best regards,
More information about the ltp
mailing list