linux-um archives
 help / color / mirror / Atom feed
* [PATCH v2] um: add a UML specific futex implementation
@ 2020-11-12 19:57 anton.ivanov
  2020-12-10 22:38 ` Richard Weinberger
  0 siblings, 1 reply; 4+ messages in thread
From: anton.ivanov @ 2020-11-12 19:57 UTC (permalink / raw)
  To: linux-um; +Cc: richard, Anton Ivanov

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

The generic asm futex implementation emulates atomic access to
memory by doing a get_user followed by put_user. These translate
to two mapping operations on UML with paging enabled in the
meantime. This, in turn may end up changing interrupts,
invoking the signal loop, etc.

This replaces the generic implementation by a mapping followed
by an operation on the mapped segment. It is for now limited
to 64 bit as the 32 bit may also need to take care of highmem,
etc.

Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/include/asm/futex.h   |  20 ++++++
 arch/um/kernel/skas/uaccess.c | 119 ++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+)
 create mode 100644 arch/um/include/asm/futex.h

diff --git a/arch/um/include/asm/futex.h b/arch/um/include/asm/futex.h
new file mode 100644
index 000000000000..9af35ab66b6e
--- /dev/null
+++ b/arch/um/include/asm/futex.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_FUTEX_H
+#define _ASM_GENERIC_FUTEX_H
+
+#ifdef CONFIG_64BIT
+
+#include <linux/futex.h>
+#include <linux/uaccess.h>
+#include <asm/errno.h>
+
+
+extern int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
+extern int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
+			      u32 oldval, u32 newval);
+
+#else
+#include <"asm-generic/futex.h">
+#endif /* CONFIG_64BIT */
+
+#endif
diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
index 2dec915abe6f..21a8f2b283bb 100644
--- a/arch/um/kernel/skas/uaccess.c
+++ b/arch/um/kernel/skas/uaccess.c
@@ -11,6 +11,7 @@
 #include <asm/current.h>
 #include <asm/page.h>
 #include <kern_util.h>
+#include <asm/futex.h>
 #include <os.h>
 
 pte_t *virt_to_pte(struct mm_struct *mm, unsigned long addr)
@@ -248,3 +249,121 @@ long __strnlen_user(const void __user *str, long len)
 	return 0;
 }
 EXPORT_SYMBOL(__strnlen_user);
+
+/**
+ * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
+ *			  argument and comparison of the previous
+ *			  futex value with another constant.
+ *
+ * @encoded_op:	encoded operation to execute
+ * @uaddr:	pointer to user space address
+ *
+ * Return:
+ * 0 - On success
+ * -EFAULT - User access resulted in a page fault
+ * -EAGAIN - Atomic operation was unable to complete due to contention
+ * -ENOSYS - Operation not supported
+ */
+int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
+{
+	int oldval, ret;
+	struct page *page;
+	pte_t *pte;
+
+	ret = -EFAULT;
+	if (!access_ok(uaddr, sizeof(*uaddr)))
+		return -EFAULT;
+	preempt_disable();
+	pte = maybe_map((unsigned long) uaddr, 1);
+	if (pte == NULL)
+		goto out_inuser;
+
+	page = pte_page(*pte);
+	pagefault_disable();
+	uaddr = page_address(page) + (((unsigned long) uaddr) & ~PAGE_MASK);
+
+	oldval = *uaddr;
+
+	ret = 0;
+
+	switch (op) {
+	case FUTEX_OP_SET:
+		*uaddr = oparg;
+		break;
+	case FUTEX_OP_ADD:
+		*uaddr += oparg;
+		break;
+	case FUTEX_OP_OR:
+		*uaddr |= oparg;
+		break;
+	case FUTEX_OP_ANDN:
+		*uaddr &= ~oparg;
+		break;
+	case FUTEX_OP_XOR:
+		*uaddr ^= oparg;
+		break;
+	default:
+		ret = -ENOSYS;
+	}
+	pagefault_enable();
+
+out_inuser:
+	preempt_enable();
+
+	if (ret == 0)
+		*oval = oldval;
+
+	return ret;
+}
+EXPORT_SYMBOL(arch_futex_atomic_op_inuser);
+
+/**
+ * futex_atomic_cmpxchg_inatomic() - Compare and exchange the content of the
+ *				uaddr with newval if the current value is
+ *				oldval.
+ * @uval:	pointer to store content of @uaddr
+ * @uaddr:	pointer to user space address
+ * @oldval:	old value
+ * @newval:	new value to store to @uaddr
+ *
+ * Return:
+ * 0 - On success
+ * -EFAULT - User access resulted in a page fault
+ * -EAGAIN - Atomic operation was unable to complete due to contention
+ * -ENOSYS - Function not implemented (only if !HAVE_FUTEX_CMPXCHG)
+ */
+
+int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
+			      u32 oldval, u32 newval)
+{
+	u32 val;
+	struct page *page;
+	pte_t *pte;
+	int ret = -EFAULT;
+
+	if (!access_ok(uaddr, sizeof(*uaddr)))
+		return -EFAULT;
+
+	preempt_disable();
+	pte = maybe_map((unsigned long) uaddr, 1);
+	if (pte == NULL)
+		goto out_inatomic;
+
+	page = pte_page(*pte);
+	pagefault_disable();
+	uaddr = page_address(page) + (((unsigned long) uaddr) & ~PAGE_MASK);
+
+	val = *uaddr;
+
+	if (val == oldval)
+		*uaddr = newval;
+
+	*uval = val;
+	pagefault_enable();
+	ret = 0;
+
+out_inatomic:
+	preempt_enable();
+	return ret;
+}
+EXPORT_SYMBOL(futex_atomic_cmpxchg_inatomic);
-- 
2.20.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] um: add a UML specific futex implementation
  2020-11-12 19:57 [PATCH v2] um: add a UML specific futex implementation anton.ivanov
@ 2020-12-10 22:38 ` Richard Weinberger
  2020-12-10 22:59   ` Anton Ivanov
  2020-12-11  9:54   ` Anton Ivanov
  0 siblings, 2 replies; 4+ messages in thread
From: Richard Weinberger @ 2020-12-10 22:38 UTC (permalink / raw)
  To: Anton Ivanov; +Cc: Richard Weinberger, linux-um

On Thu, Nov 12, 2020 at 8:57 PM <anton.ivanov@cambridgegreys.com> wrote:
>
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>
> The generic asm futex implementation emulates atomic access to
> memory by doing a get_user followed by put_user. These translate
> to two mapping operations on UML with paging enabled in the
> meantime. This, in turn may end up changing interrupts,
> invoking the signal loop, etc.
>
> This replaces the generic implementation by a mapping followed
> by an operation on the mapped segment. It is for now limited
> to 64 bit as the 32 bit may also need to take care of highmem,
> etc.
>
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>  arch/um/include/asm/futex.h   |  20 ++++++
>  arch/um/kernel/skas/uaccess.c | 119 ++++++++++++++++++++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 arch/um/include/asm/futex.h
>
> diff --git a/arch/um/include/asm/futex.h b/arch/um/include/asm/futex.h
> new file mode 100644
> index 000000000000..9af35ab66b6e
> --- /dev/null
> +++ b/arch/um/include/asm/futex.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_GENERIC_FUTEX_H
> +#define _ASM_GENERIC_FUTEX_H
> +
> +#ifdef CONFIG_64BIT
> +
> +#include <linux/futex.h>
> +#include <linux/uaccess.h>
> +#include <asm/errno.h>
> +
> +
> +extern int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
> +extern int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> +                             u32 oldval, u32 newval);
> +
> +#else
> +#include <"asm-generic/futex.h">
> +#endif /* CONFIG_64BIT */
> +
> +#endif
> diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
> index 2dec915abe6f..21a8f2b283bb 100644
> --- a/arch/um/kernel/skas/uaccess.c
> +++ b/arch/um/kernel/skas/uaccess.c
> @@ -11,6 +11,7 @@
>  #include <asm/current.h>
>  #include <asm/page.h>
>  #include <kern_util.h>
> +#include <asm/futex.h>
>  #include <os.h>
>
>  pte_t *virt_to_pte(struct mm_struct *mm, unsigned long addr)
> @@ -248,3 +249,121 @@ long __strnlen_user(const void __user *str, long len)
>         return 0;
>  }
>  EXPORT_SYMBOL(__strnlen_user);
> +
> +/**
> + * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
> + *                       argument and comparison of the previous
> + *                       futex value with another constant.
> + *
> + * @encoded_op:        encoded operation to execute
> + * @uaddr:     pointer to user space address
> + *
> + * Return:
> + * 0 - On success
> + * -EFAULT - User access resulted in a page fault
> + * -EAGAIN - Atomic operation was unable to complete due to contention
> + * -ENOSYS - Operation not supported
> + */
> +int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)

Does this even build in 32bits mode?
AFAICT the generic futex implementation has
arch_futex_atomic_op_inuser() as static inline...

-- 
Thanks,
//richard

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] um: add a UML specific futex implementation
  2020-12-10 22:38 ` Richard Weinberger
@ 2020-12-10 22:59   ` Anton Ivanov
  2020-12-11  9:54   ` Anton Ivanov
  1 sibling, 0 replies; 4+ messages in thread
From: Anton Ivanov @ 2020-12-10 22:59 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: Richard Weinberger, linux-um

On 10/12/2020 22:38, Richard Weinberger wrote:
> On Thu, Nov 12, 2020 at 8:57 PM <anton.ivanov@cambridgegreys.com> wrote:
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> The generic asm futex implementation emulates atomic access to
>> memory by doing a get_user followed by put_user. These translate
>> to two mapping operations on UML with paging enabled in the
>> meantime. This, in turn may end up changing interrupts,
>> invoking the signal loop, etc.
>>
>> This replaces the generic implementation by a mapping followed
>> by an operation on the mapped segment. It is for now limited
>> to 64 bit as the 32 bit may also need to take care of highmem,
>> etc.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   arch/um/include/asm/futex.h   |  20 ++++++
>>   arch/um/kernel/skas/uaccess.c | 119 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 139 insertions(+)
>>   create mode 100644 arch/um/include/asm/futex.h
>>
>> diff --git a/arch/um/include/asm/futex.h b/arch/um/include/asm/futex.h
>> new file mode 100644
>> index 000000000000..9af35ab66b6e
>> --- /dev/null
>> +++ b/arch/um/include/asm/futex.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_GENERIC_FUTEX_H
>> +#define _ASM_GENERIC_FUTEX_H
>> +
>> +#ifdef CONFIG_64BIT
>> +
>> +#include <linux/futex.h>
>> +#include <linux/uaccess.h>
>> +#include <asm/errno.h>
>> +
>> +
>> +extern int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
>> +extern int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>> +                             u32 oldval, u32 newval);
>> +
>> +#else
>> +#include <"asm-generic/futex.h">
>> +#endif /* CONFIG_64BIT */
>> +
>> +#endif
>> diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
>> index 2dec915abe6f..21a8f2b283bb 100644
>> --- a/arch/um/kernel/skas/uaccess.c
>> +++ b/arch/um/kernel/skas/uaccess.c
>> @@ -11,6 +11,7 @@
>>   #include <asm/current.h>
>>   #include <asm/page.h>
>>   #include <kern_util.h>
>> +#include <asm/futex.h>
>>   #include <os.h>
>>
>>   pte_t *virt_to_pte(struct mm_struct *mm, unsigned long addr)
>> @@ -248,3 +249,121 @@ long __strnlen_user(const void __user *str, long len)
>>          return 0;
>>   }
>>   EXPORT_SYMBOL(__strnlen_user);
>> +
>> +/**
>> + * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
>> + *                       argument and comparison of the previous
>> + *                       futex value with another constant.
>> + *
>> + * @encoded_op:        encoded operation to execute
>> + * @uaddr:     pointer to user space address
>> + *
>> + * Return:
>> + * 0 - On success
>> + * -EFAULT - User access resulted in a page fault
>> + * -EAGAIN - Atomic operation was unable to complete due to contention
>> + * -ENOSYS - Operation not supported
>> + */
>> +int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> Does this even build in 32bits mode?
> AFAICT the generic futex implementation has
> arch_futex_atomic_op_inuser() as static inline...
>
I did not try, mea culpa. It was pulling the generic before. So generic 
should work.

I can retest this one and the others in 32 bit mode tomorrow (most of 
them were written to fall back to generic implementation for 32).

A.

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] um: add a UML specific futex implementation
  2020-12-10 22:38 ` Richard Weinberger
  2020-12-10 22:59   ` Anton Ivanov
@ 2020-12-11  9:54   ` Anton Ivanov
  1 sibling, 0 replies; 4+ messages in thread
From: Anton Ivanov @ 2020-12-11  9:54 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: Richard Weinberger, linux-um

On 10/12/2020 22:38, Richard Weinberger wrote:
> On Thu, Nov 12, 2020 at 8:57 PM <anton.ivanov@cambridgegreys.com> wrote:
>>
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> The generic asm futex implementation emulates atomic access to
>> memory by doing a get_user followed by put_user. These translate
>> to two mapping operations on UML with paging enabled in the
>> meantime. This, in turn may end up changing interrupts,
>> invoking the signal loop, etc.
>>
>> This replaces the generic implementation by a mapping followed
>> by an operation on the mapped segment. It is for now limited
>> to 64 bit as the 32 bit may also need to take care of highmem,
>> etc.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   arch/um/include/asm/futex.h   |  20 ++++++
>>   arch/um/kernel/skas/uaccess.c | 119 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 139 insertions(+)
>>   create mode 100644 arch/um/include/asm/futex.h
>>
>> diff --git a/arch/um/include/asm/futex.h b/arch/um/include/asm/futex.h
>> new file mode 100644
>> index 000000000000..9af35ab66b6e
>> --- /dev/null
>> +++ b/arch/um/include/asm/futex.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_GENERIC_FUTEX_H
>> +#define _ASM_GENERIC_FUTEX_H
>> +
>> +#ifdef CONFIG_64BIT
>> +
>> +#include <linux/futex.h>
>> +#include <linux/uaccess.h>
>> +#include <asm/errno.h>
>> +
>> +
>> +extern int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
>> +extern int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>> +                             u32 oldval, u32 newval);
>> +
>> +#else
>> +#include <"asm-generic/futex.h">
>> +#endif /* CONFIG_64BIT */
>> +
>> +#endif
>> diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
>> index 2dec915abe6f..21a8f2b283bb 100644
>> --- a/arch/um/kernel/skas/uaccess.c
>> +++ b/arch/um/kernel/skas/uaccess.c
>> @@ -11,6 +11,7 @@
>>   #include <asm/current.h>
>>   #include <asm/page.h>
>>   #include <kern_util.h>
>> +#include <asm/futex.h>
>>   #include <os.h>
>>
>>   pte_t *virt_to_pte(struct mm_struct *mm, unsigned long addr)
>> @@ -248,3 +249,121 @@ long __strnlen_user(const void __user *str, long len)
>>          return 0;
>>   }
>>   EXPORT_SYMBOL(__strnlen_user);
>> +
>> +/**
>> + * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
>> + *                       argument and comparison of the previous
>> + *                       futex value with another constant.
>> + *
>> + * @encoded_op:        encoded operation to execute
>> + * @uaddr:     pointer to user space address
>> + *
>> + * Return:
>> + * 0 - On success
>> + * -EFAULT - User access resulted in a page fault
>> + * -EAGAIN - Atomic operation was unable to complete due to contention
>> + * -ENOSYS - Operation not supported
>> + */
>> +int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> 
> Does this even build in 32bits mode?
> AFAICT the generic futex implementation has
> arch_futex_atomic_op_inuser() as static inline...
> 

It's an inline, but it includes two very slow non-inline calls:

https://elixir.bootlin.com/linux/latest/source/include/asm-generic/futex.h#L39

if (unlikely(get_user(oldval, uaddr) != 0))

and

https://elixir.bootlin.com/linux/latest/source/include/asm-generic/futex.h#L65

if (ret == 0 && unlikely(put_user(tmp, uaddr) != 0))

So better to have not inline, but actually fast and done in a single atomic operation on the target memory location.

I will fix all 32 bit and resubmit (and promise to test it on 32 next time :)

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-12-11  9:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-12 19:57 [PATCH v2] um: add a UML specific futex implementation anton.ivanov
2020-12-10 22:38 ` Richard Weinberger
2020-12-10 22:59   ` Anton Ivanov
2020-12-11  9:54   ` Anton Ivanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox