[LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API
Ricardo B. Marlière
rbm@suse.com
Thu Apr 3 15:58:33 CEST 2025
On Thu Apr 3, 2025 at 10:22 AM -03, Andrea Cervesato wrote:
> Hi Ricardo,
>
> On 4/3/25 14:36, Ricardo B. Marlière via ltp wrote:
>> From: Ricardo B. Marlière <rbm@suse.com>
>>
>> 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
>> ---
>> .../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(-)
>>
>> diff --git a/testcases/kernel/syscalls/set_thread_area/set_thread_area.h b/testcases/kernel/syscalls/set_thread_area/set_thread_area.h
>> deleted file mode 100644
>> index 2bd2469d549ecf8c5c589a0a9485f886d043f7ed..0000000000000000000000000000000000000000
>> --- a/testcases/kernel/syscalls/set_thread_area/set_thread_area.h
>> +++ /dev/null
>> @@ -1,31 +0,0 @@
>> -#include <stdio.h>
>> -#include <errno.h>
>> -
>> -/* Harness Specific Include Files. */
>> -#include "test.h"
>> -#include "lapi/syscalls.h"
>> -#include "config.h"
>> -
>> -#if defined HAVE_ASM_LDT_H
>> -#include <linux/unistd.h>
>> -#include <asm/ldt.h>
>> -
>> -#if defined HAVE_STRUCT_USER_DESC
>> -typedef struct user_desc thread_area_s;
>> -#elif defined HAVE_STRUCT_MODIFY_LDT_LDT_S
>> -typedef struct modify_ldt_ldt_s thread_area_s;
>> -#else
>> -typedef struct user_desc {
>> - unsigned int entry_number;
>> - unsigned long int base_addr;
>> - unsigned int limit;
>> - unsigned int seg_32bit:1;
>> - unsigned int contents:2;
>> - unsigned int read_exec_only:1;
>> - unsigned int limit_in_pages:1;
>> - unsigned int seg_not_present:1;
>> - unsigned int useable:1;
>> - unsigned int empty:25;
>> -} thread_area_s;
>> -#endif /* HAVE_STRUCT_USER_DESC */
>> -#endif /* HAVE_ASM_LDT_H */
>> diff --git a/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
>> index 30626d5e90ebf8e20624c370cc4474cb51a6b102..ba8d90c826c668fd94edecb95d121ec7c6bbaa06 100644
>> --- a/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
>> +++ b/testcases/kernel/syscalls/set_thread_area/set_thread_area01.c
>> @@ -1,111 +1,64 @@
>> -/*************************************************************************
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> * Copyright (c) Crackerjack Project., 2007
>> * Copyright (c) Manas Kumar Nayak <maknayak@in.ibm.com>
>> * Copyright (c) Cyril Hrubis <chrubis@suse.cz> 2011
>> + * Copyright (c) 2025 SUSE LLC Ricardo B. Marlière <rbm@suse.com>
>> + */
>> +
>> +/*\
>> + * Basic test of i386 thread-local storage for set_thread_area and
>> + * get_thread_area syscalls.
> This is not only a basic test, but also a test that verifies errors for
> set_thread_area and get_thread_area syscalls.
>> *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation; either version 2 of the License, or
>> - * (at your option) any later version.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
>> - * the GNU General Public License for more details.
>> - *
>> - * You should have received a copy of the GNU General Public License
>> - * along with this program; if not, write to the Free Software
>> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + * [Algorithm]
>> *
>> - ************************************************************************/
>> + * The test will first call set_thread_area to a struct user_desc pointer with
>> + * entry_number = -1, which will be set to a free entry_number upon exiting.
>> + * Therefore, a subsequent call to get_thread_area using the same pointer will
>> + * be valid. The same process is done but using -2 instead, which returns EINVAL
>> + * to both calls. Finally, it's done one more time but to an invalid pointer and
>> + * therefore an EFAULT is returned.
> It's usually better to describe the algorithm using dotted list, so it's
> easier to read.
Ack.
>> + */
>>
>> -#include "set_thread_area.h"
>> +#include "tst_test.h"
>>
>> -char *TCID = "set_thread_area_01";
>> -int TST_TOTAL = 6;
>> +#ifdef __i386__
>> +#include "lapi/ldt.h"
>>
>> -#if defined(HAVE_ASM_LDT_H) && defined(HAVE_STRUCT_USER_DESC)
>> +/* 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.
>> + */
>> +static struct user_desc entry = { .entry_number = -1 };
>> +static struct user_desc invalid_entry = { .entry_number = -2 };
>>
>> -static void cleanup(void)
>> +static int set_thread_area(const struct user_desc *entry)
>> {
>> + return tst_syscall(__NR_set_thread_area, entry);
>> }
>>
>> -static void setup(void)
>> +static int get_thread_area(const struct user_desc *entry)
>> {
>> - TEST_PAUSE;
>> + return tst_syscall(__NR_get_thread_area, entry);
>> }
>>
>> -struct test {
>> - int syscall;
>> - const char *const syscall_name;
>> - thread_area_s *u_info;
>> - int exp_ret;
>> - int exp_errno;
>> -};
>> -
>> -/*
>> - * The set_thread_area uses a free entry_number if entry number is set to -1
>> - * and upon the syscall exit the entry number is set to entry which was used.
>> - * So when we call get_thread_area on u_info1, the entry number is initalized
>> - * corectly by the previous set_thread_area.
>> - */
>> -static struct user_desc u_info1 = {.entry_number = -1 };
>> -static struct user_desc u_info2 = {.entry_number = -2 };
>> -
>> -#define VALUE_AND_STRING(val) val, #val
>> -
>> -static struct test tests[] = {
>> - {VALUE_AND_STRING(__NR_set_thread_area), &u_info1, 0, 0},
>> - {VALUE_AND_STRING(__NR_get_thread_area), &u_info1, 0, 0},
>> - {VALUE_AND_STRING(__NR_set_thread_area), &u_info2, -1, EINVAL},
>> - {VALUE_AND_STRING(__NR_get_thread_area), &u_info2, -1, EINVAL},
>> - {VALUE_AND_STRING(__NR_set_thread_area), (void *)-9, -1, EFAULT},
>> - {VALUE_AND_STRING(__NR_get_thread_area), (void *)-9, -1, EFAULT},
>> -};
>
> This was a good idea, but the implementation doesn't fit with the LTP
> API. Test cases in our scenario might improve readability. In
> particular, we can do something like this:
>
> - define 2 variant for the test (check tst_variant). The first variant
> will call set_thread_area and the second will call get_thread_area
> - define 2 test cases. The first will set input value to generate a
> EINVAL and the secon will set input value to generate EFAULT
>
> And this is the first test that will check for errors only: it will run
> both error tests for set_thread_area and get_thread_area. The second
> test is only calling set+get and verify that we have TPASS from syscalls
> and data set is equivalent to data read.
>
> In this way we can organize the testing suite properly and split errors
> from base functionalities as we do in the other testing suites. WDYT?
>
Overall, it makes sense to me. I'm not yet familiar with tst_variant but
from a quick glance it seems to be a good idea in this test. I'll give
it a try.
>> -
>> -int main(int argc, char *argv[])
>> +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);
>> +}
>>
>> - tst_resm(TPASS, "%s returned %li errno %i (%s)",
>> - tests[i].syscall_name, TEST_RETURN,
>> - TEST_ERRNO, strerror(TEST_ERRNO));
>> - }
>> - }
>> +static struct tst_test test = {
>> + .test_all = run,
>> +};
>>
>> - cleanup();
>> - tst_exit();
>> -}
>> #else
>> -int main(void)
>> -{
>> - tst_brkm(TCONF, NULL,
>> - "set_thread_area isn't available for this architecture");
>> -}
>> -#endif
>> +TST_TEST_TCONF("Test supported only on i386");
>> +#endif /* __i386__ */
>>
>> ---
>> base-commit: 898cc14ad412fb521867b43fed5c4e067b76f809
> This is a super cool feature from b4 :-)
Indeed :)
>> 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,
> Kind regards,
> Andrea Cervesato
Thanks for reviewing,
- Ricardo.
More information about the ltp
mailing list