| Message ID | 20240920083036.87291-1-haifeng.xu@shopee.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ht_affinity.c: fix ht_affinity test failure | expand |
Hi! > The type of cpumask pointer used in set_affinity() is unsigned long, but > ht_affinity used a unsigned int pointer. When kernel copy cpumask from > user-space pointer, the high 32bit of cpumask is a random value. So the > process can't be bind to the cpu specified by users. Good catch, however it would be better if we used sizeof on the mask instead of sizeof(unsigned long) in the sched_setaffinity() as well: diff --git a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c index f6e9f2745..3c2fe1bf1 100644 --- a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c +++ b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c @@ -67,7 +67,7 @@ int HT_SetAffinity(void) tst_resm(TINFO, "Set test process affinity."); printf("mask: %x\n", mask); - sched_setaffinity(pid, sizeof(unsigned long), &mask); + sched_setaffinity(pid, sizeof(mask), &mask); for (j = 0; j < 10; j++) { for (k = 0; k < 10; k++) { @@ -95,7 +95,7 @@ int HT_SetAffinity(void) tst_resm(TINFO, "Set test process affinity."); printf("mask: %x\n", mask); - sched_setaffinity(pid, sizeof(unsigned long), &mask); + sched_setaffinity(pid, sizeof(mask), &mask); for (j = 0; j < 10; j++) { for (k = 0; k < 10; k++) { Most of the code does that already which makes it impossible to pass different size than the actual size.
On 2024/9/20 16:58, Cyril Hrubis wrote: > Hi! >> The type of cpumask pointer used in set_affinity() is unsigned long, but >> ht_affinity used a unsigned int pointer. When kernel copy cpumask from >> user-space pointer, the high 32bit of cpumask is a random value. So the >> process can't be bind to the cpu specified by users. > > Good catch, however it would be better if we used sizeof on the mask > instead of sizeof(unsigned long) in the sched_setaffinity() as well: > > diff --git a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c > index f6e9f2745..3c2fe1bf1 100644 > --- a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c > +++ b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c > @@ -67,7 +67,7 @@ int HT_SetAffinity(void) > tst_resm(TINFO, "Set test process affinity."); > printf("mask: %x\n", mask); > > - sched_setaffinity(pid, sizeof(unsigned long), &mask); > + sched_setaffinity(pid, sizeof(mask), &mask); > > for (j = 0; j < 10; j++) { > for (k = 0; k < 10; k++) { > @@ -95,7 +95,7 @@ int HT_SetAffinity(void) > tst_resm(TINFO, "Set test process affinity."); > printf("mask: %x\n", mask); > > - sched_setaffinity(pid, sizeof(unsigned long), &mask); > + sched_setaffinity(pid, sizeof(mask), &mask); > > for (j = 0; j < 10; j++) { > for (k = 0; k < 10; k++) { > > > > Most of the code does that already which makes it impossible to pass > different size than the actual size. > From the kernel source code, the user_mask_ptr is unsigned long. SYSCALL_DEFINE3(sched_setaffinity, pid_t, pid, unsigned int, len, unsigned long __user *, user_mask_ptr) so maybe we can keep the type of user_mask_ptr consistent with it.
Hi! > From the kernel source code, the user_mask_ptr is unsigned long. > > SYSCALL_DEFINE3(sched_setaffinity, pid_t, pid, unsigned int, len, > unsigned long __user *, user_mask_ptr) > > so maybe we can keep the type of user_mask_ptr consistent with it. It's actually accessed as an array. See get_user_cpu_mask() function just above this syscall definition and userspace is suppose to pass an array big enough for all the flags of CPUs it cares about. There are also macros in the libc sched.h header that allows you to allocate an array big enough for the current machine and manipulate the bits, see man CPU_ALLOC.
On 2024/9/20 20:22, Cyril Hrubis wrote: > Hi! >> From the kernel source code, the user_mask_ptr is unsigned long. >> >> SYSCALL_DEFINE3(sched_setaffinity, pid_t, pid, unsigned int, len, >> unsigned long __user *, user_mask_ptr) >> >> so maybe we can keep the type of user_mask_ptr consistent with it. > > It's actually accessed as an array. See get_user_cpu_mask() function just > above this syscall definition and userspace is suppose to pass an array > big enough for all the flags of CPUs it cares about. > > There are also macros in the libc sched.h header that allows you to > allocate an array big enough for the current machine and manipulate the > bits, see man CPU_ALLOC. > OK. I'll post next version with your suggestions. Thanks!
diff --git a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c index f6e9f2745..e6829c2d2 100644 --- a/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c +++ b/testcases/kernel/sched/hyperthreading/ht_affinity/ht_affinity.c @@ -48,7 +48,7 @@ len - length in bytes of the bitmask pointed to by user_mask_ptr. int HT_SetAffinity(void) { - unsigned int mask; + unsigned long mask; pid_t pid; int result = 1; int cpu_count, i, j, k, cpuid; @@ -65,7 +65,7 @@ int HT_SetAffinity(void) for (i = 0, mask = 0x1; i < cpu_count; i++, mask = mask << 1) { tst_resm(TINFO, "Set test process affinity."); - printf("mask: %x\n", mask); + printf("mask: %lx\n", mask); sched_setaffinity(pid, sizeof(unsigned long), &mask); @@ -93,7 +93,7 @@ int HT_SetAffinity(void) for (i = 0, mask = 0x3; i < cpu_count - 1; i++, mask = mask << 1) { tst_resm(TINFO, "Set test process affinity."); - printf("mask: %x\n", mask); + printf("mask: %lx\n", mask); sched_setaffinity(pid, sizeof(unsigned long), &mask);
Running ht_affinity test fails: smt_smp_affinity 0 TINFO : Set affinity through system call smt_smp_affinity 0 TINFO : Set test process affinity. mask: 1 smt_smp_affinity 0 TINFO : ...Error ... smt_smp_affinity 0 TINFO : Set test process affinity. mask: c0000000 smt_smp_affinity 0 TINFO : ...Error smt_smp_affinity 3 TFAIL : ht_affinity.c:240: System call setaffinity() is error. The type of cpumask pointer used in set_affinity() is unsigned long, but ht_affinity used a unsigned int pointer. When kernel copy cpumask from user-space pointer, the high 32bit of cpumask is a random value. So the process can't be bind to the cpu specified by users. After converting the cpumask from unsigned int to unsigned long, ht_affinity test succeeds: smt_smp_affinity 0 TINFO : Set affinity through system call smt_smp_affinity 0 TINFO : Set test process affinity. mask: 1 smt_smp_affinity 0 TINFO : ...OK ... smt_smp_affinity 0 TINFO : Set test process affinity. mask: c0000000 smt_smp_affinity 0 TINFO : ...OK smt_smp_affinity 3 TPASS : System call setaffinity() is OK. Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> --- .../kernel/sched/hyperthreading/ht_affinity/ht_affinity.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)