[PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[Wei Gao]

From: wei gao <wegao@suse.com>

Current implementation lead LTP test case futex_waitv failed when compiled with
-m32. This patch add new compat_sys_futex_waitv to handle m32 mode syscall.

The failure reason is futex_waitv in m32 mode will deliver kernel with struct
old_timespec32 timeout, but this struct type can not directly used by current
sys_futex_waitv implementation.

The new function copy main logic of current sys_futex_waitv, just update parameter
type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
and use get_old_timespec32 within the new function to get timeout value.

Signed-off-by: wei gao <wegao@suse.com>
---
 arch/x86/entry/syscalls/syscall_32.tbl |  2 +-
 kernel/futex/syscalls.c                | 61 ++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index c8fac5205803..11bd927dd417 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -453,7 +453,7 @@
 446	i386	landlock_restrict_self	sys_landlock_restrict_self
 447	i386	memfd_secret		sys_memfd_secret
 448	i386	process_mrelease	sys_process_mrelease
-449	i386	futex_waitv		sys_futex_waitv
+449	i386	futex_waitv		sys_futex_waitv  compat_sys_futex_waitv
 450	i386	set_mempolicy_home_node		sys_set_mempolicy_home_node
 451	i386	cachestat		sys_cachestat
 452	i386	fchmodat2		sys_fchmodat2
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 4b6da9116aa6..62d69f8ec34c 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -486,6 +486,67 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
 
 	return ret;
 }
+
+COMPAT_SYSCALL_DEFINE5(futex_waitv, struct futex_waitv __user *, waiters,
+		unsigned int, nr_futexes, unsigned int, flags,
+		struct old_timespec32 __user *, timeout, clockid_t, clockid)
+{
+	struct hrtimer_sleeper to;
+	struct futex_vector *futexv;
+	struct timespec64 ts;
+	ktime_t time;
+	int ret;
+
+	/* This syscall supports no flags for now */
+	if (flags)
+		return -EINVAL;
+
+	if (!nr_futexes || nr_futexes > FUTEX_WAITV_MAX || !waiters)
+		return -EINVAL;
+
+	if (timeout) {
+		int flag_clkid = 0, flag_init = 0;
+
+		if (clockid == CLOCK_REALTIME) {
+			flag_clkid = FLAGS_CLOCKRT;
+			flag_init = FUTEX_CLOCK_REALTIME;
+		}
+
+		if (clockid != CLOCK_REALTIME && clockid != CLOCK_MONOTONIC)
+			return -EINVAL;
+
+		if (get_old_timespec32(&ts, timeout))
+			return -EFAULT;
+
+		/*
+		 * Since there's no opcode for futex_waitv, use
+		 * FUTEX_WAIT_BITSET that uses absolute timeout as well
+		 */
+		ret = futex_init_timeout(FUTEX_WAIT_BITSET, flag_init, &ts, &time);
+		if (ret)
+			return ret;
+
+		futex_setup_timer(&time, &to, flag_clkid, 0);
+	}
+
+	futexv = kcalloc(nr_futexes, sizeof(*futexv), GFP_KERNEL);
+	if (!futexv) {
+		ret = -ENOMEM;
+		goto destroy_timer;
+	}
+
+	ret = futex_parse_waitv(futexv, waiters, nr_futexes, futex_wake_mark,
+				NULL);
+	if (!ret)
+		ret = futex_wait_multiple(futexv, nr_futexes, timeout ? &to : NULL);
+
+	kfree(futexv);
+
+destroy_timer:
+	if (timeout)
+		futex2_destroy_timeout(&to);
+	return ret;
+}
 #endif /* CONFIG_COMPAT */
 
 #ifdef CONFIG_COMPAT_32BIT_TIME
-- 
2.34.1

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[André Almeida]

[+CC Arnd]

Hi Wei,

Em 23/11/2023 02:31, Wei Gao escreveu:
> From: wei gao <wegao@suse.com>
> 
> Current implementation lead LTP test case futex_waitv failed when compiled with
> -m32. This patch add new compat_sys_futex_waitv to handle m32 mode syscall.
> 
> The failure reason is futex_waitv in m32 mode will deliver kernel with struct
> old_timespec32 timeout, but this struct type can not directly used by current
> sys_futex_waitv implementation.
> 
> The new function copy main logic of current sys_futex_waitv, just update parameter
> type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
> and use get_old_timespec32 within the new function to get timeout value.
> 

From, what I recall, we don't want to add new syscalls with 
old_timespec32, giving that they will have a limited lifetime. Instead, 
userspace should be able to come up with a 64-bit timespec 
implementation for -m32.

Thanks,
	André

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[kernel test robot]

Hi Wei,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/x86/asm]
[also build test ERROR on tip/locking/core linus/master v6.7-rc2 next-20231124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wei-Gao/futex-Add-compat_sys_futex_waitv-for-32bit-compatibility/20231123-133427
base:   tip/x86/asm
patch link:    https://lore.kernel.org/r/20231123053140.16062-1-wegao%40suse.com
patch subject: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility
config: x86_64-buildonly-randconfig-005-20231123 (https://download.01.org/0day-ci/archive/20231124/202311241440.BxBAsQ2i-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241440.BxBAsQ2i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311241440.BxBAsQ2i-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld: arch/x86/entry/syscall_32.o:(.rodata+0xe08): undefined reference to `__ia32_compat_sys_futex_waitv'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[Wei Gao]

On Thu, Nov 23, 2023 at 01:09:55PM -0300, André Almeida wrote:
> [+CC Arnd]
> 
> Hi Wei,
> 
> Em 23/11/2023 02:31, Wei Gao escreveu:
> > From: wei gao <wegao@suse.com>
> > 
> > Current implementation lead LTP test case futex_waitv failed when compiled with
> > -m32. This patch add new compat_sys_futex_waitv to handle m32 mode syscall.
> > 
> > The failure reason is futex_waitv in m32 mode will deliver kernel with struct
> > old_timespec32 timeout, but this struct type can not directly used by current
> > sys_futex_waitv implementation.
> > 
> > The new function copy main logic of current sys_futex_waitv, just update parameter
> > type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
> > and use get_old_timespec32 within the new function to get timeout value.
> > 
> 
> From, what I recall, we don't want to add new syscalls with old_timespec32,
> giving that they will have a limited lifetime. Instead, userspace should be
> able to come up with a 64-bit timespec implementation for -m32.
> 
> Thanks,
> 	André

Just a comment, I have checked the glibc latest code but do not see any implemention(*.c) on
futex_waitv syscall. So normally you have to do syscall directly with __NR_futex_waitv from 
userspace. So i guess glibc-side can not covert this struct correctly currently. Correct me if
any misunderstanding.

Thanks
Wei Gao

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[André Almeida]

Hi Wei,

Em 27/11/2023 09:15, Wei Gao escreveu:
> On Thu, Nov 23, 2023 at 01:09:55PM -0300, André Almeida wrote:
>> [+CC Arnd]
>>
>> Hi Wei,
>>
>> Em 23/11/2023 02:31, Wei Gao escreveu:
>>> From: wei gao <wegao@suse.com>
>>>
>>> Current implementation lead LTP test case futex_waitv failed when compiled with
>>> -m32. This patch add new compat_sys_futex_waitv to handle m32 mode syscall.
>>>
>>> The failure reason is futex_waitv in m32 mode will deliver kernel with struct
>>> old_timespec32 timeout, but this struct type can not directly used by current
>>> sys_futex_waitv implementation.
>>>
>>> The new function copy main logic of current sys_futex_waitv, just update parameter
>>> type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
>>> and use get_old_timespec32 within the new function to get timeout value.
>>>
>>
>> From, what I recall, we don't want to add new syscalls with old_timespec32,
>> giving that they will have a limited lifetime. Instead, userspace should be
>> able to come up with a 64-bit timespec implementation for -m32.
>>
>> Thanks,
>> 	André
> 
> Just a comment, I have checked the glibc latest code but do not see any implemention(*.c) on
> futex_waitv syscall. So normally you have to do syscall directly with __NR_futex_waitv from
> userspace. So i guess glibc-side can not covert this struct correctly currently. Correct me if
> any misunderstanding.
> 

futex() has no syscall wrappers in glibc. Userspace needs to figure out 
everything by themselves, including which struct they should use, and I 
don't think that glibc does any conversion. If you create manually a 
timespec64 that works in -m32, and pass this to sycall(__NR_futex_waitv, 
..., &timeout, ...), it should work correctly. You can read more about 
how glibc is planning to deal with this at [1]. Please let me know if 
now it's more clear :)

[1] https://sourceware.org/glibc/wiki/Y2038ProofnessDesign

> Thanks
> Wei Gao

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[Thomas Gleixner]

On Thu, Nov 23 2023 at 00:31, Wei Gao wrote:
> The new function copy main logic of current sys_futex_waitv, just update parameter
> type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
> and use get_old_timespec32 within the new function to get timeout
> value.

That's not how it works.

struct __kernel_timespec is the same on 64bit and 32bit syscalls.

User space has to use the proper type when invoking a syscall and and
not just decide that it can use something arbitrary.

All new syscalls which deal with time use the Y2038 aware data types and
do not have compat fallbacks because there is no requirement to have
them.

If user space want's to use struct timespec on 32bit nevertheless in the
programm for a new syscall, which is obviously stupid in the context of
Y2038, then it's a user space problem to convert back and forth between
the two data types.

Fix LTP to be Y2038 safe instead.

Thanks,

        tglx

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[Wei Gao]

On Wed, Nov 29, 2023 at 03:56:12PM -0300, André Almeida wrote:
> Hi Wei,
> 
> Em 27/11/2023 09:15, Wei Gao escreveu:
> > On Thu, Nov 23, 2023 at 01:09:55PM -0300, André Almeida wrote:
> > > [+CC Arnd]
> > > 
> > > Hi Wei,
> > > 
> > > Em 23/11/2023 02:31, Wei Gao escreveu:
> > > > From: wei gao <wegao@suse.com>
> > > > 
> > > > Current implementation lead LTP test case futex_waitv failed when compiled with
> > > > -m32. This patch add new compat_sys_futex_waitv to handle m32 mode syscall.
> > > > 
> > > > The failure reason is futex_waitv in m32 mode will deliver kernel with struct
> > > > old_timespec32 timeout, but this struct type can not directly used by current
> > > > sys_futex_waitv implementation.
> > > > 
> > > > The new function copy main logic of current sys_futex_waitv, just update parameter
> > > > type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
> > > > and use get_old_timespec32 within the new function to get timeout value.
> > > > 
> > > 
> > > From, what I recall, we don't want to add new syscalls with old_timespec32,
> > > giving that they will have a limited lifetime. Instead, userspace should be
> > > able to come up with a 64-bit timespec implementation for -m32.
> > > 
> > > Thanks,
> > > 	André
> > 
> > Just a comment, I have checked the glibc latest code but do not see any implemention(*.c) on
> > futex_waitv syscall. So normally you have to do syscall directly with __NR_futex_waitv from
> > userspace. So i guess glibc-side can not covert this struct correctly currently. Correct me if
> > any misunderstanding.
> > 
> 
> futex() has no syscall wrappers in glibc. Userspace needs to figure out
> everything by themselves, including which struct they should use, and I
> don't think that glibc does any conversion. If you create manually a
> timespec64 that works in -m32, and pass this to sycall(__NR_futex_waitv,
> ..., &timeout, ...), it should work correctly. You can read more about how
> glibc is planning to deal with this at [1]. Please let me know if now it's
> more clear :)
> 
> [1] https://sourceware.org/glibc/wiki/Y2038ProofnessDesign

Thanks a lot for your detail explaination and good learning link, it's more clear to me now : )

> 
> > Thanks
> > Wei Gao

Re: [PATCH v1] futex: Add compat_sys_futex_waitv for 32bit compatibility

[Wei Gao]

On Wed, Nov 29, 2023 at 09:54:56PM +0100, Thomas Gleixner wrote:
> On Thu, Nov 23 2023 at 00:31, Wei Gao wrote:
> > The new function copy main logic of current sys_futex_waitv, just update parameter
> > type from "struct __kernel_timespec __user *" to "struct old_timespec32 __user *,"
> > and use get_old_timespec32 within the new function to get timeout
> > value.
> 
> That's not how it works.
> 
> struct __kernel_timespec is the same on 64bit and 32bit syscalls.
> 
> User space has to use the proper type when invoking a syscall and and
> not just decide that it can use something arbitrary.
> 
> All new syscalls which deal with time use the Y2038 aware data types and
> do not have compat fallbacks because there is no requirement to have
> them.
> 
> If user space want's to use struct timespec on 32bit nevertheless in the
> programm for a new syscall, which is obviously stupid in the context of
> Y2038, then it's a user space problem to convert back and forth between
> the two data types.
> 
> Fix LTP to be Y2038 safe instead.

Thanks a lot for your suggestion, will check it.

> 
> Thanks,
> 
>         tglx