[LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Ricardo B. Marlière via ltp]

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 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.
+ */
 
-#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},
-};
-
-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
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,
-- 
Ricardo B. Marlière <rbm@suse.com>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Andrea Cervesato via ltp]

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.
> + */
>   
> -#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?

> -
> -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 :-)
> 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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Ricardo B. Marli��re via ltp]

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.



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Petr Vorel]

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.

> ---
>  .../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.

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},
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

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.

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
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.

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,

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Andrea Cervesato via ltp]

Hi Petr,

On 4/3/25 15:59, 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.
>
>> ---
>>   .../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.
>
> 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},
> 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
>
> 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.
man page also says:

*ENOSYS get_thread_area*() or*set_thread_area*() was invoked as a
               64-bit system call.

>
> [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.
>
> 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},
Right, I would use .supported_archs as well. So we can get rid of the 
#ifndef at the beginning.
>
> Ah, the dependency [1] is at the end in b4 tag:
>> prerequisite-message-id:<20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
b4 shazam command downloads the prerequisites commits inside the current 
branch, before applying any patch, so there's no need to download 
patches manually and to apply them before reviewing.
> 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
> 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.
Good catch.
> 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,
- Andrea

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Ricardo B. Marli��re via ltp]

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,


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Petr Vorel]

Hi Andrea,

> Hi Petr,

...
> > Ah, the dependency [1] is at the end in b4 tag:
> > > prerequisite-message-id:<20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
> b4 shazam command downloads the prerequisites commits inside the current
> branch, before applying any patch, so there's no need to download patches
> manually and to apply them before reviewing.

Unfortunately most of the maintainers and reviewers don't use b4 (yet).
Most still use patches directly from ML or from patchwork/lore. We get by with
this, but it complicates things a bit.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/set_thread_area01: Refactor into new API

[Ricardo B. Marli��re via ltp]

On Thu Apr 3, 2025 at 5:32 PM -03, Petr Vorel wrote:
> Hi Andrea,
>
>> Hi Petr,
>
> ...
>> > Ah, the dependency [1] is at the end in b4 tag:
>> > > prerequisite-message-id:<20250402-conversions-modify_ldt-v6-0-2e4b0e27870e@suse.com>
>> b4 shazam command downloads the prerequisites commits inside the current
>> branch, before applying any patch, so there's no need to download patches
>> manually and to apply them before reviewing.
>
> Unfortunately most of the maintainers and reviewers don't use b4 (yet).
> Most still use patches directly from ML or from patchwork/lore. We get by with
> this, but it complicates things a bit.

No worries, next time I have the need I'll provide a public git ref.

>
> Kind regards,
> Petr


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp