* Re: [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Shawn Landden @ 2019-11-04 0:51 UTC (permalink / raw)
To: Thomas Gleixner
Cc: libc-alpha@sourceware.org, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, Arnd Bergmann, Deepa Dinamani,
Oleg Nesterov, Andrew Morton, Catalin Marinas, Keith Packard
In-Reply-To: <20191104002909.25783-1-shawn@git.icu>
I am sorry, I will fix this and resubmit.
03.11.2019, 19:29, "Shawn Landden" <shawn@git.icu>:
> The robust futexes ABI was designed to be flexible to changing ABIs in
> glibc, however it did not take into consideration that this ABI is
> particularly sticky, and suffers from lock-step problems, due to the
> fact that the ABI is shared between processes. This introduces a new
> size in set_robust_list that takes an additional futex_offset2 value
> so that two locking ABIs can be used at the same time.
>
> If this new ABI is used, then bit 1 of the *next pointer of the
> user-space robust_list indicates that the futex_offset2 value should
> be used in place of the existing futex_offset.
>
> The use case for this is sharing locks between 32-bit and 64-bit
> processes, which Linux supports, but glibc does not, and is difficult
> to implement with the current Linux support because of mix-matched
> ABIs. Keith Packard has complained about this:
> https://keithp.com/blogs/Shared_Memory_Fences/
>
> This can also be used to add a new ABI that uses smaller structs,
> as the existing ABI on x86_64 is a minimum of 32 bytes, and 20 bytes
> would suffice.
>
> v2: fix size of compat_extended_robust_list_head
> fix some issues with number literals being implicitly ints
> ---
> include/linux/compat.h | 5 +
> include/linux/sched.h | 6 ++
> include/uapi/linux/futex.h | 31 +++++++
> kernel/futex.c | 182 ++++++++++++++++++++++++-------------
> 4 files changed, 160 insertions(+), 64 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 16dafd9f4b86..00a0741bf658 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -379,10 +379,15 @@ struct compat_robust_list_head {
> struct compat_robust_list list;
> compat_long_t futex_offset;
> compat_uptr_t list_op_pending;
> };
>
> +struct compat_extended_robust_list_head {
> + struct compat_robust_list_head list_head;
> + compat_long_t futex_offset2;
> +};
> +
> #ifdef CONFIG_COMPAT_OLD_SIGACTION
> struct compat_old_sigaction {
> compat_uptr_t sa_handler;
> compat_old_sigset_t sa_mask;
> compat_ulong_t sa_flags;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 9f51932bd543..894258fd44ac 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1057,10 +1057,16 @@ struct task_struct {
> #ifdef CONFIG_X86_CPU_RESCTRL
> u32 closid;
> u32 rmid;
> #endif
> #ifdef CONFIG_FUTEX
> + /*
> + * bottom two bits are masked
> + * 0: struct extended_robust_list_head
> + * 1: struct robust_list_head
> + * same for compat_robust_list
> + */
> struct robust_list_head __user *robust_list;
> #ifdef CONFIG_COMPAT
> struct compat_robust_list_head __user *compat_robust_list;
> #endif
> struct list_head pi_state_list;
> diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
> index a89eb0accd5e..30c08e07f26b 100644
> --- a/include/uapi/linux/futex.h
> +++ b/include/uapi/linux/futex.h
> @@ -92,10 +92,41 @@ struct robust_list_head {
> * so only truly owned locks will be handled.
> */
> struct robust_list __user *list_op_pending;
> };
>
> +/*
> + * Extensible per-thread list head:
> + *
> + * As locks are shared between processes, the futex_offset field
> + * has ABI lock-stepping issues, which the original robust_list_head
> + * structure did not anticipate. (And which prevents 32-bit/64-bit
> + * interoperability, as well as shrinking of mutex structures).
> + * This new extensible_robust_list_head allows multiple
> + * concurrent futex_offset values, chosen using the bottom 2 bits of the
> + * robust_list *next pointer, which are now masked in BOTH the old and
> + * new ABI.
> + *
> + * Note: this structure is part of the syscall ABI like
> + * robust_list_head above, and must have a different size than
> + * robust_list_head.
> + *
> + */
> +struct extended_robust_list_head {
> + struct robust_list_head list_head;
> +
> + /*
> + * These relative offsets are set by user-space. They give the kernel
> + * the relative position of the futex field to examine, based on the
> + * bit 1 *next pointer.
> + * The original version was insufficiently flexible. Locks are held
> + * in shared memory between processes, and a process might want to hold
> + * locks of two ABIs at the same time.
> + */
> + long futex_offset2;
> +};
> +
> /*
> * Are there any waiters for this robust futex:
> */
> #define FUTEX_WAITERS 0x80000000
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 6d50728ef2e7..3a17d2d63178 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3396,17 +3396,20 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
> SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
> size_t, len)
> {
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
> - /*
> - * The kernel knows only one size for now:
> - */
> - if (unlikely(len != sizeof(*head)))
> +
> + if (unlikely(len != sizeof(struct robust_list_head) &&
> + len != sizeof(struct extensible_robust_list_head)))
> return -EINVAL;
>
> - current->robust_list = head;
> + current->robust_list = head & 0b11UL;
> + BUILD_BUG_ON(sizeof(struct robust_list_head) ==
> + sizeof(struct extended_robust_list_head));
> + if (len == sizeof(struct robust_list_head))
> + current->robust_list |= 1;
>
> return 0;
> }
>
> /**
> @@ -3419,10 +3422,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> struct robust_list_head __user * __user *, head_ptr,
> size_t __user *, len_ptr)
> {
> struct robust_list_head __user *head;
> unsigned long ret;
> + size_t len;
> struct task_struct *p;
>
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
>
> @@ -3439,14 +3443,18 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
>
> ret = -EPERM;
> if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> - head = p->robust_list;
> + head = p->robust_list & ~0b11UL;
> + if (p->robust_list & 0b11 == 0b1)
> + len = sizeof(struct robust_list_head);
> + else
> + len = sizeof(struct extended_robust_list_head);
> rcu_read_unlock();
>
> - if (put_user(sizeof(*head), len_ptr))
> + if (put_user(len, len_ptr))
> return -EFAULT;
> return put_user(head, head_ptr);
>
> err_unlock:
> rcu_read_unlock();
> @@ -3524,23 +3532,26 @@ static int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int p
>
> return 0;
> }
>
> /*
> - * Fetch a robust-list pointer. Bit 0 signals PI futexes:
> + * Fetch a robust-list pointer. Bit 0 signals PI futexes. Bit 1 choses which
> + * futex_offset to use:
> */
> static inline int fetch_robust_entry(struct robust_list __user **entry,
> struct robust_list __user * __user *head,
> - unsigned int *pi)
> + unsigned int *pi,
> + *unsigned int *second_abi)
> {
> unsigned long uentry;
>
> if (get_user(uentry, (unsigned long __user *)head))
> return -EFAULT;
>
> - *entry = (void __user *)(uentry & ~1UL);
> + *entry = (void __user *)(uentry & ~0b11UL);
> *pi = uentry & 1;
> + *second_abi = uentry & 0b10;
>
> return 0;
> }
>
> /*
> @@ -3549,69 +3560,84 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
> *
> * We silently return on any sign of list-walking problem.
> */
> void exit_robust_list(struct task_struct *curr)
> {
> - struct robust_list_head __user *head = curr->robust_list;
> - struct robust_list __user *entry, *next_entry, *pending;
> - unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
> - unsigned int uninitialized_var(next_pi);
> - unsigned long futex_offset;
> + struct robust_list_head __user *head_ptr = curr->robust_list & ~1UL;
> + unsigned int is_extended_list = curr->robust_list & 1 == 0;
> + struct extended_robust_list_head head;
> + struct robust_list __user *entry = &head->list_head.list.next,
> + *next_entry, *pending;
> + unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
> + second_abip;
> + unsigned int uninitialized_var(next_pi),
> + uninitialized_var(next_second_abi);
> int rc;
>
> if (!futex_cmpxchg_enabled)
> return;
>
> /*
> - * Fetch the list head (which was registered earlier, via
> - * sys_set_robust_list()):
> + * fetch_robust_entry code is duplicated here to avoid excessive calls
> + * to get_user()
> */
> - if (fetch_robust_entry(&entry, &head->list.next, &pi))
> - return;
> - /*
> - * Fetch the relative futex offset:
> - */
> - if (get_user(futex_offset, &head->futex_offset))
> - return;
> - /*
> - * Fetch any possibly pending lock-add first, and handle it
> - * if it exists:
> - */
> - if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
> - return;
> + if (is_extended_list) {
> + if (get_user(head, (struct extended_robust_list_head *)
> + head_ptr))
> + return;
> + } else {
> + if (get_user(head.list_head, head_ptr))
> + return;
> + }
> +
> + pi = head.list_head.list.next & 1;
> + second_abi = head.list_head.list.next & 0b10;
> + head.list_head.list.next &= ~0b11UL;
> + pip = head.list_head.list_op_pending & 1;
> + second_abip = head.list_head.list_op_pending & 0b10;
> + head.list_head.list_op_pending &= ~0b11UL;
>
> next_entry = NULL; /* avoid warning with gcc */
> - while (entry != &head->list) {
> + while (entry != &head->list_head.list) {
> /*
> * Fetch the next entry in the list before calling
> * handle_futex_death:
> */
> - rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi);
> + rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi,
> + &next_second_abi);
> /*
> * A pending lock might already be on the list, so
> * don't process it twice:
> */
> - if (entry != pending)
> + if (entry != pending) {
> + long futex_offset = second_abi ?
> + head.futex_offset2 :
> + head.list_head.futex_offset;
> if (handle_futex_death((void __user *)entry + futex_offset,
> curr, pi))
> return;
> + }
> if (rc)
> return;
> entry = next_entry;
> pi = next_pi;
> + second_abi = next_second_abi;
> /*
> * Avoid excessively long or circular lists:
> */
> if (!--limit)
> break;
>
> cond_resched();
> }
>
> - if (pending)
> + if (pending) {
> + long futex_offset = second_abip ? head.futex_offset2 :
> + head.list_head.futex_offset;
> handle_futex_death((void __user *)pending + futex_offset,
> curr, pip);
> + }
> }
>
> long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
> u32 __user *uaddr2, u32 val2, u32 val3)
> {
> @@ -3707,21 +3733,25 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
> }
>
> #ifdef CONFIG_COMPAT
> /*
> - * Fetch a robust-list pointer. Bit 0 signals PI futexes:
> + * Fetch a robust-list pointer. Bit 0 signals PI futexes.
> + * Bit 1 choses which futex_offset to use:
> */
> static inline int
> -compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
> - compat_uptr_t __user *head, unsigned int *pi)
> +compat_fetch_robust_entry(compat_uptr_t *uentry,
> + struct robust_list __user **entry,
> + compat_uptr_t __user *head, unsigned int *pi,
> + unsigned int *second_abi)
> {
> if (get_user(*uentry, head))
> return -EFAULT;
>
> - *entry = compat_ptr((*uentry) & ~1);
> + *entry = compat_ptr((*uentry) & ~0b11);
> *pi = (unsigned int)(*uentry) & 1;
> + *second_abi = (unsigned int)(*uentry) & 0b10;
>
> return 0;
> }
>
> static void __user *futex_uaddr(struct robust_list __user *entry,
> @@ -3739,72 +3769,86 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
> *
> * We silently return on any sign of list-walking problem.
> */
> void compat_exit_robust_list(struct task_struct *curr)
> {
> - struct compat_robust_list_head __user *head = curr->compat_robust_list;
> - struct robust_list __user *entry, *next_entry, *pending;
> - unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
> - unsigned int uninitialized_var(next_pi);
> + struct compat_robust_list_head __user *head = curr->compat_robust_list &
> + ~1UL;
> + unsigned int is_extended_list = curr->compat_robust_list & 1 == 0;
> + struct compat_extended_robust_list_head head;
> + struct robust_list __user *entry = &head->list_head.list.next,
> + *next_entry, *pending;
> + unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
> + second_abip;
> + unsigned int uninitialized_var(next_pi),
> + uninitialized_var(next_second_abi);
> compat_uptr_t uentry, next_uentry, upending;
> - compat_long_t futex_offset;
> int rc;
>
> if (!futex_cmpxchg_enabled)
> return;
>
> /*
> - * Fetch the list head (which was registered earlier, via
> - * sys_set_robust_list()):
> - */
> - if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
> - return;
> - /*
> - * Fetch the relative futex offset:
> - */
> - if (get_user(futex_offset, &head->futex_offset))
> - return;
> - /*
> - * Fetch any possibly pending lock-add first, and handle it
> - * if it exists:
> + * compat_fetch_robust_entry code is duplicated here to avoid excessive
> + * calls to get_user()
> */
> - if (compat_fetch_robust_entry(&upending, &pending,
> - &head->list_op_pending, &pip))
> - return;
> + if (is_extended_list) {
> + if (get_user(head, (struct compat_extended_robust_list_head *)
> + head_ptr))
> + return;
> + } else {
> + if (get_user(head.list_head, head_ptr))
> + return;
> + }
> +
> + pi = head.list_head.list.next & 1;
> + second_abi = head.list_head.list.next & 0b10;
> + head.list_head.list.next &= ~0b11UL;
> + pip = head.list_head.list_op_pending & 1;
> + second_abip = head.list_head.list_op_pending & 0b10;
> + head.list_head.list_op_pending &= ~0b11UL;
>
> next_entry = NULL; /* avoid warning with gcc */
> while (entry != (struct robust_list __user *) &head->list) {
> /*
> * Fetch the next entry in the list before calling
> * handle_futex_death:
> */
> rc = compat_fetch_robust_entry(&next_uentry, &next_entry,
> - (compat_uptr_t __user *)&entry->next, &next_pi);
> + (compat_uptr_t __user *)&entry->next, &next_pi,
> + &next_second_abi);
> /*
> * A pending lock might already be on the list, so
> * dont process it twice:
> */
> if (entry != pending) {
> + compat_long_t futex_offset = second_abi ?
> + head.futex_offset2 :
> + head.list_head.futex_offset;
> void __user *uaddr = futex_uaddr(entry, futex_offset);
>
> if (handle_futex_death(uaddr, curr, pi))
> return;
> }
> if (rc)
> return;
> uentry = next_uentry;
> entry = next_entry;
> pi = next_pi;
> + second_abi = next_second_abi;
> /*
> * Avoid excessively long or circular lists:
> */
> if (!--limit)
> break;
>
> cond_resched();
> }
> if (pending) {
> + compat_long_t futex_offset = second_abip ?
> + head.futex_offset2 :
> + head.list_head.futex_offset;
> void __user *uaddr = futex_uaddr(pending, futex_offset);
>
> handle_futex_death(uaddr, curr, pip);
> }
> }
> @@ -3814,23 +3858,29 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
> compat_size_t, len)
> {
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
>
> - if (unlikely(len != sizeof(*head)))
> + if (unlikely(len != sizeof(struct compat_robust_list_head) &&
> + len != sizeof(struct compat_extended_robust_list_head)))
> return -EINVAL;
>
> - current->compat_robust_list = head;
> + current->compat_robust_list = head & ~0b11;
> + BUILD_BUG_ON(sizeof(compat_robust_list_head) ==
> + sizeof(compat_extended_robust_list_head));
> + if (len == sizeof(compat_robust_list_head))
> + current->compat_robust_list |= 0b1;
>
> return 0;
> }
>
> COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
> compat_uptr_t __user *, head_ptr,
> compat_size_t __user *, len_ptr)
> {
> struct compat_robust_list_head __user *head;
> + size_t len;
> unsigned long ret;
> struct task_struct *p;
>
> if (!futex_cmpxchg_enabled)
> return -ENOSYS;
> @@ -3848,14 +3898,18 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
>
> ret = -EPERM;
> if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> - head = p->compat_robust_list;
> + head = p->compat_robust_list & ~0b11;
> + if (p->compat_robust_list & 0b11 == 0b1)
> + len = sizeof(struct compat_robust_list_head);
> + else
> + len = sizeof(struct compat_extended_robust_list_head);
> rcu_read_unlock();
>
> - if (put_user(sizeof(*head), len_ptr))
> + if (put_user(len, len_ptr))
> return -EFAULT;
> return put_user(ptr_to_compat(head), head_ptr);
>
> err_unlock:
> rcu_read_unlock();
> --
> 2.20.1
--
Shawn Landden
^ permalink raw reply
* [RFC v2 PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Shawn Landden @ 2019-11-04 0:29 UTC (permalink / raw)
To: Thomas Gleixner
Cc: libc-alpha, linux-api, linux-kernel, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Shawn Landden, Keith Packard
The robust futexes ABI was designed to be flexible to changing ABIs in
glibc, however it did not take into consideration that this ABI is
particularly sticky, and suffers from lock-step problems, due to the
fact that the ABI is shared between processes. This introduces a new
size in set_robust_list that takes an additional futex_offset2 value
so that two locking ABIs can be used at the same time.
If this new ABI is used, then bit 1 of the *next pointer of the
user-space robust_list indicates that the futex_offset2 value should
be used in place of the existing futex_offset.
The use case for this is sharing locks between 32-bit and 64-bit
processes, which Linux supports, but glibc does not, and is difficult
to implement with the current Linux support because of mix-matched
ABIs. Keith Packard has complained about this:
https://keithp.com/blogs/Shared_Memory_Fences/
This can also be used to add a new ABI that uses smaller structs,
as the existing ABI on x86_64 is a minimum of 32 bytes, and 20 bytes
would suffice.
v2: fix size of compat_extended_robust_list_head
fix some issues with number literals being implicitly ints
---
include/linux/compat.h | 5 +
include/linux/sched.h | 6 ++
include/uapi/linux/futex.h | 31 +++++++
kernel/futex.c | 182 ++++++++++++++++++++++++-------------
4 files changed, 160 insertions(+), 64 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 16dafd9f4b86..00a0741bf658 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -379,10 +379,15 @@ struct compat_robust_list_head {
struct compat_robust_list list;
compat_long_t futex_offset;
compat_uptr_t list_op_pending;
};
+struct compat_extended_robust_list_head {
+ struct compat_robust_list_head list_head;
+ compat_long_t futex_offset2;
+};
+
#ifdef CONFIG_COMPAT_OLD_SIGACTION
struct compat_old_sigaction {
compat_uptr_t sa_handler;
compat_old_sigset_t sa_mask;
compat_ulong_t sa_flags;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..894258fd44ac 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1057,10 +1057,16 @@ struct task_struct {
#ifdef CONFIG_X86_CPU_RESCTRL
u32 closid;
u32 rmid;
#endif
#ifdef CONFIG_FUTEX
+ /*
+ * bottom two bits are masked
+ * 0: struct extended_robust_list_head
+ * 1: struct robust_list_head
+ * same for compat_robust_list
+ */
struct robust_list_head __user *robust_list;
#ifdef CONFIG_COMPAT
struct compat_robust_list_head __user *compat_robust_list;
#endif
struct list_head pi_state_list;
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index a89eb0accd5e..30c08e07f26b 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -92,10 +92,41 @@ struct robust_list_head {
* so only truly owned locks will be handled.
*/
struct robust_list __user *list_op_pending;
};
+/*
+ * Extensible per-thread list head:
+ *
+ * As locks are shared between processes, the futex_offset field
+ * has ABI lock-stepping issues, which the original robust_list_head
+ * structure did not anticipate. (And which prevents 32-bit/64-bit
+ * interoperability, as well as shrinking of mutex structures).
+ * This new extensible_robust_list_head allows multiple
+ * concurrent futex_offset values, chosen using the bottom 2 bits of the
+ * robust_list *next pointer, which are now masked in BOTH the old and
+ * new ABI.
+ *
+ * Note: this structure is part of the syscall ABI like
+ * robust_list_head above, and must have a different size than
+ * robust_list_head.
+ *
+ */
+struct extended_robust_list_head {
+ struct robust_list_head list_head;
+
+ /*
+ * These relative offsets are set by user-space. They give the kernel
+ * the relative position of the futex field to examine, based on the
+ * bit 1 *next pointer.
+ * The original version was insufficiently flexible. Locks are held
+ * in shared memory between processes, and a process might want to hold
+ * locks of two ABIs at the same time.
+ */
+ long futex_offset2;
+};
+
/*
* Are there any waiters for this robust futex:
*/
#define FUTEX_WAITERS 0x80000000
diff --git a/kernel/futex.c b/kernel/futex.c
index 6d50728ef2e7..3a17d2d63178 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3396,17 +3396,20 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
size_t, len)
{
if (!futex_cmpxchg_enabled)
return -ENOSYS;
- /*
- * The kernel knows only one size for now:
- */
- if (unlikely(len != sizeof(*head)))
+
+ if (unlikely(len != sizeof(struct robust_list_head) &&
+ len != sizeof(struct extensible_robust_list_head)))
return -EINVAL;
- current->robust_list = head;
+ current->robust_list = head & 0b11UL;
+ BUILD_BUG_ON(sizeof(struct robust_list_head) ==
+ sizeof(struct extended_robust_list_head));
+ if (len == sizeof(struct robust_list_head))
+ current->robust_list |= 1;
return 0;
}
/**
@@ -3419,10 +3422,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head __user * __user *, head_ptr,
size_t __user *, len_ptr)
{
struct robust_list_head __user *head;
unsigned long ret;
+ size_t len;
struct task_struct *p;
if (!futex_cmpxchg_enabled)
return -ENOSYS;
@@ -3439,14 +3443,18 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
ret = -EPERM;
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->robust_list;
+ head = p->robust_list & ~0b11UL;
+ if (p->robust_list & 0b11 == 0b1)
+ len = sizeof(struct robust_list_head);
+ else
+ len = sizeof(struct extended_robust_list_head);
rcu_read_unlock();
- if (put_user(sizeof(*head), len_ptr))
+ if (put_user(len, len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
err_unlock:
rcu_read_unlock();
@@ -3524,23 +3532,26 @@ static int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int p
return 0;
}
/*
- * Fetch a robust-list pointer. Bit 0 signals PI futexes:
+ * Fetch a robust-list pointer. Bit 0 signals PI futexes. Bit 1 choses which
+ * futex_offset to use:
*/
static inline int fetch_robust_entry(struct robust_list __user **entry,
struct robust_list __user * __user *head,
- unsigned int *pi)
+ unsigned int *pi,
+ *unsigned int *second_abi)
{
unsigned long uentry;
if (get_user(uentry, (unsigned long __user *)head))
return -EFAULT;
- *entry = (void __user *)(uentry & ~1UL);
+ *entry = (void __user *)(uentry & ~0b11UL);
*pi = uentry & 1;
+ *second_abi = uentry & 0b10;
return 0;
}
/*
@@ -3549,69 +3560,84 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
void exit_robust_list(struct task_struct *curr)
{
- struct robust_list_head __user *head = curr->robust_list;
- struct robust_list __user *entry, *next_entry, *pending;
- unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
- unsigned int uninitialized_var(next_pi);
- unsigned long futex_offset;
+ struct robust_list_head __user *head_ptr = curr->robust_list & ~1UL;
+ unsigned int is_extended_list = curr->robust_list & 1 == 0;
+ struct extended_robust_list_head head;
+ struct robust_list __user *entry = &head->list_head.list.next,
+ *next_entry, *pending;
+ unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
+ second_abip;
+ unsigned int uninitialized_var(next_pi),
+ uninitialized_var(next_second_abi);
int rc;
if (!futex_cmpxchg_enabled)
return;
/*
- * Fetch the list head (which was registered earlier, via
- * sys_set_robust_list()):
+ * fetch_robust_entry code is duplicated here to avoid excessive calls
+ * to get_user()
*/
- if (fetch_robust_entry(&entry, &head->list.next, &pi))
- return;
- /*
- * Fetch the relative futex offset:
- */
- if (get_user(futex_offset, &head->futex_offset))
- return;
- /*
- * Fetch any possibly pending lock-add first, and handle it
- * if it exists:
- */
- if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
- return;
+ if (is_extended_list) {
+ if (get_user(head, (struct extended_robust_list_head *)
+ head_ptr))
+ return;
+ } else {
+ if (get_user(head.list_head, head_ptr))
+ return;
+ }
+
+ pi = head.list_head.list.next & 1;
+ second_abi = head.list_head.list.next & 0b10;
+ head.list_head.list.next &= ~0b11UL;
+ pip = head.list_head.list_op_pending & 1;
+ second_abip = head.list_head.list_op_pending & 0b10;
+ head.list_head.list_op_pending &= ~0b11UL;
next_entry = NULL; /* avoid warning with gcc */
- while (entry != &head->list) {
+ while (entry != &head->list_head.list) {
/*
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
- rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi);
+ rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi,
+ &next_second_abi);
/*
* A pending lock might already be on the list, so
* don't process it twice:
*/
- if (entry != pending)
+ if (entry != pending) {
+ long futex_offset = second_abi ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
if (handle_futex_death((void __user *)entry + futex_offset,
curr, pi))
return;
+ }
if (rc)
return;
entry = next_entry;
pi = next_pi;
+ second_abi = next_second_abi;
/*
* Avoid excessively long or circular lists:
*/
if (!--limit)
break;
cond_resched();
}
- if (pending)
+ if (pending) {
+ long futex_offset = second_abip ? head.futex_offset2 :
+ head.list_head.futex_offset;
handle_futex_death((void __user *)pending + futex_offset,
curr, pip);
+ }
}
long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
u32 __user *uaddr2, u32 val2, u32 val3)
{
@@ -3707,21 +3733,25 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
}
#ifdef CONFIG_COMPAT
/*
- * Fetch a robust-list pointer. Bit 0 signals PI futexes:
+ * Fetch a robust-list pointer. Bit 0 signals PI futexes.
+ * Bit 1 choses which futex_offset to use:
*/
static inline int
-compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
- compat_uptr_t __user *head, unsigned int *pi)
+compat_fetch_robust_entry(compat_uptr_t *uentry,
+ struct robust_list __user **entry,
+ compat_uptr_t __user *head, unsigned int *pi,
+ unsigned int *second_abi)
{
if (get_user(*uentry, head))
return -EFAULT;
- *entry = compat_ptr((*uentry) & ~1);
+ *entry = compat_ptr((*uentry) & ~0b11);
*pi = (unsigned int)(*uentry) & 1;
+ *second_abi = (unsigned int)(*uentry) & 0b10;
return 0;
}
static void __user *futex_uaddr(struct robust_list __user *entry,
@@ -3739,72 +3769,86 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
*
* We silently return on any sign of list-walking problem.
*/
void compat_exit_robust_list(struct task_struct *curr)
{
- struct compat_robust_list_head __user *head = curr->compat_robust_list;
- struct robust_list __user *entry, *next_entry, *pending;
- unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
- unsigned int uninitialized_var(next_pi);
+ struct compat_robust_list_head __user *head = curr->compat_robust_list &
+ ~1UL;
+ unsigned int is_extended_list = curr->compat_robust_list & 1 == 0;
+ struct compat_extended_robust_list_head head;
+ struct robust_list __user *entry = &head->list_head.list.next,
+ *next_entry, *pending;
+ unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
+ second_abip;
+ unsigned int uninitialized_var(next_pi),
+ uninitialized_var(next_second_abi);
compat_uptr_t uentry, next_uentry, upending;
- compat_long_t futex_offset;
int rc;
if (!futex_cmpxchg_enabled)
return;
/*
- * Fetch the list head (which was registered earlier, via
- * sys_set_robust_list()):
- */
- if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
- return;
- /*
- * Fetch the relative futex offset:
- */
- if (get_user(futex_offset, &head->futex_offset))
- return;
- /*
- * Fetch any possibly pending lock-add first, and handle it
- * if it exists:
+ * compat_fetch_robust_entry code is duplicated here to avoid excessive
+ * calls to get_user()
*/
- if (compat_fetch_robust_entry(&upending, &pending,
- &head->list_op_pending, &pip))
- return;
+ if (is_extended_list) {
+ if (get_user(head, (struct compat_extended_robust_list_head *)
+ head_ptr))
+ return;
+ } else {
+ if (get_user(head.list_head, head_ptr))
+ return;
+ }
+
+ pi = head.list_head.list.next & 1;
+ second_abi = head.list_head.list.next & 0b10;
+ head.list_head.list.next &= ~0b11UL;
+ pip = head.list_head.list_op_pending & 1;
+ second_abip = head.list_head.list_op_pending & 0b10;
+ head.list_head.list_op_pending &= ~0b11UL;
next_entry = NULL; /* avoid warning with gcc */
while (entry != (struct robust_list __user *) &head->list) {
/*
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
rc = compat_fetch_robust_entry(&next_uentry, &next_entry,
- (compat_uptr_t __user *)&entry->next, &next_pi);
+ (compat_uptr_t __user *)&entry->next, &next_pi,
+ &next_second_abi);
/*
* A pending lock might already be on the list, so
* dont process it twice:
*/
if (entry != pending) {
+ compat_long_t futex_offset = second_abi ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
void __user *uaddr = futex_uaddr(entry, futex_offset);
if (handle_futex_death(uaddr, curr, pi))
return;
}
if (rc)
return;
uentry = next_uentry;
entry = next_entry;
pi = next_pi;
+ second_abi = next_second_abi;
/*
* Avoid excessively long or circular lists:
*/
if (!--limit)
break;
cond_resched();
}
if (pending) {
+ compat_long_t futex_offset = second_abip ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
void __user *uaddr = futex_uaddr(pending, futex_offset);
handle_futex_death(uaddr, curr, pip);
}
}
@@ -3814,23 +3858,29 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
compat_size_t, len)
{
if (!futex_cmpxchg_enabled)
return -ENOSYS;
- if (unlikely(len != sizeof(*head)))
+ if (unlikely(len != sizeof(struct compat_robust_list_head) &&
+ len != sizeof(struct compat_extended_robust_list_head)))
return -EINVAL;
- current->compat_robust_list = head;
+ current->compat_robust_list = head & ~0b11;
+ BUILD_BUG_ON(sizeof(compat_robust_list_head) ==
+ sizeof(compat_extended_robust_list_head));
+ if (len == sizeof(compat_robust_list_head))
+ current->compat_robust_list |= 0b1;
return 0;
}
COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
compat_uptr_t __user *, head_ptr,
compat_size_t __user *, len_ptr)
{
struct compat_robust_list_head __user *head;
+ size_t len;
unsigned long ret;
struct task_struct *p;
if (!futex_cmpxchg_enabled)
return -ENOSYS;
@@ -3848,14 +3898,18 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
ret = -EPERM;
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->compat_robust_list;
+ head = p->compat_robust_list & ~0b11;
+ if (p->compat_robust_list & 0b11 == 0b1)
+ len = sizeof(struct compat_robust_list_head);
+ else
+ len = sizeof(struct compat_extended_robust_list_head);
rcu_read_unlock();
- if (put_user(sizeof(*head), len_ptr))
+ if (put_user(len, len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
err_unlock:
rcu_read_unlock();
--
2.20.1
^ permalink raw reply related
* [RFC PATCH] futex: extend set_robust_list to allow 2 locking ABIs at the same time.
From: Shawn Landden @ 2019-11-04 0:20 UTC (permalink / raw)
To: Thomas Gleixner
Cc: libc-alpha, linux-api, linux-kernel, Arnd Bergmann,
Deepa Dinamani, Oleg Nesterov, Andrew Morton, Catalin Marinas,
Shawn Landden, Keith Packard
The robust futexes ABI was designed to be flexible to changing ABIs in
glibc, however it did not take into consideration that this ABI is
particularly sticky, and suffers from lock-step problems, due to the
fact that the ABI is shared between processes. This introduces a new
size in set_robust_list that takes an additional futex_offset2 value
so that two locking ABIs can be used at the same time.
If this new ABI is used, then bit 1 of the *next pointer of the
user-space robust_list indicates that the futex_offset2 value should
be used in place of the existing futex_offset.
The use case for this is sharing locks between 32-bit and 64-bit
processes, which Linux supports, but glibc does not, and is difficult
to implement with the current Linux support because of mix-matched
ABIs. Keith Packard has complained about this:
https://keithp.com/blogs/Shared_Memory_Fences/
This can also be used to add a new ABI that uses smaller structs,
as the existing ABI on x86_64 is a minimum of 32 bytes, and 20 bytes
would suffice. (12 on 32-bit systems)
If you wants 3 ABIs (perhaps existing 32-bit, existing 64-bit, and
small) then with this patch you are out of luck, as bit 0 is already
taken to indicate PI locks.
---
include/linux/compat.h | 7 ++
include/linux/sched.h | 6 ++
include/uapi/linux/futex.h | 31 +++++++
kernel/futex.c | 182 ++++++++++++++++++++++++-------------
4 files changed, 162 insertions(+), 64 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 16dafd9f4b86..4a9e6a1c2af0 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -379,10 +379,17 @@ struct compat_robust_list_head {
struct compat_robust_list list;
compat_long_t futex_offset;
compat_uptr_t list_op_pending;
};
+struct compat_extended_robust_list_head {
+ struct compat_robust_list_head list_head;
+ compat_long_t futex_offset2;
+ compat_long_t futex_offset3;
+ compat_long_t futex_offset4;
+};
+
#ifdef CONFIG_COMPAT_OLD_SIGACTION
struct compat_old_sigaction {
compat_uptr_t sa_handler;
compat_old_sigset_t sa_mask;
compat_ulong_t sa_flags;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9f51932bd543..894258fd44ac 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1057,10 +1057,16 @@ struct task_struct {
#ifdef CONFIG_X86_CPU_RESCTRL
u32 closid;
u32 rmid;
#endif
#ifdef CONFIG_FUTEX
+ /*
+ * bottom two bits are masked
+ * 0: struct extended_robust_list_head
+ * 1: struct robust_list_head
+ * same for compat_robust_list
+ */
struct robust_list_head __user *robust_list;
#ifdef CONFIG_COMPAT
struct compat_robust_list_head __user *compat_robust_list;
#endif
struct list_head pi_state_list;
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index a89eb0accd5e..30c08e07f26b 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -92,10 +92,41 @@ struct robust_list_head {
* so only truly owned locks will be handled.
*/
struct robust_list __user *list_op_pending;
};
+/*
+ * Extensible per-thread list head:
+ *
+ * As locks are shared between processes, the futex_offset field
+ * has ABI lock-stepping issues, which the original robust_list_head
+ * structure did not anticipate. (And which prevents 32-bit/64-bit
+ * interoperability, as well as shrinking of mutex structures).
+ * This new extensible_robust_list_head allows multiple
+ * concurrent futex_offset values, chosen using the bottom 2 bits of the
+ * robust_list *next pointer, which are now masked in BOTH the old and
+ * new ABI.
+ *
+ * Note: this structure is part of the syscall ABI like
+ * robust_list_head above, and must have a different size than
+ * robust_list_head.
+ *
+ */
+struct extended_robust_list_head {
+ struct robust_list_head list_head;
+
+ /*
+ * These relative offsets are set by user-space. They give the kernel
+ * the relative position of the futex field to examine, based on the
+ * bit 1 *next pointer.
+ * The original version was insufficiently flexible. Locks are held
+ * in shared memory between processes, and a process might want to hold
+ * locks of two ABIs at the same time.
+ */
+ long futex_offset2;
+};
+
/*
* Are there any waiters for this robust futex:
*/
#define FUTEX_WAITERS 0x80000000
diff --git a/kernel/futex.c b/kernel/futex.c
index 6d50728ef2e7..b63af0fcecfc 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3396,17 +3396,20 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
size_t, len)
{
if (!futex_cmpxchg_enabled)
return -ENOSYS;
- /*
- * The kernel knows only one size for now:
- */
- if (unlikely(len != sizeof(*head)))
+
+ if (unlikely(len != sizeof(struct robust_list_head) &&
+ len != sizeof(struct extensible_robust_list_head)))
return -EINVAL;
- current->robust_list = head;
+ current->robust_list = head & 0b11;
+ BUILD_BUG_ON(sizeof(struct robust_list_head) ==
+ sizeof(struct extended_robust_list_head));
+ if (len == sizeof(struct robust_list_head))
+ current->robust_list |= 1;
return 0;
}
/**
@@ -3419,10 +3422,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head __user * __user *, head_ptr,
size_t __user *, len_ptr)
{
struct robust_list_head __user *head;
unsigned long ret;
+ size_t len;
struct task_struct *p;
if (!futex_cmpxchg_enabled)
return -ENOSYS;
@@ -3439,14 +3443,18 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
ret = -EPERM;
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->robust_list;
+ head = p->robust_list & ~0b11;
+ if (p->robust_list & 0b11 == 0b1)
+ len = sizeof(struct robust_list_head);
+ else
+ len = sizeof(struct extended_robust_list_head);
rcu_read_unlock();
- if (put_user(sizeof(*head), len_ptr))
+ if (put_user(len, len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
err_unlock:
rcu_read_unlock();
@@ -3524,23 +3532,26 @@ static int handle_futex_death(u32 __user *uaddr, struct task_struct *curr, int p
return 0;
}
/*
- * Fetch a robust-list pointer. Bit 0 signals PI futexes:
+ * Fetch a robust-list pointer. Bit 0 signals PI futexes. Bit 1 choses which
+ * futex_offset to use:
*/
static inline int fetch_robust_entry(struct robust_list __user **entry,
struct robust_list __user * __user *head,
- unsigned int *pi)
+ unsigned int *pi,
+ *unsigned int *second_abi)
{
unsigned long uentry;
if (get_user(uentry, (unsigned long __user *)head))
return -EFAULT;
- *entry = (void __user *)(uentry & ~1UL);
+ *entry = (void __user *)(uentry & ~0b11UL);
*pi = uentry & 1;
+ *second_abi = uentry & 0b10;
return 0;
}
/*
@@ -3549,69 +3560,84 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
void exit_robust_list(struct task_struct *curr)
{
- struct robust_list_head __user *head = curr->robust_list;
- struct robust_list __user *entry, *next_entry, *pending;
- unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
- unsigned int uninitialized_var(next_pi);
- unsigned long futex_offset;
+ struct robust_list_head __user *head_ptr = curr->robust_list & ~1UL;
+ unsigned int is_extended_list = curr->robust_list & 1 == 0;
+ struct extended_robust_list_head head;
+ struct robust_list __user *entry = &head->list_head.list.next,
+ *next_entry, *pending;
+ unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
+ second_abip;
+ unsigned int uninitialized_var(next_pi),
+ uninitialized_var(next_second_abi);
int rc;
if (!futex_cmpxchg_enabled)
return;
/*
- * Fetch the list head (which was registered earlier, via
- * sys_set_robust_list()):
+ * fetch_robust_entry code is duplicated here to avoid excessive calls
+ * to get_user()
*/
- if (fetch_robust_entry(&entry, &head->list.next, &pi))
- return;
- /*
- * Fetch the relative futex offset:
- */
- if (get_user(futex_offset, &head->futex_offset))
- return;
- /*
- * Fetch any possibly pending lock-add first, and handle it
- * if it exists:
- */
- if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
- return;
+ if (is_extended_list) {
+ if (get_user(head, (struct extended_robust_list_head *)
+ head_ptr))
+ return;
+ } else {
+ if (get_user(head.list_head, head_ptr))
+ return;
+ }
+
+ pi = head.list_head.list.next & 1;
+ second_abi = head.list_head.list.next & 0b10;
+ head.list_head.list.next &= ~0b11UL;
+ pip = head.list_head.list_op_pending & 1;
+ second_abip = head.list_head.list_op_pending & 0b10;
+ head.list_head.list_op_pending &= ~0b11UL;
next_entry = NULL; /* avoid warning with gcc */
- while (entry != &head->list) {
+ while (entry != &head->list_head.list) {
/*
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
- rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi);
+ rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi,
+ &next_second_abi);
/*
* A pending lock might already be on the list, so
* don't process it twice:
*/
- if (entry != pending)
+ if (entry != pending) {
+ long futex_offset = second_abi ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
if (handle_futex_death((void __user *)entry + futex_offset,
curr, pi))
return;
+ }
if (rc)
return;
entry = next_entry;
pi = next_pi;
+ second_abi = next_second_abi;
/*
* Avoid excessively long or circular lists:
*/
if (!--limit)
break;
cond_resched();
}
- if (pending)
+ if (pending) {
+ long futex_offset = second_abip ? head.futex_offset2 :
+ head.list_head.futex_offset;
handle_futex_death((void __user *)pending + futex_offset,
curr, pip);
+ }
}
long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
u32 __user *uaddr2, u32 val2, u32 val3)
{
@@ -3707,21 +3733,25 @@ SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
}
#ifdef CONFIG_COMPAT
/*
- * Fetch a robust-list pointer. Bit 0 signals PI futexes:
+ * Fetch a robust-list pointer. Bit 0 signals PI futexes.
+ * Bit 1 choses which futex_offset to use:
*/
static inline int
-compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
- compat_uptr_t __user *head, unsigned int *pi)
+compat_fetch_robust_entry(compat_uptr_t *uentry,
+ struct robust_list __user **entry,
+ compat_uptr_t __user *head, unsigned int *pi,
+ unsigned int *second_abi)
{
if (get_user(*uentry, head))
return -EFAULT;
- *entry = compat_ptr((*uentry) & ~1);
+ *entry = compat_ptr((*uentry) & ~0b11);
*pi = (unsigned int)(*uentry) & 1;
+ *second_abi = (unsigned int)(*uentry) & 0b10;
return 0;
}
static void __user *futex_uaddr(struct robust_list __user *entry,
@@ -3739,72 +3769,86 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
*
* We silently return on any sign of list-walking problem.
*/
void compat_exit_robust_list(struct task_struct *curr)
{
- struct compat_robust_list_head __user *head = curr->compat_robust_list;
- struct robust_list __user *entry, *next_entry, *pending;
- unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
- unsigned int uninitialized_var(next_pi);
+ struct compat_robust_list_head __user *head = curr->compat_robust_list &
+ ~1UL;
+ unsigned int is_extended_list = curr->compat_robust_list & 1 == 0;
+ struct compat_extended_robust_list_head head;
+ struct robust_list __user *entry = &head->list_head.list.next,
+ *next_entry, *pending;
+ unsigned int limit = ROBUST_LIST_LIMIT, pi, pip, second_abi,
+ second_abip;
+ unsigned int uninitialized_var(next_pi),
+ uninitialized_var(next_second_abi);
compat_uptr_t uentry, next_uentry, upending;
- compat_long_t futex_offset;
int rc;
if (!futex_cmpxchg_enabled)
return;
/*
- * Fetch the list head (which was registered earlier, via
- * sys_set_robust_list()):
- */
- if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
- return;
- /*
- * Fetch the relative futex offset:
- */
- if (get_user(futex_offset, &head->futex_offset))
- return;
- /*
- * Fetch any possibly pending lock-add first, and handle it
- * if it exists:
+ * compat_fetch_robust_entry code is duplicated here to avoid excessive
+ * calls to get_user()
*/
- if (compat_fetch_robust_entry(&upending, &pending,
- &head->list_op_pending, &pip))
- return;
+ if (is_extended_list) {
+ if (get_user(head, (struct compat_extended_robust_list_head *)
+ head_ptr))
+ return;
+ } else {
+ if (get_user(head.list_head, head_ptr))
+ return;
+ }
+
+ pi = head.list_head.list.next & 1;
+ second_abi = head.list_head.list.next & 0b10;
+ head.list_head.list.next &= ~0b11UL;
+ pip = head.list_head.list_op_pending & 1;
+ second_abip = head.list_head.list_op_pending & 0b10;
+ head.list_head.list_op_pending &= ~0b11UL;
next_entry = NULL; /* avoid warning with gcc */
while (entry != (struct robust_list __user *) &head->list) {
/*
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
rc = compat_fetch_robust_entry(&next_uentry, &next_entry,
- (compat_uptr_t __user *)&entry->next, &next_pi);
+ (compat_uptr_t __user *)&entry->next, &next_pi,
+ &next_second_abi);
/*
* A pending lock might already be on the list, so
* dont process it twice:
*/
if (entry != pending) {
+ compat_long_t futex_offset = second_abi ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
void __user *uaddr = futex_uaddr(entry, futex_offset);
if (handle_futex_death(uaddr, curr, pi))
return;
}
if (rc)
return;
uentry = next_uentry;
entry = next_entry;
pi = next_pi;
+ second_abi = next_second_abi;
/*
* Avoid excessively long or circular lists:
*/
if (!--limit)
break;
cond_resched();
}
if (pending) {
+ compat_long_t futex_offset = second_abip ?
+ head.futex_offset2 :
+ head.list_head.futex_offset;
void __user *uaddr = futex_uaddr(pending, futex_offset);
handle_futex_death(uaddr, curr, pip);
}
}
@@ -3814,23 +3858,29 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
compat_size_t, len)
{
if (!futex_cmpxchg_enabled)
return -ENOSYS;
- if (unlikely(len != sizeof(*head)))
+ if (unlikely(len != sizeof(struct compat_robust_list_head) &&
+ len != sizeof(struct compat_extended_robust_list_head)))
return -EINVAL;
- current->compat_robust_list = head;
+ current->compat_robust_list = head & ~0b11;
+ BUILD_BUG_ON(sizeof(compat_robust_list_head) ==
+ sizeof(compat_extended_robust_list_head));
+ if (len == sizeof(compat_robust_list_head))
+ current->compat_robust_list |= 0b1;
return 0;
}
COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
compat_uptr_t __user *, head_ptr,
compat_size_t __user *, len_ptr)
{
struct compat_robust_list_head __user *head;
+ size_t len;
unsigned long ret;
struct task_struct *p;
if (!futex_cmpxchg_enabled)
return -ENOSYS;
@@ -3848,14 +3898,18 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
ret = -EPERM;
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->compat_robust_list;
+ head = p->compat_robust_list & ~0b11;
+ if (p->compat_robust_list & 0b11 == 0b1)
+ len = sizeof(struct compat_robust_list_head);
+ else
+ len = sizeof(struct compat_extended_robust_list_head);
rcu_read_unlock();
- if (put_user(sizeof(*head), len_ptr))
+ if (put_user(len, len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
err_unlock:
rcu_read_unlock();
--
2.20.1
^ permalink raw reply related
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Konstantin Khlebnikov @ 2019-11-03 12:02 UTC (permalink / raw)
To: Andy Lutomirski, Linus Torvalds
Cc: David Howells, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, Nicolas Dichtel, raven, Christian Brauner,
keyrings, USB list, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List, Miklos Szeredi
In-Reply-To: <CALCETrWZjW88OY2mh7v8cUU_6XTSJTkQhAfNbSC17AdhEWwVAA@mail.gmail.com>
On 03/11/2019 02.14, Andy Lutomirski wrote:
> On Sat, Nov 2, 2019 at 4:10 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> On Sat, Nov 2, 2019 at 4:02 PM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> But I don't think anybody actually _did_ any of that. But that's
>>> basically the argument for the three splice operations:
>>> write/vmsplice/splice(). Which one you use depends on the lifetime and
>>> the source of your data. write() is obviously for the copy case (the
>>> source data might not be stable), while splice() is for the "data from
>>> another source", and vmsplace() is "data is from stable data in my
>>> vm".
>>
>> Btw, it's really worth noting that "splice()" and friends are from a
>> more happy-go-lucky time when we were experimenting with new
>> interfaces, and in a day and age when people thought that interfaces
>> like "sendpage()" and zero-copy and playing games with the VM was a
>> great thing to do.
>
> I suppose a nicer interface might be:
>
>
> madvise(buf, len, MADV_STABILIZE);
>
> (MADV_STABILIZE is an imaginary operation that write protects the
> memory a la fork() but without the copying part.)
>
> vmsplice_safer(fd, ...);
>
> Where vmsplice_safer() is like vmsplice, except that it only works on
> write-protected pages. If you vmsplice_safer() some memory and then
> write to the memory, the pipe keeps the old copy.
>
> But this can all be done with memfd and splice, too, I think.
Looks monstrous. This will kill all fun and profit. =)
I think vmsplice should at least deprecate and ignore SPLICE_F_GIFT.
It almost never works - if page still mapped then page_count in
generic_pipe_buf_steal() will be at least 2 (pte and pipe gup).
But if user munmap vma between splicing and consuming (and page not
stuck in lazy tlb and per-cpu vectors) then page from anon lru
could be spliced into file. Ouch.
And looks like fuse device still accepts SPLICE_F_MOVE.
>
>
>>
>> It turns out that VM games are almost always more expensive than just
>> copying the data in the first place, but hey, people didn't know that,
>> and zero-copy was seen a big deal.
>>
>> The reality is that almost nobody uses splice and vmsplice at all, and
>> they have been a much bigger headache than they are worth. If I could
>> go back in time and not do them, I would. But there have been a few
>> very special uses that seem to actually like the interfaces.
>>
>> But it's entirely possible that we should kill vmsplice() (likely by
>> just implementing the semantics as "write()") because it's not common
>> enough to have the complexity.
>
> I think this is the right choice.
>
> FWIW, the openssl vmsplice() call looks dubious, but I suspect it's
> okay because it's vmsplicing to a netlink socket, and the kernel code
> on the other end won't read the data after it returns a response.
>
> --Andy
>
^ permalink raw reply
* Re: [RFC PATCH 04/10] pipe: Use head and tail pointers for the ring, not cursor and length [ver #2]
From: Matthew Wilcox @ 2019-11-03 11:17 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <24075.1572533871@warthog.procyon.org.uk>
On Thu, Oct 31, 2019 at 02:57:51PM +0000, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > It's shorter and more obvious to just write
> >
> > pipe->head = head;
> >
> > than it is to write
> >
> > pipe_commit_write(pipe, head);
>
> But easier to find the latter. But whatever.
May I suggest that you use a name that's easier to grep for?
$ git grep -cw p_tail
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c:9
$ git grep -cw p_head
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c:3
^ permalink raw reply
* Re: [RFC PATCH 07/10] pipe: Conditionalise wakeup in pipe_read() [ver #2]
From: Konstantin Khlebnikov @ 2019-11-03 11:04 UTC (permalink / raw)
To: David Howells
Cc: torvalds, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, linux-security-module, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <3165.1572539884@warthog.procyon.org.uk>
On 31/10/2019 19.38, David Howells wrote:
> Okay, attached is a change that might give you what you want. I tried my
> pipe-bench program (see cover note) with perf. The output of the program with
> the patch applied was:
>
> - pipe 305127298 36262221772 302185181 7887690
>
> The output of perf with the patch applied:
>
> 239,943.92 msec task-clock # 1.997 CPUs utilized
> 17,728 context-switches # 73.884 M/sec
> 124 cpu-migrations # 0.517 M/sec
> 9,330 page-faults # 38.884 M/sec
> 885,107,207,365 cycles # 3688822.793 GHz
> 1,386,873,499,490 instructions # 1.57 insn per cycle
> 311,037,372,339 branches # 1296296921.931 M/sec
> 33,467,827 branch-misses # 0.01% of all branches
>
> And without:
>
> 239,891.87 msec task-clock # 1.997 CPUs utilized
> 22,187 context-switches # 92.488 M/sec
> 133 cpu-migrations # 0.554 M/sec
> 9,334 page-faults # 38.909 M/sec
> 884,906,976,128 cycles # 3688787.725 GHz
> 1,391,986,932,265 instructions # 1.57 insn per cycle
> 311,394,686,857 branches # 1298067400.849 M/sec
> 30,242,823 branch-misses # 0.01% of all branches
>
> So it did make something like a 20% reduction in context switches.
Ok. Looks promising. Depending on workload reduction might be much bigger.
I suppose buffer resize (grow) makes wakeup unconditionally. Should be ok.
>
> David
> ---
> diff --git a/fs/pipe.c b/fs/pipe.c
> index e3d5f7a39123..5167921edd73 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -276,7 +276,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> size_t total_len = iov_iter_count(to);
> struct file *filp = iocb->ki_filp;
> struct pipe_inode_info *pipe = filp->private_data;
> - int do_wakeup;
> + int do_wakeup, wake;
> ssize_t ret;
>
> /* Null read succeeds. */
> @@ -329,11 +329,12 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
> tail++;
> pipe->tail = tail;
> do_wakeup = 1;
> - if (head - (tail - 1) == pipe->max_usage)
> + wake = head - (tail - 1) == pipe->max_usage / 2;
> + if (wake)
> wake_up_interruptible_sync_poll_locked(
> &pipe->wait, EPOLLOUT | EPOLLWRNORM);
> spin_unlock_irq(&pipe->wait.lock);
> - if (head - (tail - 1) == pipe->max_usage)
> + if (wake)
> kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
> }
> total_len -= chars;
>
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Andy Lutomirski @ 2019-11-02 23:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, USB list, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgPQutQ8d8kUCvAFi+hfNWgaNLiZPkbg-GXY2DCtD-Z5Q@mail.gmail.com>
On Sat, Nov 2, 2019 at 4:10 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Nov 2, 2019 at 4:02 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > But I don't think anybody actually _did_ any of that. But that's
> > basically the argument for the three splice operations:
> > write/vmsplice/splice(). Which one you use depends on the lifetime and
> > the source of your data. write() is obviously for the copy case (the
> > source data might not be stable), while splice() is for the "data from
> > another source", and vmsplace() is "data is from stable data in my
> > vm".
>
> Btw, it's really worth noting that "splice()" and friends are from a
> more happy-go-lucky time when we were experimenting with new
> interfaces, and in a day and age when people thought that interfaces
> like "sendpage()" and zero-copy and playing games with the VM was a
> great thing to do.
I suppose a nicer interface might be:
madvise(buf, len, MADV_STABILIZE);
(MADV_STABILIZE is an imaginary operation that write protects the
memory a la fork() but without the copying part.)
vmsplice_safer(fd, ...);
Where vmsplice_safer() is like vmsplice, except that it only works on
write-protected pages. If you vmsplice_safer() some memory and then
write to the memory, the pipe keeps the old copy.
But this can all be done with memfd and splice, too, I think.
>
> It turns out that VM games are almost always more expensive than just
> copying the data in the first place, but hey, people didn't know that,
> and zero-copy was seen a big deal.
>
> The reality is that almost nobody uses splice and vmsplice at all, and
> they have been a much bigger headache than they are worth. If I could
> go back in time and not do them, I would. But there have been a few
> very special uses that seem to actually like the interfaces.
>
> But it's entirely possible that we should kill vmsplice() (likely by
> just implementing the semantics as "write()") because it's not common
> enough to have the complexity.
I think this is the right choice.
FWIW, the openssl vmsplice() call looks dubious, but I suspect it's
okay because it's vmsplicing to a netlink socket, and the kernel code
on the other end won't read the data after it returns a response.
--Andy
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Linus Torvalds @ 2019-11-02 23:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgzRU9RjkZG0L9_yrnFN69REkrSokTQOGZMUkvdispvuQ@mail.gmail.com>
On Sat, Nov 2, 2019 at 4:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But I don't think anybody actually _did_ any of that. But that's
> basically the argument for the three splice operations:
> write/vmsplice/splice(). Which one you use depends on the lifetime and
> the source of your data. write() is obviously for the copy case (the
> source data might not be stable), while splice() is for the "data from
> another source", and vmsplace() is "data is from stable data in my
> vm".
Btw, it's really worth noting that "splice()" and friends are from a
more happy-go-lucky time when we were experimenting with new
interfaces, and in a day and age when people thought that interfaces
like "sendpage()" and zero-copy and playing games with the VM was a
great thing to do.
It turns out that VM games are almost always more expensive than just
copying the data in the first place, but hey, people didn't know that,
and zero-copy was seen a big deal.
The reality is that almost nobody uses splice and vmsplice at all, and
they have been a much bigger headache than they are worth. If I could
go back in time and not do them, I would. But there have been a few
very special uses that seem to actually like the interfaces.
But it's entirely possible that we should kill vmsplice() (likely by
just implementing the semantics as "write()") because it's not common
enough to have the complexity.
Linus
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Linus Torvalds @ 2019-11-02 23:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <E590C3AF-1D09-4927-B83F-DD0A6A148B6D@amacapital.net>
On Sat, Nov 2, 2019 at 3:30 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> So you allocate memory, vmsplice, and munmap() without reusing it?
You can re-use it as much as you want. Just don't write to it.
So the traditional argument for this was "I do a caching http server".
If you don't ever load the data into user space at all and just push
file data out, you just use splice() from the file to the target. But
if you generate some of the data in memory, and you cache it, you use
vmsplice().
And then it really is very easy to set up: make sure you generate your
caches with a new clean private mmap, and you can throw them out with
munmap (or just over-mmap it with the new cache, of course).
If you don't cache it, then there's no advantage to vmsplice() - just
write() it and forget about it. The whole (and only) point of
vmsplice() is when you want to zero-copy the data, and that's
generally likely only an advantage if you can do it multiple times.
But I don't think anybody actually _did_ any of that. But that's
basically the argument for the three splice operations:
write/vmsplice/splice(). Which one you use depends on the lifetime and
the source of your data. write() is obviously for the copy case (the
source data might not be stable), while splice() is for the "data from
another source", and vmsplace() is "data is from stable data in my
vm".
There's the reverse op, of course, but we never implemented that:
mmap() on the pipe could do the reverse of a vmsplice() (moving from
the pipe to the vm), but it would only work if everything was
page-aligned, which it effectively never is. It's basically a
benchmark-only operation.
And the existence of vmsplice() is because we actually had code to
play games with making write() do a zero-copy but mark the source as
being COW. It was _wonderful_ for benchmarks, and was completely
useless for real world case because in the real world you always took
the COW fault. So vmsplice() is basically a "hey, I know what I'm
doing, and you can just take the page as-is because the source is
stable".
Linus
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Andy Lutomirski @ 2019-11-02 22:30 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wj1BLz6s9cG9Ptk4ULxrTy=MkF7ZH=HF67d7M5HL1fd_A@mail.gmail.com>
> On Nov 2, 2019, at 3:04 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> On Sat, Nov 2, 2019 at 1:31 PM Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Add in the fact that it’s not obvious that vmsplice *can* be used correctly, and I’m wondering if we should just remove it or make it just do write() under the hood.
>
> Sure it can. Just don't modify the data you vmsplice. It's really that simple.
So you allocate memory, vmsplice, and munmap() without reusing it? Just plain free() won’t be good enough. I suspect the TLB overhead will make this a loss in most workloads?
Or maybe you vmsplice from a read-only mapping of a file that you know no one modifies? This could be useful, but you can just splice() from the file directly.
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Linus Torvalds @ 2019-11-02 22:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wj1BLz6s9cG9Ptk4ULxrTy=MkF7ZH=HF67d7M5HL1fd_A@mail.gmail.com>
On Sat, Nov 2, 2019 at 3:03 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Nov 2, 2019 at 1:31 PM Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Add in the fact that it’s not obvious that vmsplice *can* be used correctly, and I’m wondering if we should just remove it or make it just do write() under the hood.
>
> Sure it can. Just don't modify the data you vmsplice. It's really that simple.
>
> That said, if we don't have any actual users, then we should look at
> removing it (maybe turning it into "write()" as you say). Not because
> it's hard to use, but simply because it probably doesn't have that
> many uses.
Looking at debian code search, there are _some_ uses (including
openssl and fuse):
https://codesearch.debian.net/search?q=%3D+vmsplice%28&literal=1
but I didn't check any more closely what they do.
Linus
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Linus Torvalds @ 2019-11-02 22:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CC3328CC-2F05-461E-AAC3-8DEBAB1BA162@amacapital.net>
On Sat, Nov 2, 2019 at 1:31 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> Add in the fact that it’s not obvious that vmsplice *can* be used correctly, and I’m wondering if we should just remove it or make it just do write() under the hood.
Sure it can. Just don't modify the data you vmsplice. It's really that simple.
That said, if we don't have any actual users, then we should look at
removing it (maybe turning it into "write()" as you say). Not because
it's hard to use, but simply because it probably doesn't have that
many uses.
Linus
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Andy Lutomirski @ 2019-11-02 20:31 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <25886.1572723272@warthog.procyon.org.uk>
> On Nov 2, 2019, at 12:34 PM, David Howells <dhowells@redhat.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>>>> Similar synchronization is required for reusing memory after vmsplice()?
>>>> I don't see other way how sender could safely change these pages.
>
> Actually, it's probably worse than that. If the output of the pipe gets teed
> or spliced somewhere else, you still don't know when the vmspliced pages are
> finished with.
>
>
I sometimes wonder whether vmsplice should be disallowed or severely restricted. Even ignoring these usability issues, it makes me very uncomfortable that you can have some data queue up on a pipe, tee() it, and get *different* data in the original pipe and the teed copy because the sender used vmsplice and is messing with you.
Add in the fact that it’s not obvious that vmsplice *can* be used correctly, and I’m wondering if we should just remove it or make it just do write() under the hood.
I suppose the kernel could guarantee that it stops referring to the vmsplice source pages as soon as anything sees *or* tees the data. This way it would be, at least in principle, possible to say “hey, the pipe has consumed the first n vmspliced bytes, so I can reuse that memory”.
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: David Howells @ 2019-11-02 19:34 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Konstantin Khlebnikov, Rasmus Villemoes,
Greg Kroah-Hartman, Peter Zijlstra, Nicolas Dichtel, raven,
Christian Brauner, keyrings, linux-usb, linux-block, LSM List,
linux-fsdevel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wg_X_7JSYT-a3qHrzvuWGMyffDWtQ4n7adBp_fe5w0BsA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > Similar synchronization is required for reusing memory after vmsplice()?
> > > I don't see other way how sender could safely change these pages.
Actually, it's probably worse than that. If the output of the pipe gets teed
or spliced somewhere else, you still don't know when the vmspliced pages are
finished with.
David
^ permalink raw reply
* Re: [RFC PATCH 11/10] pipe: Add fsync() support [ver #2]
From: Linus Torvalds @ 2019-11-02 18:53 UTC (permalink / raw)
To: David Howells
Cc: Konstantin Khlebnikov, Rasmus Villemoes, Greg Kroah-Hartman,
Peter Zijlstra, Nicolas Dichtel, raven, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <17311.1572534953@warthog.procyon.org.uk>
On Thu, Oct 31, 2019 at 8:16 AM David Howells <dhowells@redhat.com> wrote:
>
> Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote:
>
> > Similar synchronization is required for reusing memory after vmsplice()?
> > I don't see other way how sender could safely change these pages.
>
> Sounds like a point - if you have multiple parallel contributors to the pipe
> via vmsplice(), then FIONREAD is of no use. To use use FIONREAD, you have to
> let the pipe become empty before you can be sure.
Well, the rules for vmsplice is simply to not change the source pages.
It's zero-copy, after all.
If you want to change the source pages, you need to just use write() instead.
That said, even then the right model isn't fsync(). If you really want
to have something like "notify me when this buffer has been used", it
should be some kind of sequence count thing, not a "wait for empty".
Which might be useful in theory, but would be something quite
different (and honestly, I wouldn't expect it to find all that
widespread use)
Linus
^ permalink raw reply
* Re: tracefs splats in lockdown=confidentiality mode
From: Steven Rostedt @ 2019-11-01 22:15 UTC (permalink / raw)
To: dann frazier
Cc: linux-kernel, linux-security-module, Seth Forshee,
Matthew Garrett, James Morris, Linux API, Ben Hutchings, Al Viro,
Linus Torvalds
In-Reply-To: <20191101210803.GA9841@xps13.dannf>
On Fri, 1 Nov 2019 15:08:03 -0600
dann frazier <dann.frazier@canonical.com> wrote:
> hey,
> fyi, I'm seeing a bunch of errors from tracefs when booting 5.4-rc5 in
> lockdown=confidentiality mode:
>
> [ 1.763630] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
> [ 1.772332] Could not create tracefs 'available_events' entry
> [ 1.778633] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
> [ 1.787095] Could not create tracefs 'set_event' entry
> [ 1.792412] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
> (...)
> [ 2.899481] Could not create tracefs 'set_graph_notrace' entry
> [ 2.905671] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
> [ 2.913934] ------------[ cut here ]------------
> [ 2.918435] Could not register function stat for cpu 0
> [ 2.923717] WARNING: CPU: 1 PID: 1 at kernel/trace/ftrace.c:987 ftrace_init_tracefs_toplevel+0x168/0x1bc
> [ 2.933939] Modules linked in:
> [ 2.937290] CPU: 1 PID: 1 Comm:
Looks to me that it's working as designed ;-)
I'm guessing we could quiet these warnings for boot up though. :-/
But there should be at least one message that states that the tracefs
files are not being created due to lockdown.
-- Steve
^ permalink raw reply
* Re: [RFC PATCH 00/11] pipe: Notification queue preparation [ver #3]
From: Linus Torvalds @ 2019-11-01 22:12 UTC (permalink / raw)
To: David Howells
Cc: Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
Nicolas Dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, LSM List, linux-fsdevel, Linux API,
Linux Kernel Mailing List
In-Reply-To: <13964.1572645926@warthog.procyon.org.uk>
On Fri, Nov 1, 2019 at 3:05 PM David Howells <dhowells@redhat.com> wrote:
>
> Changing those to non-sync:
Your benchmark seems very insensitive to just about any changes.
I suspect it is because you only test throughput. Latency is what the
pipe wakeup has been optimized for, and which tends to be much more
sensitive to other changes too (eg locking).
That said, I'm not convinced a latency test would show much either.
Linus
^ permalink raw reply
* Re: [RFC PATCH 00/11] pipe: Notification queue preparation [ver #3]
From: David Howells @ 2019-11-01 22:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
Nicolas Dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, LSM List, linux-fsdevel, Linux API,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wjqx4j2vqg-tAwthNP1gcAcj1x4B7sq6Npbi8QJTUMd-A@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Side note: we have a couple of cases where I don't think we should use
> the "sync" version at all.
>
> Both pipe_read() and pipe_write() have that
>
> if (do_wakeup) {
> wake_up_interruptible_sync_poll(&pipe->wait, ...
>
> code at the end, outside the loop. But those two wake-ups aren't
> actually synchronous.
Changing those to non-sync:
BENCHMARK BEST TOTAL BYTES AVG BYTES STDDEV
=============== =============== =============== =============== ===============
pipe 305816126 36255936983 302132808 8880788
splice 282402106 27102249370 225852078 210033443
vmsplice 440022611 48896995196 407474959 59906438
Changing the others in pipe_read() and pipe_write() too:
pipe 305609682 36285967942 302383066 7415744
splice 282475690 27891475073 232428958 201687522
vmsplice 451458280 51949421503 432911845 34925242
The cumulative patch is attached below. I'm not sure how well this should
make a difference with my benchmark programs since each thread can run on its
own CPU.
David
---
diff --git a/fs/pipe.c b/fs/pipe.c
index 9cd5cbef9552..c5e3765465f0 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -332,7 +332,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
do_wakeup = 1;
wake = head - (tail - 1) == pipe->max_usage / 2;
if (wake)
- wake_up_interruptible_sync_poll_locked(
+ wake_up_locked_poll(
&pipe->wait, EPOLLOUT | EPOLLWRNORM);
spin_unlock_irq(&pipe->wait.lock);
if (wake)
@@ -371,7 +371,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
/* Signal writers asynchronously that there is more room. */
if (do_wakeup) {
- wake_up_interruptible_sync_poll(&pipe->wait, EPOLLOUT | EPOLLWRNORM);
+ wake_up_interruptible_poll(&pipe->wait, EPOLLOUT | EPOLLWRNORM);
kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
}
if (ret > 0)
@@ -477,7 +477,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
* syscall merging.
* FIXME! Is this really true?
*/
- wake_up_interruptible_sync_poll_locked(
+ wake_up_locked_poll(
&pipe->wait, EPOLLIN | EPOLLRDNORM);
spin_unlock_irq(&pipe->wait.lock);
@@ -531,7 +531,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
out:
__pipe_unlock(pipe);
if (do_wakeup) {
- wake_up_interruptible_sync_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM);
+ wake_up_interruptible_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM);
kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
}
if (ret > 0 && sb_start_write_trylock(file_inode(filp)->i_sb)) {
^ permalink raw reply related
* tracefs splats in lockdown=confidentiality mode
From: dann frazier @ 2019-11-01 21:08 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, linux-security-module, Seth Forshee,
Matthew Garrett, James Morris, Linux API, Ben Hutchings, Al Viro,
Linus Torvalds
hey,
fyi, I'm seeing a bunch of errors from tracefs when booting 5.4-rc5 in
lockdown=confidentiality mode:
[ 1.763630] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 1.772332] Could not create tracefs 'available_events' entry
[ 1.778633] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 1.787095] Could not create tracefs 'set_event' entry
[ 1.792412] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
(...)
[ 2.899481] Could not create tracefs 'set_graph_notrace' entry
[ 2.905671] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 2.913934] ------------[ cut here ]------------
[ 2.918435] Could not register function stat for cpu 0
[ 2.923717] WARNING: CPU: 1 PID: 1 at kernel/trace/ftrace.c:987 ftrace_init_tracefs_toplevel+0x168/0x1bc
[ 2.933939] Modules linked in:
[ 2.937290] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.4.0-050400rc5-generic #201910271430
[ 2.946528] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[ 2.954088] pstate: 60400005 (nZCv daif +PAN -UAO)
[ 2.959359] pc : ftrace_init_tracefs_toplevel+0x168/0x1bc
[ 2.965262] lr : ftrace_init_tracefs_toplevel+0x168/0x1bc
[ 2.971192] sp : ffff80001002bd40
[ 2.974852] x29: ffff80001002bd40 x28: 0000000000000000
[ 2.980680] x27: 0000000000000000 x26: ffff8000119f9358
[ 2.986552] x25: ffff8000119f9580 x24: ffff00007fb77200
[ 2.992359] x23: ffff00007c873e80 x22: ffff80001153d200
[ 2.998201] x21: ffff800010d8ad08 x20: 0000000000000000
[ 3.004007] x19: 0000000000000000 x18: 0000000000000010
[ 3.009851] x17: 0000000000000007 x16: 000000000000000e
[ 3.015674] x15: ffff00007d1213e8 x14: ffffffffffffffff
[ 3.021526] x13: ffff80009002ba47 x12: ffff80001002ba4f
[ 3.027338] x11: ffff800011a1e000 x10: 0000000000000000
[ 3.033148] x9 : ffff800011c13000 x8 : 000000000000015d
[ 3.038984] x7 : 0000000000000017 x6 : ffff800011c129c9
[ 3.044783] x5 : 0000000000000007 x4 : 0000000000000000
[ 3.050617] x3 : 0000000000000000 x2 : 00000000ffffffff
[ 3.056430] x1 : bcf0a68bd924d700 x0 : 0000000000000000
[ 3.062258] Call trace:
[ 3.064951] ftrace_init_tracefs_toplevel+0x168/0x1bc
[ 3.070571] tracer_init_tracefs+0xc0/0x1fc
[ 3.075165] do_one_initcall+0x50/0x220
[ 3.079384] kernel_init_freeable+0x1ec/0x2b0
[ 3.084186] kernel_init+0x18/0x108
[ 3.088032] ret_from_fork+0x10/0x18
[ 3.091983] ---[ end trace 32f7e54339335d2a ]---
[ 3.097149] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.105311] Could not create tracefs 'tracing_thresh' entry
[ 3.110762] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.119199] Could not create tracefs 'README' entry
[ 3.124212] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.132920] Could not create tracefs 'saved_cmdlines' entry
[ 3.139075] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.147592] Could not create tracefs 'saved_cmdlines_size' entry
[ 3.153950] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.162288] Could not create tracefs 'saved_tgids' entry
[ 3.206061] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.214629] Could not create tracefs 'dyn_ftrace_total_info' entry
[ 3.221063] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.229245] Could not create tracefs 'funcgraph-overrun' entry
[ 3.234990] ------------[ cut here ]------------
[ 3.239724] Failed to create trace option: funcgraph-overrun
[ 3.239774] WARNING: CPU: 1 PID: 1 at kernel/trace/trace.c:8106 create_trace_option_files+0x200/0x230
[ 3.255931] Modules linked in:
[ 3.259332] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W 5.4.0-050400rc5-generic #201910271430
[ 3.270051] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[ 3.277621] pstate: 60400005 (nZCv daif +PAN -UAO)
[ 3.282902] pc : create_trace_option_files+0x200/0x230
[ 3.288564] lr : create_trace_option_files+0x200/0x230
[ 3.294247] sp : ffff80001002bd10
[ 3.297914] x29: ffff80001002bd10 x28: ffff800011a34508
[ 3.303769] x27: ffff00007cb12e00 x26: ffff800011bb243d
[ 3.309638] x25: ffff800010d8b750 x24: ffff00007cb12e00
[ 3.315485] x23: 0000000000000000 x22: ffff800011a34508
[ 3.321327] x21: ffff800011a344d0 x20: 0000000000000000
[ 3.327284] x19: ffff800011a320b8 x18: 0000000000000010
[ 3.333121] x17: 0000000000000007 x16: 000000000000000e
[ 3.338980] x15: ffff00007d1213e8 x14: ffffffffffffffff
[ 3.344827] x13: ffff80009002b9a7 x12: ffff80001002b9af
[ 3.350690] x11: ffff800011a1e000 x10: 0000000000000000
[ 3.356524] x9 : 00000000fffffffe x8 : 6870617267636e75
[ 3.362394] x7 : 66203a6e6f697470 x6 : ffff800011c12498
[ 3.368229] x5 : 0000000000000030 x4 : 0000000000000000
[ 3.374082] x3 : 0000000000000000 x2 : 00000000ffffffff
[ 3.379915] x1 : bcf0a68bd924d700 x0 : 0000000000000000
[ 3.385768] Call trace:
[ 3.388478] create_trace_option_files+0x200/0x230
[ 3.393849] __update_tracer_options+0x34/0x48
[ 3.398748] tracer_init_tracefs+0x1e0/0x1fc
[ 3.403454] do_one_initcall+0x50/0x220
[ 3.407692] kernel_init_freeable+0x1ec/0x2b0
[ 3.412494] kernel_init+0x18/0x108
[ 3.416351] ret_from_fork+0x10/0x18
[ 3.420287] ---[ end trace 32f7e54339335d2b ]---
[ 3.425479] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.433724] Could not create tracefs 'funcgraph-cpu' entry
(...)
[ 3.692698] Could not create tracefs 'uprobe_events' entry
[ 3.698724] Lockdown: swapper/0: use of tracefs is restricted; see man kernel_lockdown.7
[ 3.707217] Could not create tracefs 'uprobe_profile' entry
-dann
^ permalink raw reply
* Re: [RFC PATCH 00/11] pipe: Notification queue preparation [ver #3]
From: Linus Torvalds @ 2019-11-01 19:24 UTC (permalink / raw)
To: David Howells
Cc: Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
Nicolas Dichtel, raven, Christian Brauner, keyrings, linux-usb,
linux-block, LSM List, linux-fsdevel, Linux API,
Linux Kernel Mailing List
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
On Fri, Nov 1, 2019 at 10:34 AM David Howells <dhowells@redhat.com> wrote:
> (1) It removes the nr_exclusive argument from __wake_up_sync_key() as this
> is always 1. This prepares for step 2.
>
> (2) Adds wake_up_interruptible_sync_poll_locked() so that poll can be
> woken up from a function that's holding the poll waitqueue spinlock.
Side note: we have a couple of cases where I don't think we should use
the "sync" version at all.
Both pipe_read() and pipe_write() have that
if (do_wakeup) {
wake_up_interruptible_sync_poll(&pipe->wait, ...
code at the end, outside the loop. But those two wake-ups aren't
actually synchronous.
A sync wake is supposedly something where the waker is just about to
go to sleep, telling the scheduler that "don't bother trying to pick
another cpu, this process is going to sleep and you can stay here".
I'm not sure how much this matters, but it does strike me that it's
wrong. We're not going to sleep at all in that case - this is not the
"I filled the whole buffer, so I'm going to sleep" case (or the "I've
read all the data, I'm waiting for more".
It's entirely possible that we always wake pipe wakeups to be sync
just because it's a common pattern (and a common benchmark), but this
series made me look at it again. Particularly since David has
benchmarks that don't seem to show a lot of fluctuation with his
changes - I wonder how much the sync logic buys us (or hurts us)?
Linus
^ permalink raw reply
* [RFC PATCH 11/11] pipe: Increase the writer-wakeup threshold to reduce context-switch count [ver #3]
From: David Howells @ 2019-11-01 17:35 UTC (permalink / raw)
To: torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
Increase the threshold at which the reader sends a wake event to the
writers in the queue such that the queue must be half empty before the wake
is issued rather than the wake being issued when just a single slot
available.
This reduces the number of context switches in the tests significantly,
without altering the amount of work achieved. With my pipe-bench program,
there's a 20% reduction versus an unpatched kernel.
Suggested-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/pipe.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index aba2455caabe..9cd5cbef9552 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -324,16 +324,18 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
}
if (!buf->len) {
+ bool wake;
pipe_buf_release(pipe, buf);
spin_lock_irq(&pipe->wait.lock);
tail++;
pipe->tail = tail;
do_wakeup = 1;
- if (head - (tail - 1) == pipe->max_usage)
+ wake = head - (tail - 1) == pipe->max_usage / 2;
+ if (wake)
wake_up_interruptible_sync_poll_locked(
&pipe->wait, EPOLLOUT | EPOLLWRNORM);
spin_unlock_irq(&pipe->wait.lock);
- if (head - (tail - 1) == pipe->max_usage)
+ if (wake)
kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
}
total_len -= chars;
^ permalink raw reply related
* [RFC PATCH 10/11] pipe: Check for ring full inside of the spinlock in pipe_write() [ver #3]
From: David Howells @ 2019-11-01 17:35 UTC (permalink / raw)
To: torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
Make pipe_write() check to see if the ring has become full between it
taking the pipe mutex, checking the ring status and then taking the
spinlock.
This can happen if a notification is written into the pipe as that happens
without the pipe mutex.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/pipe.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/pipe.c b/fs/pipe.c
index d7b8d3f22987..aba2455caabe 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -463,6 +463,11 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
spin_lock_irq(&pipe->wait.lock);
head = pipe->head;
+ if (pipe_full(head, pipe->tail, max_usage)) {
+ spin_unlock_irq(&pipe->wait.lock);
+ continue;
+ }
+
pipe->head = head + 1;
/* Always wake up, even if the copy fails. Otherwise
^ permalink raw reply related
* [RFC PATCH 09/11] pipe: Remove redundant wakeup from pipe_write() [ver #3]
From: David Howells @ 2019-11-01 17:35 UTC (permalink / raw)
To: torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
Remove a redundant wakeup from pipe_write().
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/pipe.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index ce77ac0d8901..d7b8d3f22987 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -517,11 +517,6 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
ret = -ERESTARTSYS;
break;
}
- if (do_wakeup) {
- wake_up_interruptible_sync_poll(&pipe->wait, EPOLLIN | EPOLLRDNORM);
- kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
- do_wakeup = 0;
- }
pipe->waiting_writers++;
pipe_wait(pipe);
pipe->waiting_writers--;
^ permalink raw reply related
* [RFC PATCH 08/11] pipe: Rearrange sequence in pipe_write() to preallocate slot [ver #3]
From: David Howells @ 2019-11-01 17:35 UTC (permalink / raw)
To: torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
Rearrange the sequence in pipe_write() so that the allocation of the new
buffer, the allocation of a ring slot and the attachment to the ring is
done under the pipe wait spinlock and then the lock is dropped and the
buffer can be filled.
The data copy needs to be done with the spinlock unheld and irqs enabled,
so the lock needs to be dropped first. However, the reader can't progress
as we're holding pipe->mutex.
We also need to drop the lock as that would impact others looking at the
pipe waitqueue, such as poll(), the consumer and a future kernel message
writer.
We just abandon the preallocated slot if we get a copy error. Future
writes may continue it and a future read will eventually recycle it.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/pipe.c | 51 +++++++++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 18 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index c16950e36ded..ce77ac0d8901 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -387,7 +387,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
{
struct file *filp = iocb->ki_filp;
struct pipe_inode_info *pipe = filp->private_data;
- unsigned int head, tail, max_usage, mask;
+ unsigned int head, max_usage, mask;
ssize_t ret = 0;
int do_wakeup = 0;
size_t total_len = iov_iter_count(from);
@@ -405,14 +405,13 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
goto out;
}
- tail = pipe->tail;
head = pipe->head;
max_usage = pipe->max_usage;
mask = pipe->ring_size - 1;
/* We try to merge small writes */
chars = total_len & (PAGE_SIZE-1); /* size of the last buffer */
- if (!pipe_empty(head, tail) && chars != 0) {
+ if (!pipe_empty(head, pipe->tail) && chars != 0) {
struct pipe_buffer *buf = &pipe->bufs[(head - 1) & mask];
int offset = buf->offset + buf->len;
@@ -441,8 +440,8 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
break;
}
- tail = pipe->tail;
- if (!pipe_full(head, tail, max_usage)) {
+ head = pipe->head;
+ if (!pipe_full(head, pipe->tail, max_usage)) {
struct pipe_buffer *buf = &pipe->bufs[head & mask];
struct page *page = pipe->tmp_page;
int copied;
@@ -455,40 +454,56 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
}
pipe->tmp_page = page;
}
+
+ /* Allocate a slot in the ring in advance and attach an
+ * empty buffer. If we fault or otherwise fail to use
+ * it, either the reader will consume it or it'll still
+ * be there for the next write.
+ */
+ spin_lock_irq(&pipe->wait.lock);
+
+ head = pipe->head;
+ pipe->head = head + 1;
+
/* Always wake up, even if the copy fails. Otherwise
* we lock up (O_NONBLOCK-)readers that sleep due to
* syscall merging.
* FIXME! Is this really true?
*/
- do_wakeup = 1;
- copied = copy_page_from_iter(page, 0, PAGE_SIZE, from);
- if (unlikely(copied < PAGE_SIZE && iov_iter_count(from))) {
- if (!ret)
- ret = -EFAULT;
- break;
- }
- ret += copied;
+ wake_up_interruptible_sync_poll_locked(
+ &pipe->wait, EPOLLIN | EPOLLRDNORM);
+
+ spin_unlock_irq(&pipe->wait.lock);
+ kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
/* Insert it into the buffer array */
+ buf = &pipe->bufs[head & mask];
buf->page = page;
buf->ops = &anon_pipe_buf_ops;
buf->offset = 0;
- buf->len = copied;
+ buf->len = 0;
buf->flags = 0;
if (is_packetized(filp)) {
buf->ops = &packet_pipe_buf_ops;
buf->flags = PIPE_BUF_FLAG_PACKET;
}
-
- head++;
- pipe->head = head;
pipe->tmp_page = NULL;
+ copied = copy_page_from_iter(page, 0, PAGE_SIZE, from);
+ if (unlikely(copied < PAGE_SIZE && iov_iter_count(from))) {
+ if (!ret)
+ ret = -EFAULT;
+ break;
+ }
+ ret += copied;
+ buf->offset = 0;
+ buf->len = copied;
+
if (!iov_iter_count(from))
break;
}
- if (!pipe_full(head, tail, max_usage))
+ if (!pipe_full(head, pipe->tail, max_usage))
continue;
/* Wait for buffer space to become available. */
^ permalink raw reply related
* [RFC PATCH 07/11] pipe: Conditionalise wakeup in pipe_read() [ver #3]
From: David Howells @ 2019-11-01 17:35 UTC (permalink / raw)
To: torvalds
Cc: dhowells, Rasmus Villemoes, Greg Kroah-Hartman, Peter Zijlstra,
nicolas.dichtel, raven, Christian Brauner
In-Reply-To: <157262963995.13142.5568934007158044624.stgit@warthog.procyon.org.uk>
Only do a wakeup in pipe_read() if we made space in a completely full
buffer. The producer shouldn't be waiting on pipe->wait otherwise.
Signed-off-by: David Howells <dhowells@redhat.com>
---
fs/pipe.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index ea134f69a292..c16950e36ded 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -328,11 +328,13 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
spin_lock_irq(&pipe->wait.lock);
tail++;
pipe->tail = tail;
- do_wakeup = 0;
- wake_up_interruptible_sync_poll_locked(
- &pipe->wait, EPOLLOUT | EPOLLWRNORM);
+ do_wakeup = 1;
+ if (head - (tail - 1) == pipe->max_usage)
+ wake_up_interruptible_sync_poll_locked(
+ &pipe->wait, EPOLLOUT | EPOLLWRNORM);
spin_unlock_irq(&pipe->wait.lock);
- kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
+ if (head - (tail - 1) == pipe->max_usage)
+ kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
}
total_len -= chars;
if (!total_len)
@@ -361,11 +363,6 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
ret = -ERESTARTSYS;
break;
}
- if (do_wakeup) {
- wake_up_interruptible_sync_poll(&pipe->wait, EPOLLOUT | EPOLLWRNORM);
- kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
- do_wakeup = 0;
- }
pipe_wait(pipe);
}
__pipe_unlock(pipe);
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox