* [PATCH v4 4/7] futex: Create set_robust_list2
From: André Almeida @ 2025-05-20 20:48 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, André Almeida
In-Reply-To: <20250520-tonyk-robust_futex-v4-0-1123093e59de@igalia.com>
Create a new robust_list() syscall. The current syscall can't be
expanded to cover the following use case, so a new one is needed. This
new syscall allows users to set multiple robust lists per process and to
have either 32bit or 64bit pointers in the list.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a
normal robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the
naming starts to get confusing, I reckon). If `index == -1`, that means
that user wants to set a new robust_list, and the kernel will append it
in the end of the list, assign a new index and return this index to the
user. If `index >= 0`, that means that user wants to re-set `*head` of
an already existing list (similarly to what happens when you call
set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list,
or if the internal list is full, an error is returned.
Unaligned `head` addresses are refused by the kernel with -EINVAL.
User cannot remove lists.
* Implementation
The old syscall's set/get_robust_list() are converted to use the linked
list as well. When using only the old syscalls user shouldn't any
difference as the internal code will handle the linked list insertion as
usual. When mixing old and new interfaces users should be aware that one
of the elements of the list was created by another syscall and they
should have special care handling this element index.
On exit, the linked list is parsed and all robust lists regardless of
which interface it was used to create them are handled.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/futex.h | 5 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 2 +
include/uapi/linux/futex.h | 24 +++++++++
kernel/futex/core.c | 111 ++++++++++++++++++++++++++++++--------
kernel/futex/futex.h | 5 ++
kernel/futex/syscalls.c | 81 ++++++++++++++++++++++++++--
7 files changed, 204 insertions(+), 29 deletions(-)
diff --git a/include/linux/futex.h b/include/linux/futex.h
index cd7c5d12c846566c56f3f3ea74b95e437a6e8193..7721629926535c775bd7b05b5283a3d0b51262d6 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -75,10 +75,11 @@ enum {
static inline void futex_init_task(struct task_struct *tsk)
{
- tsk->robust_list = NULL;
+ tsk->robust_list_index = -1;
#ifdef CONFIG_COMPAT
- tsk->compat_robust_list = NULL;
+ tsk->compat_robust_list_index = -1;
#endif
+ INIT_LIST_HEAD(&tsk->robust_list2);
INIT_LIST_HEAD(&tsk->pi_state_list);
tsk->pi_state_cache = NULL;
tsk->futex_state = FUTEX_STATE_OK;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 51e5d05a9fcd407dcd53b7b7cb8c59783660a826..a37c55cf0a4d942ec1fbedb8bcd4be5a3ebb20bb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1322,10 +1322,11 @@ struct task_struct {
u32 rmid;
#endif
#ifdef CONFIG_FUTEX
- struct robust_list_head __user *robust_list;
+ int robust_list_index;
#ifdef CONFIG_COMPAT
- struct robust_list_head32 __user *compat_robust_list;
+ int compat_robust_list_index;
#endif
+ struct list_head robust_list2;
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
struct mutex futex_exit_mutex;
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 2892a45023af6d3eb941623d4fed04841ab07e02..ebe68c2c88eb5390dda184ce9268a8d3a606c9e5 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -852,6 +852,8 @@ __SYSCALL(__NR_removexattrat, sys_removexattrat)
#define __NR_open_tree_attr 467
__SYSCALL(__NR_open_tree_attr, sys_open_tree_attr)
+#define __NR_set_robust_list2 467
+
#undef __NR_syscalls
#define __NR_syscalls 468
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index 7e2744ec89336a260e89883e95222eda199eeb7f..cbd321eca03afb6bdcf47e9534761d82f9de7e43 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -153,6 +153,30 @@ struct robust_list_head {
struct robust_list __user *list_op_pending;
};
+#define ROBUST_LISTS_PER_TASK 10
+
+enum robust_list2_type {
+ ROBUST_LIST_32BIT,
+ ROBUST_LIST_64BIT,
+};
+
+#define ROBUST_LIST_TYPE_MASK (ROBUST_LIST_32BIT | ROBUST_LIST_64BIT)
+
+/*
+ * This is an entry of a linked list of robust lists.
+ *
+ * @head: can point to a 64bit list or a 32bit list
+ * @list_type: determine the size of the futex pointers in the list
+ * @index: the index of this entry in the list
+ * @list: linked list element
+ */
+struct robust_list2_entry {
+ void __user *head;
+ enum robust_list2_type list_type;
+ unsigned int index;
+ struct list_head list;
+};
+
/*
* Are there any waiters for this robust futex:
*/
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 8640770aadc611b7341a3abb41bdb740e6394479..49b3bc592948a811f995017027f33ad8f285531f 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -1151,9 +1151,9 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
- 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 next_pi;
@@ -1213,7 +1213,8 @@ static void exit_robust_list64(struct task_struct *curr)
}
}
#else
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
}
@@ -1250,9 +1251,9 @@ fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list32(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr,
+ struct robust_list_head32 __user *head)
{
- struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
@@ -1318,6 +1319,70 @@ static void exit_robust_list32(struct task_struct *curr)
}
}
+long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type)
+{
+ struct list_head *list2 = ¤t->robust_list2;
+ struct robust_list2_entry *prev, *new = NULL;
+
+ if (index == -1) {
+ if (list_empty(list2)) {
+ index = 0;
+ } else {
+ prev = list_last_entry(list2, struct robust_list2_entry, list);
+ index = prev->index + 1;
+ }
+
+ if (index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ new = kmalloc(sizeof(struct robust_list2_entry), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ list_add_tail(&new->list, list2);
+ new->index = index;
+
+ } else if (index >= 0) {
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2))
+ return -ENOENT;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index) {
+ new = curr;
+ break;
+ }
+ }
+
+ if (!new)
+ return -ENOENT;
+ }
+
+ BUG_ON(!new);
+ new->head = head;
+ new->list_type = type;
+
+ return index;
+}
+
+struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task)
+{
+ struct list_head *list2 = &task->robust_list2;
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2) || index == -1)
+ return NULL;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index)
+ return curr->head;
+ }
+
+ return NULL;
+}
+
#ifdef CONFIG_FUTEX_PI
/*
@@ -1411,24 +1476,28 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
-#ifdef CONFIG_64BIT
- if (unlikely(tsk->robust_list)) {
- exit_robust_list64(tsk);
- tsk->robust_list = NULL;
- }
-#else
- if (unlikely(tsk->robust_list)) {
- exit_robust_list32(tsk);
- tsk->robust_list = NULL;
- }
-#endif
+ struct robust_list2_entry *curr, *n;
+ struct list_head *list2 = &tsk->robust_list2;
-#ifdef CONFIG_COMPAT
- if (unlikely(tsk->compat_robust_list)) {
- exit_robust_list32(tsk);
- tsk->compat_robust_list = NULL;
+ /*
+ * Walk through the linked list, parsing robust lists and freeing the
+ * allocated lists
+ */
+ if (unlikely(!list_empty(list2))) {
+ list_for_each_entry_safe(curr, n, list2, list) {
+ if (curr->head != NULL) {
+ if (curr->list_type == ROBUST_LIST_64BIT)
+ exit_robust_list64(tsk, curr->head);
+ else if (curr->list_type == ROBUST_LIST_32BIT)
+ exit_robust_list32(tsk, curr->head);
+ curr->head = NULL;
+ }
+ list_del_init(&curr->list);
+ kfree(curr);
+ }
}
-#endif
+
+ tsk->robust_list_index = -1;
if (unlikely(!list_empty(&tsk->pi_state_list)))
exit_pi_state_list(tsk);
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index fcd1617212eed0e3c2367d2b463a0e019eda6d13..67201e51fa1798a21ff68f60b1e35977b9bd267b 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -467,6 +467,11 @@ extern int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
extern int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
ktime_t *abs_time, u32 bitset);
+extern long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type);
+
+extern struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task);
+
/**
* struct futex_vector - Auxiliary struct for futex_waitv()
* @w: Userspace provided data
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index dba193dfd216cc929c8f4d979aa2bcd99237e2d8..56ee1123cbd8ea26c8d22aa74e5faed2974ec577 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -20,6 +20,18 @@
* the list. There can only be one such pending lock.
*/
+#ifdef CONFIG_64BIT
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_64BIT;
+}
+#else
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_32BIT;
+}
+#endif
+
/**
* sys_set_robust_list() - Set the robust-futex list head of a task
* @head: pointer to the list-head
@@ -28,17 +40,63 @@
SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
size_t, len)
{
+ unsigned int type = robust_list_native_type();
+ int ret;
+
/*
* The kernel knows only one size for now:
*/
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->robust_list = head;
+ ret = do_set_robust_list2(head, current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
return 0;
}
+#define ROBUST_LIST_FLAGS ROBUST_LIST_TYPE_MASK
+
+/*
+ * sys_set_robust_list2()
+ *
+ * When index == -1, create a new list for user. When index >= 0, try to find
+ * the corresponding list and re-set the head there.
+ *
+ * Return values:
+ * >= 0: success, index of the robust list
+ * -EINVAL: invalid flags, invalid index
+ * -ENOENT: requested index no where to be found
+ * -ENOMEM: error allocating new list
+ * -ESRCH: too many allocated lists
+ */
+SYSCALL_DEFINE3(set_robust_list2, struct robust_list_head __user *, head,
+ int, index, unsigned int, flags)
+{
+ unsigned int type;
+
+ type = flags & ROBUST_LIST_TYPE_MASK;
+
+ if (index < -1 || index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ if ((flags & ~ROBUST_LIST_FLAGS) != 0)
+ return -EINVAL;
+
+ if (((uintptr_t) head % sizeof(u32)) != 0)
+ return -EINVAL;
+
+#ifndef CONFIG_64BIT
+ if (type == ROBUST_LIST_64BIT)
+ return -EINVAL;
+#endif
+
+ return do_set_robust_list2(head, index, type);
+}
+
/**
* sys_get_robust_list() - Get the robust-futex list head of a task
* @pid: pid of the process [zero for current task]
@@ -52,6 +110,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -68,9 +127,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->robust_list;
+ index = p->robust_list_index;
rcu_read_unlock();
+ head = get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
@@ -443,10 +504,19 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
struct robust_list_head32 __user *, head,
compat_size_t, len)
{
+ unsigned int type = ROBUST_LIST_32BIT;
+ int ret;
+
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->compat_robust_list = head;
+ ret = do_set_robust_list2((struct robust_list_head __user *) head,
+ current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
+
return 0;
}
@@ -458,6 +528,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -474,9 +545,11 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->compat_robust_list;
+ index = p->compat_robust_list_index;
rcu_read_unlock();
+ head = (struct robust_list_head32 __user *) get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
--
2.49.0
^ permalink raw reply related
* [PATCH v4 3/7] futex: Use explicit sizes for compat_exit_robust_list
From: André Almeida @ 2025-05-20 20:48 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, André Almeida
In-Reply-To: <20250520-tonyk-robust_futex-v4-0-1123093e59de@igalia.com>
There are two functions for handling robust lists during the task
exit: exit_robust_list() and compat_exit_robust_list(). The first one
handles either 64bit or 32bit lists, depending if it's a 64bit or 32bit
kernel. The compat_exit_robust_list() only exists in 64bit kernels that
supports 32bit syscalls, and handles 32bit lists.
For the new syscall set_robust_list2(), 64bit kernels need to be able to
handle 32bit lists despite having or not support for 32bit syscalls, so
make compat_exit_robust_list() exist regardless of compat_ config.
Also, use explicitly sizing, otherwise in a 32bit kernel both
exit_robust_list() and compat_exit_robust_list() would be the exactly
same function, with none of them dealing with 64bit robust lists.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/compat.h | 12 +-----------
include/linux/futex.h | 11 +++++++++++
include/linux/sched.h | 2 +-
kernel/futex/core.c | 44 ++++++++++++++++++++++++++++----------------
kernel/futex/syscalls.c | 4 ++--
5 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 56cebaff0c910fda853a0e2b3d6d0517e55f8b38..968a9135ff486cf9c8be2a18b80cd4c46e890236 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -385,16 +385,6 @@ struct compat_ifconf {
compat_caddr_t ifcbuf;
};
-struct compat_robust_list {
- compat_uptr_t next;
-};
-
-struct compat_robust_list_head {
- struct compat_robust_list list;
- compat_long_t futex_offset;
- compat_uptr_t list_op_pending;
-};
-
#ifdef CONFIG_COMPAT_OLD_SIGACTION
struct compat_old_sigaction {
compat_uptr_t sa_handler;
@@ -672,7 +662,7 @@ asmlinkage long compat_sys_waitid(int, compat_pid_t,
struct compat_siginfo __user *, int,
struct compat_rusage __user *);
asmlinkage long
-compat_sys_set_robust_list(struct compat_robust_list_head __user *head,
+compat_sys_set_robust_list(struct robust_list_head32 __user *head,
compat_size_t len);
asmlinkage long
compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
diff --git a/include/linux/futex.h b/include/linux/futex.h
index 168ffd5996b4808491c05bdc7c8d0aeca1d37ee5..cd7c5d12c846566c56f3f3ea74b95e437a6e8193 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -56,6 +56,17 @@ union futex_key {
#define FUTEX_KEY_INIT (union futex_key) { .both = { .ptr = 0ULL } }
#ifdef CONFIG_FUTEX
+
+struct robust_list32 {
+ u32 next;
+};
+
+struct robust_list_head32 {
+ struct robust_list32 list;
+ s32 futex_offset;
+ u32 list_op_pending;
+};
+
enum {
FUTEX_STATE_OK,
FUTEX_STATE_EXITING,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 45e5953b8f326c2ff5e19de469d6cba27cc4c17d..51e5d05a9fcd407dcd53b7b7cb8c59783660a826 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1324,7 +1324,7 @@ struct task_struct {
#ifdef CONFIG_FUTEX
struct robust_list_head __user *robust_list;
#ifdef CONFIG_COMPAT
- struct compat_robust_list_head __user *compat_robust_list;
+ struct robust_list_head32 __user *compat_robust_list;
#endif
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 19a2c65f3d373c0b60c864a6fe0604787221d342..8640770aadc611b7341a3abb41bdb740e6394479 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -1144,13 +1144,14 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
return 0;
}
+#ifdef CONFIG_64BIT
/*
* Walk curr->robust_list (very carefully, it's a userspace list!)
* and mark any locks found there dead, and notify any waiters.
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr)
{
struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
@@ -1211,8 +1212,13 @@ static void exit_robust_list(struct task_struct *curr)
curr, pip, HANDLE_DEATH_PENDING);
}
}
+#else
+static void exit_robust_list64(struct task_struct *curr)
+{
+ pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
+}
+#endif
-#ifdef CONFIG_COMPAT
static void __user *futex_uaddr(struct robust_list __user *entry,
compat_long_t futex_offset)
{
@@ -1226,13 +1232,13 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
* Fetch a robust-list pointer. Bit 0 signals PI futexes:
*/
static inline int
-compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
- compat_uptr_t __user *head, unsigned int *pi)
+fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
+ u32 __user *head, unsigned int *pi)
{
if (get_user(*uentry, head))
return -EFAULT;
- *entry = compat_ptr((*uentry) & ~1);
+ *entry = (void __user *)(unsigned long)((*uentry) & ~1);
*pi = (unsigned int)(*uentry) & 1;
return 0;
@@ -1244,21 +1250,21 @@ compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **ent
*
* We silently return on any sign of list-walking problem.
*/
-static void compat_exit_robust_list(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr)
{
- struct compat_robust_list_head __user *head = curr->compat_robust_list;
+ struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
- compat_uptr_t uentry, next_uentry, upending;
- compat_long_t futex_offset;
+ u32 uentry, next_uentry, upending;
+ s32 futex_offset;
int rc;
/*
* Fetch the list head (which was registered earlier, via
* sys_set_robust_list()):
*/
- if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
+ if (fetch_robust_entry32((u32 *)&uentry, &entry, (u32 *)&head->list.next, &pi))
return;
/*
* Fetch the relative futex offset:
@@ -1269,7 +1275,7 @@ static void compat_exit_robust_list(struct task_struct *curr)
* Fetch any possibly pending lock-add first, and handle it
* if it exists:
*/
- if (compat_fetch_robust_entry(&upending, &pending,
+ if (fetch_robust_entry32(&upending, &pending,
&head->list_op_pending, &pip))
return;
@@ -1279,8 +1285,8 @@ static void compat_exit_robust_list(struct task_struct *curr)
* 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);
+ rc = fetch_robust_entry32(&next_uentry, &next_entry,
+ (u32 __user *)&entry->next, &next_pi);
/*
* A pending lock might already be on the list, so
* dont process it twice:
@@ -1311,7 +1317,6 @@ static void compat_exit_robust_list(struct task_struct *curr)
handle_futex_death(uaddr, curr, pip, HANDLE_DEATH_PENDING);
}
}
-#endif
#ifdef CONFIG_FUTEX_PI
@@ -1406,14 +1411,21 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
+#ifdef CONFIG_64BIT
if (unlikely(tsk->robust_list)) {
- exit_robust_list(tsk);
+ exit_robust_list64(tsk);
tsk->robust_list = NULL;
}
+#else
+ if (unlikely(tsk->robust_list)) {
+ exit_robust_list32(tsk);
+ tsk->robust_list = NULL;
+ }
+#endif
#ifdef CONFIG_COMPAT
if (unlikely(tsk->compat_robust_list)) {
- compat_exit_robust_list(tsk);
+ exit_robust_list32(tsk);
tsk->compat_robust_list = NULL;
}
#endif
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 4b6da9116aa6c33db9796e3055ce0c90b02d7b91..dba193dfd216cc929c8f4d979aa2bcd99237e2d8 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -440,7 +440,7 @@ SYSCALL_DEFINE4(futex_requeue,
#ifdef CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE2(set_robust_list,
- struct compat_robust_list_head __user *, head,
+ struct robust_list_head32 __user *, head,
compat_size_t, len)
{
if (unlikely(len != sizeof(*head)))
@@ -455,7 +455,7 @@ 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;
+ struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
--
2.49.0
^ permalink raw reply related
* [PATCH v4 2/7] selftests/futex: Create test for robust list
From: André Almeida @ 2025-05-20 20:48 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, André Almeida
In-Reply-To: <20250520-tonyk-robust_futex-v4-0-1123093e59de@igalia.com>
Create a test for the robust list mechanism. Test the following uAPI
operations:
- Creating a robust mutex where the lock waiter is wake by the kernel
when the lock owner died
- Setting a robust list to the current task
- Getting a robust list from the current task
- Getting a robust list from another task
- Using the list_op_pending field from robust_list_head struct to test
robustness when the lock owner dies before completing the locking
- Setting a invalid size for syscall argument `len`
- Adding multiple elements to a robust list wait waiting for each of
them
- Creating a circular list and checking that the kernel does not get
stuck in an infinity loop
This is the expected output:
TAP version 13
1..7
ok 1 test_robustness
ok 2 test_set_robust_list_invalid_size
ok 3 test_get_robust_list_self
ok 4 test_get_robust_list_child
ok 5 test_set_list_op_pending
ok 6 test_robust_list_multiple_elements
ok 7 test_circular_list
# Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
.../testing/selftests/futex/functional/.gitignore | 1 +
tools/testing/selftests/futex/functional/Makefile | 3 +-
.../selftests/futex/functional/robust_list.c | 554 +++++++++++++++++++++
3 files changed, 557 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/futex/functional/.gitignore b/tools/testing/selftests/futex/functional/.gitignore
index 7b24ae89594a9db211d4b8469ebcef8d1f7012d8..7f447ebfbc62bbad9add0dc86a75abcdb8a4d9a7 100644
--- a/tools/testing/selftests/futex/functional/.gitignore
+++ b/tools/testing/selftests/futex/functional/.gitignore
@@ -11,3 +11,4 @@ futex_wait_timeout
futex_wait_uninitialized_heap
futex_wait_wouldblock
futex_waitv
+robust_list
diff --git a/tools/testing/selftests/futex/functional/Makefile b/tools/testing/selftests/futex/functional/Makefile
index 8cfb87f7f7c5059c82f1e6290c076d3f13f5ea41..e6fa66e622dee4de74c31c8b9b486ca01de35737 100644
--- a/tools/testing/selftests/futex/functional/Makefile
+++ b/tools/testing/selftests/futex/functional/Makefile
@@ -20,7 +20,8 @@ TEST_GEN_PROGS := \
futex_priv_hash \
futex_numa_mpol \
futex_waitv \
- futex_numa
+ futex_numa \
+ robust_list
TEST_PROGS := run.sh
diff --git a/tools/testing/selftests/futex/functional/robust_list.c b/tools/testing/selftests/futex/functional/robust_list.c
new file mode 100644
index 0000000000000000000000000000000000000000..42690b2440fd29a9b12c46f67f9645ccc93d1147
--- /dev/null
+++ b/tools/testing/selftests/futex/functional/robust_list.c
@@ -0,0 +1,554 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Igalia S.L.
+ *
+ * Robust list test by André Almeida <andrealmeid@igalia.com>
+ *
+ * The robust list uAPI allows userspace to create "robust" locks, in the sense
+ * that if the lock holder thread dies, the remaining threads that are waiting
+ * for the lock won't block forever, waiting for a lock that will never be
+ * released.
+ *
+ * This is achieve by userspace setting a list where a thread can enter all the
+ * locks (futexes) that it is holding. The robust list is a linked list, and
+ * userspace register the start of the list with the syscall set_robust_list().
+ * If such thread eventually dies, the kernel will walk this list, waking up one
+ * thread waiting for each futex and marking the futex word with the flag
+ * FUTEX_OWNER_DIED.
+ *
+ * See also
+ * man set_robust_list
+ * Documententation/locking/robust-futex-ABI.rst
+ * Documententation/locking/robust-futexes.rst
+ */
+
+#define _GNU_SOURCE
+
+#include "futextest.h"
+#include "logging.h"
+
+#include <errno.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdatomic.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <sys/mman.h>
+#include <sys/wait.h>
+
+#define STACK_SIZE (1024 * 1024)
+
+#define FUTEX_TIMEOUT 3
+
+static pthread_barrier_t barrier, barrier2;
+
+int set_robust_list(struct robust_list_head *head, size_t len)
+{
+ return syscall(SYS_set_robust_list, head, len);
+}
+
+int get_robust_list(int pid, struct robust_list_head **head, size_t *len_ptr)
+{
+ return syscall(SYS_get_robust_list, pid, head, len_ptr);
+}
+
+/*
+ * Basic lock struct, contains just the futex word and the robust list element
+ * Real implementations have also a *prev to easily walk in the list
+ */
+struct lock_struct {
+ _Atomic(unsigned int) futex;
+ struct robust_list list;
+};
+
+/*
+ * Helper function to spawn a child thread. Returns -1 on error, pid on success
+ */
+static int create_child(int (*fn)(void *arg), void *arg)
+{
+ char *stack;
+ pid_t pid;
+
+ stack = mmap(NULL, STACK_SIZE, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
+ if (stack == MAP_FAILED)
+ return -1;
+
+ stack += STACK_SIZE;
+
+ pid = clone(fn, stack, CLONE_VM | SIGCHLD, arg);
+
+ if (pid == -1)
+ return -1;
+
+ return pid;
+}
+
+/*
+ * Helper function to prepare and register a robust list
+ */
+static int set_list(struct robust_list_head *head)
+{
+ int ret;
+
+ ret = set_robust_list(head, sizeof(struct robust_list_head));
+ if (ret)
+ return ret;
+
+ head->futex_offset = (size_t) offsetof(struct lock_struct, futex) -
+ (size_t) offsetof(struct lock_struct, list);
+ head->list.next = &head->list;
+ head->list_op_pending = NULL;
+
+ return 0;
+}
+
+/*
+ * A basic (and incomplete) mutex lock function with robustness
+ */
+static int mutex_lock(struct lock_struct *lock, struct robust_list_head *head, bool error_inject)
+{
+ _Atomic(unsigned int) *futex = &lock->futex;
+ unsigned int zero = 0;
+ int ret = -1;
+ pid_t tid = gettid();
+
+ /*
+ * Set list_op_pending before starting the lock, so the kernel can catch
+ * the case where the thread died during the lock operation
+ */
+ head->list_op_pending = &lock->list;
+
+ if (atomic_compare_exchange_strong(futex, &zero, tid)) {
+ /*
+ * We took the lock, insert it in the robust list
+ */
+ struct robust_list *list = &head->list;
+
+ /* Error injection to test list_op_pending */
+ if (error_inject)
+ return 0;
+
+ while (list->next != &head->list)
+ list = list->next;
+
+ list->next = &lock->list;
+ lock->list.next = &head->list;
+
+ ret = 0;
+ } else {
+ /*
+ * We didn't take the lock, wait until the owner wakes (or dies)
+ */
+ struct timespec to;
+
+ to.tv_sec = FUTEX_TIMEOUT;
+ to.tv_nsec = 0;
+
+ tid = atomic_load(futex);
+ /* Kernel ignores futexes without the waiters flag */
+ tid |= FUTEX_WAITERS;
+ atomic_store(futex, tid);
+
+ ret = futex_wait((futex_t *) futex, tid, &to, 0);
+
+ /*
+ * A real mutex_lock() implementation would loop here to finally
+ * take the lock. We don't care about that, so we stop here.
+ */
+ }
+
+ head->list_op_pending = NULL;
+
+ return ret;
+}
+
+/*
+ * This child thread will succeed taking the lock, and then will exit holding it
+ */
+static int child_fn_lock(void *arg)
+{
+ struct lock_struct *lock = (struct lock_struct *) arg;
+ struct robust_list_head head;
+ int ret;
+
+ ret = set_list(&head);
+ if (ret)
+ ksft_test_result_fail("set_robust_list error\n");
+
+ ret = mutex_lock(lock, &head, false);
+ if (ret)
+ ksft_test_result_fail("mutex_lock error\n");
+
+ pthread_barrier_wait(&barrier);
+
+ /*
+ * There's a race here: the parent thread needs to be inside
+ * futex_wait() before the child thread dies, otherwise it will miss the
+ * wakeup from handle_futex_death() that this child will emit. We wait a
+ * little bit just to make sure that this happens.
+ */
+ sleep(1);
+
+ return 0;
+}
+
+/*
+ * Spawns a child thread that will set a robust list, take the lock, register it
+ * in the robust list and die. The parent thread will wait on this futex, and
+ * should be waken up when the child exits.
+ */
+static void test_robustness(void)
+{
+ struct lock_struct lock = { .futex = 0 };
+ struct robust_list_head head;
+ _Atomic(unsigned int) *futex = &lock.futex;
+ int ret;
+
+ ret = set_list(&head);
+ ASSERT_EQ(ret, 0);
+
+ /*
+ * Lets use a barrier to ensure that the child thread takes the lock
+ * before the parent
+ */
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ASSERT_EQ(ret, 0);
+
+ ret = create_child(&child_fn_lock, &lock);
+ ASSERT_NE(ret, -1);
+
+ pthread_barrier_wait(&barrier);
+ ret = mutex_lock(&lock, &head, false);
+
+ /*
+ * futex_wait() should return 0 and the futex word should be marked with
+ * FUTEX_OWNER_DIED
+ */
+ ASSERT_EQ(ret, 0);
+ if (ret != 0)
+ printf("futex wait returned %d", errno);
+
+ ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
+
+ wait(NULL);
+ pthread_barrier_destroy(&barrier);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+/*
+ * The only valid value for len is sizeof(*head)
+ */
+static void test_set_robust_list_invalid_size(void)
+{
+ struct robust_list_head head;
+ size_t head_size = sizeof(struct robust_list_head);
+ int ret;
+
+ ret = set_robust_list(&head, head_size);
+ ASSERT_EQ(ret, 0);
+
+ ret = set_robust_list(&head, head_size * 2);
+ ASSERT_EQ(ret, -1);
+ ASSERT_EQ(errno, EINVAL);
+
+ ret = set_robust_list(&head, head_size - 1);
+ ASSERT_EQ(ret, -1);
+ ASSERT_EQ(errno, EINVAL);
+
+ ret = set_robust_list(&head, 0);
+ ASSERT_EQ(ret, -1);
+ ASSERT_EQ(errno, EINVAL);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+/*
+ * Test get_robust_list with pid = 0, getting the list of the running thread
+ */
+static void test_get_robust_list_self(void)
+{
+ struct robust_list_head head, head2, *get_head;
+ size_t head_size = sizeof(struct robust_list_head), len_ptr;
+ int ret;
+
+ ret = set_robust_list(&head, head_size);
+ ASSERT_EQ(ret, 0);
+
+ ret = get_robust_list(0, &get_head, &len_ptr);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(get_head, &head);
+ ASSERT_EQ(head_size, len_ptr);
+
+ ret = set_robust_list(&head2, head_size);
+ ASSERT_EQ(ret, 0);
+
+ ret = get_robust_list(0, &get_head, &len_ptr);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(get_head, &head2);
+ ASSERT_EQ(head_size, len_ptr);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+static int child_list(void *arg)
+{
+ struct robust_list_head *head = (struct robust_list_head *) arg;
+ int ret;
+
+ ret = set_robust_list(head, sizeof(struct robust_list_head));
+ if (ret)
+ ksft_test_result_fail("set_robust_list error\n");
+
+ pthread_barrier_wait(&barrier);
+ pthread_barrier_wait(&barrier2);
+
+ return 0;
+}
+
+/*
+ * Test get_robust_list from another thread. We use two barriers here to ensure
+ * that:
+ * 1) the child thread set the list before we try to get it from the
+ * parent
+ * 2) the child thread still alive when we try to get the list from it
+ */
+static void test_get_robust_list_child(void)
+{
+ pid_t tid;
+ int ret;
+ struct robust_list_head head, *get_head;
+ size_t len_ptr;
+
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ret = pthread_barrier_init(&barrier2, NULL, 2);
+ ASSERT_EQ(ret, 0);
+
+ tid = create_child(&child_list, &head);
+ ASSERT_NE(tid, -1);
+
+ pthread_barrier_wait(&barrier);
+
+ ret = get_robust_list(tid, &get_head, &len_ptr);
+ ASSERT_EQ(ret, 0);
+ ASSERT_EQ(&head, get_head);
+
+ pthread_barrier_wait(&barrier2);
+
+ wait(NULL);
+ pthread_barrier_destroy(&barrier);
+ pthread_barrier_destroy(&barrier2);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+static int child_fn_lock_with_error(void *arg)
+{
+ struct lock_struct *lock = (struct lock_struct *) arg;
+ struct robust_list_head head;
+ int ret;
+
+ ret = set_list(&head);
+ if (ret)
+ ksft_test_result_fail("set_robust_list error\n");
+
+ ret = mutex_lock(lock, &head, true);
+ if (ret)
+ ksft_test_result_fail("mutex_lock error\n");
+
+ pthread_barrier_wait(&barrier);
+
+ sleep(1);
+
+ return 0;
+}
+
+/*
+ * Same as robustness test, but inject an error where the mutex_lock() exits
+ * earlier, just after setting list_op_pending and taking the lock, to test the
+ * list_op_pending mechanism
+ */
+static void test_set_list_op_pending(void)
+{
+ struct lock_struct lock = { .futex = 0 };
+ struct robust_list_head head;
+ _Atomic(unsigned int) *futex = &lock.futex;
+ int ret;
+
+ ret = set_list(&head);
+ ASSERT_EQ(ret, 0);
+
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ASSERT_EQ(ret, 0);
+
+ ret = create_child(&child_fn_lock_with_error, &lock);
+ ASSERT_NE(ret, -1);
+
+ pthread_barrier_wait(&barrier);
+ ret = mutex_lock(&lock, &head, false);
+
+ ASSERT_EQ(ret, 0);
+ if (ret != 0)
+ printf("futex wait returned %d", errno);
+
+ ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
+
+ wait(NULL);
+ pthread_barrier_destroy(&barrier);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+#define CHILD_NR 10
+
+static int child_lock_holder(void *arg)
+{
+ struct lock_struct *locks = (struct lock_struct *) arg;
+ struct robust_list_head head;
+ int i;
+
+ set_list(&head);
+
+ for (i = 0; i < CHILD_NR; i++) {
+ locks[i].futex = 0;
+ mutex_lock(&locks[i], &head, false);
+ }
+
+ pthread_barrier_wait(&barrier);
+ pthread_barrier_wait(&barrier2);
+
+ sleep(1);
+ return 0;
+}
+
+static int child_wait_lock(void *arg)
+{
+ struct lock_struct *lock = (struct lock_struct *) arg;
+ struct robust_list_head head;
+ int ret;
+
+ pthread_barrier_wait(&barrier2);
+ ret = mutex_lock(lock, &head, false);
+
+ if (ret)
+ ksft_test_result_fail("mutex_lock error\n");
+
+ if (!(lock->futex | FUTEX_OWNER_DIED))
+ ksft_test_result_fail("futex not marked with FUTEX_OWNER_DIED\n");
+
+ return 0;
+}
+
+/*
+ * Test a robust list of more than one element. All the waiters should wake when
+ * the holder dies
+ */
+static void test_robust_list_multiple_elements(void)
+{
+ struct lock_struct locks[CHILD_NR];
+ int i, ret;
+
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ASSERT_EQ(ret, 0);
+ ret = pthread_barrier_init(&barrier2, NULL, CHILD_NR + 1);
+ ASSERT_EQ(ret, 0);
+
+ create_child(&child_lock_holder, &locks);
+
+ /* Wait until the locker thread takes the look */
+ pthread_barrier_wait(&barrier);
+
+ for (i = 0; i < CHILD_NR; i++)
+ create_child(&child_wait_lock, &locks[i]);
+
+ /* Wait for all children to return */
+ while (wait(NULL) > 0);
+
+ pthread_barrier_destroy(&barrier);
+ pthread_barrier_destroy(&barrier2);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+static int child_circular_list(void *arg)
+{
+ static struct robust_list_head head;
+ struct lock_struct a, b, c;
+ int ret;
+
+ ret = set_list(&head);
+ if (ret)
+ ksft_test_result_fail("set_list error\n");
+
+ head.list.next = &a.list;
+
+ /*
+ * The last element should point to head list, but we short circuit it
+ */
+ a.list.next = &b.list;
+ b.list.next = &c.list;
+ c.list.next = &a.list;
+
+ return 0;
+}
+
+/*
+ * Create a circular robust list. The kernel should be able to destroy the list
+ * while processing it so it won't be trapped in an infinite loop while handling
+ * a process exit
+ */
+static void test_circular_list(void)
+{
+ create_child(child_circular_list, NULL);
+
+ wait(NULL);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+void usage(char *prog)
+{
+ printf("Usage: %s\n", prog);
+ printf(" -c Use color\n");
+ printf(" -h Display this help message\n");
+ printf(" -v L Verbosity level: %d=QUIET %d=CRITICAL %d=INFO\n",
+ VQUIET, VCRITICAL, VINFO);
+}
+
+int main(int argc, char *argv[])
+{
+ int c;
+
+ while ((c = getopt(argc, argv, "cht:v:")) != -1) {
+ switch (c) {
+ case 'c':
+ log_color(1);
+ break;
+ case 'h':
+ usage(basename(argv[0]));
+ exit(0);
+ case 'v':
+ log_verbosity(atoi(optarg));
+ break;
+ default:
+ usage(basename(argv[0]));
+ exit(1);
+ }
+ }
+
+ ksft_print_header();
+ ksft_set_plan(7);
+
+ test_robustness();
+
+ test_set_robust_list_invalid_size();
+ test_get_robust_list_self();
+ test_get_robust_list_child();
+ test_set_list_op_pending();
+ test_robust_list_multiple_elements();
+ test_circular_list();
+
+ ksft_print_cnts();
+ return 0;
+}
--
2.49.0
^ permalink raw reply related
* [PATCH v4 0/7] futex: Create set_robust_list2
From: André Almeida @ 2025-05-20 20:48 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, André Almeida
This patch adds a new robust_list() syscall. The current syscall
can't be expanded to cover the following use case, so a new one is
needed. This new syscall allows users to set multiple robust lists per
process and to have either 32bit or 64bit pointers in the list.
* Use case
FEX-Emu[1] is an application that runs x86 and x86-64 binaries on an
AArch64 Linux host. One of the tasks of FEX-Emu is to translate syscalls
from one platform to another. Existing set_robust_list() can't be easily
translated because of two limitations:
1) x86 apps can have 32bit pointers robust lists. For a x86-64 kernel
this is not a problem, because of the compat entry point. But there's
no such compat entry point for AArch64, so the kernel would do the
pointer arithmetic wrongly. Is also unviable to userspace to keep
track every addition/removal to the robust list and keep a 64bit
version of it somewhere else to feed the kernel. Thus, the new
interface has an option of telling the kernel if the list is filled
with 32bit or 64bit pointers.
2) Apps can set just one robust list (in theory, x86-64 can set two if
they also use the compat entry point). That means that when a x86 app
asks FEX-Emu to call set_robust_list(), FEX have two options: to
overwrite their own robust list pointer and make the app robust, or
to ignore the app robust list and keep the emulator robust. The new
interface allows for multiple robust lists per application, solving
this.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a normal
robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the naming
starts to get confusing, I reckon). If `index == -1`, that means that user wants
to set a new robust_list, and the kernel will append it in the end of the list,
assign a new index and return this index to the user. If `index >= 0`, that
means that user wants to re-set `*head` of an already existing list (similarly
to what happens when you call set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list, or if
the internal list is full, an error is returned.
* Implementation
The implementation re-uses most of the existing robust list interface as
possible. The new task_struct member `struct list_head robust_list2` is just a
linked list where new lists are appended as the user requests more lists, and by
futex_cleanup(), the kernel walks through the internal list feeding
exit_robust_list() with the robust_list's.
This implementation supports up to 10 lists (defined at ROBUST_LISTS_PER_TASK),
but it was an arbitrary number for this RFC. For the described use case above, 4
should be enough, I'm not sure which should be the limit.
It doesn't support list removal (should it support?). It doesn't have a proper
get_robust_list2() yet as well, but I can add it in a next revision. We could
also have a generic robust_list() syscall that can be used to set/get and be
controlled by flags.
The new interface has a `unsigned int flags` argument, making it
extensible for future use cases as well.
It refuses unaligned `head` addresses. It doesn't have a limit for elements in a
single list (like ROBUST_LIST_LIMIT), it destroys the list as it is parsed to be
safe against circular lists.
* Testing
This patcheset has a selftest patch that expands this one:
https://lore.kernel.org/lkml/20250212131123.37431-1-andrealmeid@igalia.com/
Also, FEX-Emu added support for this interface to validate it:
https://github.com/FEX-Emu/FEX/pull/3966
Feedback is very welcomed!
Thanks,
André
[1] https://github.com/FEX-Emu/FEX
Changelog:
- Rebased on top of new futex work (private hash)
v4: https://lore.kernel.org/lkml/20250225183531.682556-1-andrealmeid@igalia.com/
- Refuse unaligned head pointers
- Ignore ROBUST_LIST_LIMIT for lists created with this interface and make it
robust against circular lists
- Fix a get_robust_list() syscall bug for getting the list from another thread
- Adapt selftest to use the new interface
v3: https://lore.kernel.org/lkml/20241217174958.477692-1-andrealmeid@igalia.com/
- Old syscall set_robust_list() adds new head to the internal linked list of
robust lists pointers, instead of having a field just for them. Remove
tsk->robust_list and use only tsk->robust_list2
v2: https://lore.kernel.org/lkml/20241101162147.284993-1-andrealmeid@igalia.com/
- Added a patch to properly deal with exit_robust_list() in 64bit vs 32bit
- Wired-up syscall for all archs
- Added more of the cover letter to the commit message
v1: https://lore.kernel.org/lkml/20241024145735.162090-1-andrealmeid@igalia.com/
---
André Almeida (7):
selftests/futex: Add ASSERT_ macros
selftests/futex: Create test for robust list
futex: Use explicit sizes for compat_exit_robust_list
futex: Create set_robust_list2
futex: Wire up set_robust_list2 syscall
futex: Remove the limit of elements for sys_set_robust_list2 lists
selftests: futex: Expand robust list test for the new interface
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/compat.h | 12 +-
include/linux/futex.h | 16 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 2 +
include/uapi/linux/futex.h | 24 +
kernel/futex/core.c | 165 ++++-
kernel/futex/futex.h | 5 +
kernel/futex/syscalls.c | 85 ++-
kernel/sys_ni.c | 1 +
scripts/syscall.tbl | 1 +
.../testing/selftests/futex/functional/.gitignore | 1 +
tools/testing/selftests/futex/functional/Makefile | 3 +-
.../selftests/futex/functional/robust_list.c | 706 +++++++++++++++++++++
tools/testing/selftests/futex/include/logging.h | 38 ++
29 files changed, 1026 insertions(+), 53 deletions(-)
---
base-commit: 3ee84e3dd88e39b55b534e17a7b9a181f1d46809
change-id: 20250225-tonyk-robust_futex-60adeedac695
Best regards,
--
André Almeida <andrealmeid@igalia.com>
^ permalink raw reply
* [PATCH v4 1/7] selftests/futex: Add ASSERT_ macros
From: André Almeida @ 2025-05-20 20:48 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, André Almeida
In-Reply-To: <20250520-tonyk-robust_futex-v4-0-1123093e59de@igalia.com>
Create ASSERT_{EQ, NE, TRUE, FALSE} macros to make test creation easier.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
tools/testing/selftests/futex/include/logging.h | 38 +++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/tools/testing/selftests/futex/include/logging.h b/tools/testing/selftests/futex/include/logging.h
index 874c69ce5cce9efa3a9d6de246f5972a75437dbf..a19755622a877932884570c8f58aaee7371d5f8f 100644
--- a/tools/testing/selftests/futex/include/logging.h
+++ b/tools/testing/selftests/futex/include/logging.h
@@ -23,6 +23,44 @@
#include <linux/futex.h>
#include "kselftest.h"
+#define ASSERT_EQ(var, value) \
+do { \
+ if (var != value) { \
+ ksft_test_result_fail("%s: expected %ld, but %s has %ld\n", \
+ __func__, (long) value, #var, \
+ (long) var); \
+ return; \
+ } \
+} while (0)
+
+#define ASSERT_NE(var, value) \
+do { \
+ if (var == value) { \
+ ksft_test_result_fail("%s: expected not %ld, but %s has %ld\n", \
+ __func__, (long) value, #var, \
+ (long) var); \
+ return; \
+ } \
+} while (0)
+
+#define ASSERT_TRUE(var) \
+do { \
+ if ((var) == 0) { \
+ ksft_test_result_fail("%s: expected %s to be true\n", \
+ __func__, #var); \
+ return; \
+ } \
+} while (0)
+
+#define ASSERT_FALSE(var) \
+do { \
+ if (var) { \
+ ksft_test_result_fail("%s: expected %s to be false\n", \
+ __func__, #var); \
+ return; \
+ } \
+} while (0)
+
/*
* Define PASS, ERROR, and FAIL strings with and without color escape
* sequences, default to no color.
--
2.49.0
^ permalink raw reply related
* Re: close(2) with EINTR has been changed by POSIX.1-2024
From: Theodore Ts'o @ 2025-05-20 13:37 UTC (permalink / raw)
To: Jan Kara, Alejandro Colomar, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, linux-man, Steffen Nurpmeso
In-Reply-To: <20250519231919.StJ5Lkhr@steffen%sdaoden.eu>
On Tue, May 20, 2025 at 01:19:19AM +0200, Steffen Nurpmeso wrote:
>
> They could not do otherwise than talking the status quo, i think.
> They have explicitly added posix_close() which overcomes the
> problem (for those operating systems which actually act like
> that). There is a long RATIONALE on this, it starts on page 747 :)
They could have just added posix_close() which provided well-defined
semantics without demanding that existing implementations make
non-backwards compatible changes to close(2). Personally, while they
were adding posix_close(2) they could have also fixed the disaster
which is the semantics around close(2) and how advisory locks get
released that were held by other file descriptors and add a profound
apologies over the insane semantics demanded by POSIX[1].
[1] "POSIX advisory locks are broken by design."
https://www.sqlite.org/src/artifact/c230a7a24?ln=994-1081
- Ted
^ permalink raw reply
* Re: close(2) with EINTR has been changed by POSIX.1-2024
From: Steffen Nurpmeso @ 2025-05-19 23:19 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jan Kara, Alejandro Colomar, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, linux-man, Steffen Nurpmeso
In-Reply-To: <20250516124147.GB7158@mit.edu>
Theodore Ts'o wrote in
<20250516124147.GB7158@mit.edu>:
|On Fri, May 16, 2025 at 12:48:56PM +0200, Jan Kara wrote:
|>> Now, POSIX.1-2024 says:
|>>
|>> If close() is interrupted by a signal that is to be caught, then
|>> it is unspecified whether it returns -1 with errno set to
|>> [EINTR] and fildes remaining open, or returns -1 with errno set
|>> to [EINPROGRESS] and fildes being closed, or returns 0 to
|>> indicate successful completion; [...]
|>>
|>> <https://pubs.opengroup.org/onlinepubs/9799919799/functions/close.html>
|>>
|>> Which seems to bless HP-UX and screw all the others, requiring them to
|>> report EINPROGRESS.
|>>
|>> Was there any discussion about what to do in the Linux kernel?
|>
|> I'm not aware of any discussions but indeed we are returning EINTR while
|> closing the fd. Frankly, changing the error code we return in that \
|> case is
|> really asking for userspace regressions so I'm of the opinion we just
|> ignore the standard as in my opinion it goes against a long established
|> reality.
|
|Yeah, it appears that the Austin Group has lost all connection with
|reality, and we should treat POSIX 2024 accordingly. Not breaking
|userspace applications is way more important that POSIX 2024
|compliance. Which is sad, because I used to really care about POSIX.1
|standard as being very useful. But that seems to be no longer the
|case...
They could not do otherwise than talking the status quo, i think.
They have explicitly added posix_close() which overcomes the
problem (for those operating systems which actually act like
that). There is a long RATIONALE on this, it starts on page 747 :)
--steffen
|
|Der Kragenbaer, The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
^ permalink raw reply
* Extending clone_args for clone3()
From: Yury Khrustalev @ 2025-05-19 14:06 UTC (permalink / raw)
To: linux-kernel
Cc: Christian Brauner, Arnd Bergmann, Mark Brown, Mark Rutland,
linux-api
Hi,
I'm working on an RFC patch for Glibc to make use of the newly added
shadow_stack_token field in struct clone_args in [1] on arm64 targets.
I encountered the following problem. Glibc might be built with newer
version of struct clone_args than the currently running kernel. In
this case, we may attempt to use a non-zero value in the new field
in args (and pass size bigger than expected by the kernel) and the
kernel will reject the syscall with E2BIG error.
This seems to be due to a fail-early approach. The unexpected non-
zero values beyond what's supported by the kernel may indicate that
userspace expects something to happen (and may even have allocated
some resources). So it's better to indicate a problem rather than
silently ignore this and have userspace encounter an error later.
However, it creates difficulty with using extended "versions" of
the clone3 syscall. AFAIK, there is no way to ask kernel about
the supported size of struct clone_args except for making syscalls
with decreasing value of size until we stop getting E2BIG.
This seems fragile and may call for writing cumbersome code. In essence,
we will have to have clone30(), clone31(), clone32()... wrappers which
probably defeats the point of why clone3 was added:
if (clone32_supported && clone32(...) == -1 && errno == E2BIG)
{
clone32_supported = false;
/* ... */
}
else if (clone31_supported && clone31(...) == -1 && errno == E2BIG)
{
clone12_supported = false;
/* ... */
}
...
Is there a neat way to work around this? What was the idea for extending
clone_args in practice?
I suppose we can't rely on kernel version because support for extended
clone_args can be backported. In any case, we'd have to do a syscall
for this (it would probably be great to have kernel version in auxv).
I appreciate any advice here.
Thanks,
Yury
[1]: https://lore.kernel.org/all/20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org/
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Michal Hocko @ 2025-05-19 14:02 UTC (permalink / raw)
To: Bharat Agrawal
Cc: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
zhangyiru3@huawei.com, mike.kravetz@oracle.com,
liuzixian4@huawei.com, wuxu.wu@huawei.com,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-api@vger.kernel.org, linux-mm@kvack.org
In-Reply-To: <SJ2PR01MB8345DF192742AC4DB3D2CBB78E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
Hi,
On Mon 19-05-25 10:21:17, Bharat Agrawal wrote:
> Hi all,
>
> Could anyone please help comment on the risks associated with an
> application throwing the "Using mlock ulimits for SHM_HUGETLB is
> deprecated" message on RHEL 8.9 with 4.18.0-513.18.1.el8_9.x86_64
> Linux kernel?
This is not RHEL specific behavior. The current Linus tree has the same
warning which has been added by
: commit 2584e517320bd48dc8d20e38a2621a2dbe58fade
: Author: Ravikiran G Thirumalai <kiran@scalex86.org>
: Date: Tue Mar 31 15:21:26 2009 -0700
:
: mm: reintroduce and deprecate rlimit based access for SHM_HUGETLB
:
: Allow non root users with sufficient mlock rlimits to be able to allocate
: hugetlb backed shm for now. Deprecate this though. This is being
: deprecated because the mlock based rlimit checks for SHM_HUGETLB is not
: consistent with mmap based huge page allocations.
:
: Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
: Reviewed-by: Mel Gorman <mel@csn.ul.ie>
: Cc: William Lee Irwin III <wli@holomorphy.com>
: Cc: Adam Litke <agl@us.ibm.com>
: Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
: Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
HTH
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Bharat Agrawal @ 2025-05-19 12:42 UTC (permalink / raw)
To: Greg KH
Cc: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
mhocko@suse.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-mm@kvack.org, legion@kernel.org
In-Reply-To: <SJ2PR01MB83451550F3A9C1636C9E50618E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
No problem. Thanks for your reply.
Bharat
________________________________________
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Monday, May 19, 2025 06:02 PM
To: Bharat Agrawal <bharat.agrawal@ansys.com>
Cc: hughd@google.com <hughd@google.com>; akpm@linux-foundation.org <akpm@linux-foundation.org>; rientjes@google.com <rientjes@google.com>; zhangyiru3@huawei.com <zhangyiru3@huawei.com>; liuzixian4@huawei.com <liuzixian4@huawei.com>; mhocko@suse.com <mhocko@suse.com>; wuxu.wu@huawei.com <wuxu.wu@huawei.com>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-api@vger.kernel.org <linux-api@vger.kernel.org>; linux-mm@kvack.org <linux-mm@kvack.org>; legion@kernel.org <legion@kernel.org>
Subject: Re: mlock ulimits for SHM_HUGETLB
[External Sender]
On Mon, May 19, 2025 at 12:04:58PM +0000, Bharat Agrawal wrote:
> Thanks Greg for the response. RHEL has not been very helpful. I'm not looking to ask for patches because of the old versions.
> These messages appear in production runs, raising concerns about possible failures. Thus, the question is: Can they be ignored safely?
Again, you are paying them for support for this, please use them, there
is nothing that the community can do to help out here, sorry.
greg k-h
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Greg KH @ 2025-05-19 12:32 UTC (permalink / raw)
To: Bharat Agrawal
Cc: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
zhangyiru3@huawei.com, liuzixian4@huawei.com, mhocko@suse.com,
wuxu.wu@huawei.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-mm@kvack.org, legion@kernel.org
In-Reply-To: <SJ2PR01MB834507D46F44F65980FE09668E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
On Mon, May 19, 2025 at 12:04:58PM +0000, Bharat Agrawal wrote:
> Thanks Greg for the response. RHEL has not been very helpful. I'm not looking to ask for patches because of the old versions.
> These messages appear in production runs, raising concerns about possible failures. Thus, the question is: Can they be ignored safely?
Again, you are paying them for support for this, please use them, there
is nothing that the community can do to help out here, sorry.
greg k-h
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Bharat Agrawal @ 2025-05-19 12:05 UTC (permalink / raw)
To: Greg KH
Cc: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
zhangyiru3@huawei.com, liuzixian4@huawei.com, mhocko@suse.com,
wuxu.wu@huawei.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-mm@kvack.org, legion@kernel.org
In-Reply-To: <SJ2PR01MB834507D46F44F65980FE09668E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
Thanks Greg for the response. RHEL has not been very helpful. I'm not looking to ask for patches because of the old versions.
These messages appear in production runs, raising concerns about possible failures. Thus, the question is: Can they be ignored safely?
Best,
Bharat
________________________________________
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Monday, May 19, 2025 05:02 PM
To: Bharat Agrawal <bharat.agrawal@ansys.com>
Cc: hughd@google.com <hughd@google.com>; akpm@linux-foundation.org <akpm@linux-foundation.org>; rientjes@google.com <rientjes@google.com>; zhangyiru3@huawei.com <zhangyiru3@huawei.com>; liuzixian4@huawei.com <liuzixian4@huawei.com>; mhocko@suse.com <mhocko@suse.com>; wuxu.wu@huawei.com <wuxu.wu@huawei.com>; linux-fsdevel@vger.kernel.org <linux-fsdevel@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-api@vger.kernel.org <linux-api@vger.kernel.org>; linux-mm@kvack.org <linux-mm@kvack.org>
Subject: Re: mlock ulimits for SHM_HUGETLB
[External Sender]
On Mon, May 19, 2025 at 10:23:33AM +0000, Bharat Agrawal wrote:
> Hi all,
>
> Could anyone please help comment on the risks associated with an application throwing the "Using mlock ulimits for SHM_HUGETLB is deprecated" message on RHEL 8.9 with 4.18.0-513.18.1.el8_9.x86_64 Linux kernel?
Why not ask RHEL support, given that you are paying them for that in
order to be using that kernel version, right?
Also note that 4.18.y is VERY old and obsolete and not supported by the
community at all.
Good luck!
greg k-h
^ permalink raw reply
* Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls
From: Dave Chinner @ 2025-05-19 11:37 UTC (permalink / raw)
To: Amir Goldstein
Cc: Christian Brauner, Arnd Bergmann, Andrey Albershteyn,
Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
Will Deacon, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
David S . Miller, Andreas Larsson, Andy Lutomirski,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
Jan Kara, Mickaël Salaün, Günther Noack,
Pali Rohár, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Ondrej Mosnacek, Tyler Hicks, Miklos Szeredi,
linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, linux-fsdevel, linux-security-module, linux-api,
Linux-Arch, selinux, ecryptfs, linux-unionfs, linux-xfs,
Andrey Albershteyn
In-Reply-To: <CAOQ4uxicuEkOas2UR4mqfus9Q2RAeKKYTwbE2XrkcE_zp8oScQ@mail.gmail.com>
On Thu, May 15, 2025 at 12:33:31PM +0200, Amir Goldstein wrote:
> On Thu, May 15, 2025 at 11:02 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, May 13, 2025 at 11:53:23AM +0200, Arnd Bergmann wrote:
> > > On Tue, May 13, 2025, at 11:17, Andrey Albershteyn wrote:
> > >
> > > >
> > > > long syscall(SYS_file_getattr, int dirfd, const char *pathname,
> > > > struct fsxattr *fsx, size_t size, unsigned int at_flags);
> > > > long syscall(SYS_file_setattr, int dirfd, const char *pathname,
> > > > struct fsxattr *fsx, size_t size, unsigned int at_flags);
> > >
> > > I don't think we can have both the "struct fsxattr" from the uapi
> > > headers, and a variable size as an additional argument. I would
> > > still prefer not having the extensible structure at all and just
> >
> > We're not going to add new interfaces that are fixed size unless for the
> > very basic cases. I don't care if we're doing that somewhere else in the
> > kernel but we're not doing that for vfs apis.
> >
> > > use fsxattr, but if you want to make it extensible in this way,
> > > it should use a different structure (name). Otherwise adding
> > > fields after fsx_pad[] would break the ioctl interface.
> >
> > Would that really be a problem? Just along the syscall simply add
> > something like:
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index c91fd2b46a77..d3943805c4be 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -868,12 +868,6 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > case FS_IOC_SETFLAGS:
> > return ioctl_setflags(filp, argp);
> >
> > - case FS_IOC_FSGETXATTR:
> > - return ioctl_fsgetxattr(filp, argp);
> > -
> > - case FS_IOC_FSSETXATTR:
> > - return ioctl_fssetxattr(filp, argp);
> > -
> > case FS_IOC_GETFSUUID:
> > return ioctl_getfsuuid(filp, argp);
> >
> > @@ -886,6 +880,20 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > break;
> > }
> >
> > + switch (_IOC_NR(cmd)) {
> > + case _IOC_NR(FS_IOC_FSGETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_FSGETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fsgetxattr(filp, argp);
> > +
> > + case _IOC_NR(FFS_IOC_FSSETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) != _IOC_TYPE(FFS_IOC_FSSETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fssetxattr(filp, argp);
> > + }
> > +
>
> I think what Arnd means is that we will not be able to change struct
> sfxattr in uapi
> going forward, because we are not going to deprecate the ioctls and
There's no need to deprecate anything to rev an ioctl API. We have
had to solve this "changing struct size" problem previously in XFS
ioctls. See XFS_IOC_FSGEOMETRY and the older XFS_IOC_FSGEOMETRY_V4
and XFS_IOC_FSGEOMETRY_V1 versions of the API/ABI.
If we need to increase the structure size, we can rename the existing
ioctl and struct to fix the version in the API, then use the
original name for the new ioctl and structure definition.
The only thing we have to make sure of is that the old and new
structures have exactly the same overlapping structure. i.e.
extension must always be done by appending new varibles, they can't
be put in the middle of the structure.
This way applications being rebuild will pick up the new definition
automatically when the system asserts that it is suppored, whilst
existing binaries will always still be supported by the kernel.
If the application wants/needs to support all possible kernels, then
if XFS_IOC_FSGEOMETRY is not supported, call XFS_IOC_FSGEOMETRY_V4,
and if that fails (only on really old irix!) or you only need
something in that original subset, call XFS_IOC_FSGEOMETRY_V1 which
will always succeed....
> Should we will need to depart from this struct definition and we might
> as well do it for the initial release of the syscall rather than later on, e.g.:
>
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -148,6 +148,17 @@ struct fsxattr {
> unsigned char fsx_pad[8];
> };
>
> +/*
> + * Variable size structure for file_[sg]et_attr().
> + */
> +struct fsx_fileattr {
> + __u32 fsx_xflags; /* xflags field value (get/set) */
> + __u32 fsx_extsize; /* extsize field value (get/set)*/
> + __u32 fsx_nextents; /* nextents field value (get) */
> + __u32 fsx_projid; /* project identifier (get/set) */
> + __u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> +};
> +
> +#define FSXATTR_SIZE_VER0 20
> +#define FSXATTR_SIZE_LATEST FSXATTR_SIZE_VER0
If all the structures overlap the same, all that is needed in the
code is to define the structure size that should be copied in and
parsed. i.e:
case FSXATTR..._V1:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr_v1));
case FSXATTR..._V2:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr_v2));
case FSXATTR...:
return ioctl_fsxattr...(args, sizeof(fsx_fileattr));
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Greg KH @ 2025-05-19 11:32 UTC (permalink / raw)
To: Bharat Agrawal
Cc: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
zhangyiru3@huawei.com, liuzixian4@huawei.com, mhocko@suse.com,
wuxu.wu@huawei.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-mm@kvack.org
In-Reply-To: <SJ2PR01MB834515EA00BD7C362A77972F8E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
On Mon, May 19, 2025 at 10:23:33AM +0000, Bharat Agrawal wrote:
> Hi all,
>
> Could anyone please help comment on the risks associated with an application throwing the "Using mlock ulimits for SHM_HUGETLB is deprecated" message on RHEL 8.9 with 4.18.0-513.18.1.el8_9.x86_64 Linux kernel?
Why not ask RHEL support, given that you are paying them for that in
order to be using that kernel version, right?
Also note that 4.18.y is VERY old and obsolete and not supported by the
community at all.
Good luck!
greg k-h
^ permalink raw reply
* Re: mlock ulimits for SHM_HUGETLB
From: Bharat Agrawal @ 2025-05-19 10:23 UTC (permalink / raw)
To: hughd@google.com, akpm@linux-foundation.org, rientjes@google.com,
zhangyiru3@huawei.com, liuzixian4@huawei.com, mhocko@suse.com,
wuxu.wu@huawei.com
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-api@vger.kernel.org, linux-mm@kvack.org
In-Reply-To: <SJ2PR01MB8345DF192742AC4DB3D2CBB78E9CA@SJ2PR01MB8345.prod.exchangelabs.com>
Hi all,
Could anyone please help comment on the risks associated with an application throwing the "Using mlock ulimits for SHM_HUGETLB is deprecated" message on RHEL 8.9 with 4.18.0-513.18.1.el8_9.x86_64 Linux kernel?
Best,
Bharat Agrawal
ANSYS, Inc.
^ permalink raw reply
* Re: [PATCH v5 0/7] fs: introduce file_getattr and file_setattr syscalls
From: Christian Brauner @ 2025-05-19 10:12 UTC (permalink / raw)
To: Amir Goldstein
Cc: Arnd Bergmann, Andrey Albershteyn, Richard Henderson, Matt Turner,
Russell King, Catalin Marinas, Will Deacon, Geert Uytterhoeven,
Michal Simek, Thomas Bogendoerfer, James E . J . Bottomley,
Helge Deller, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz, David S . Miller, Andreas Larsson,
Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Chris Zankel, Max Filippov,
Alexander Viro, Jan Kara, Mickaël Salaün,
Günther Noack, Pali Rohár, Paul Moore, James Morris,
Serge E. Hallyn, Stephen Smalley, Ondrej Mosnacek, Tyler Hicks,
Miklos Szeredi, linux-alpha, linux-kernel, linux-arm-kernel,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-fsdevel, linux-security-module,
linux-api, Linux-Arch, selinux, ecryptfs, linux-unionfs,
linux-xfs, Andrey Albershteyn
In-Reply-To: <CAOQ4uxicuEkOas2UR4mqfus9Q2RAeKKYTwbE2XrkcE_zp8oScQ@mail.gmail.com>
On Thu, May 15, 2025 at 12:33:31PM +0200, Amir Goldstein wrote:
> On Thu, May 15, 2025 at 11:02 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, May 13, 2025 at 11:53:23AM +0200, Arnd Bergmann wrote:
> > > On Tue, May 13, 2025, at 11:17, Andrey Albershteyn wrote:
> > >
> > > >
> > > > long syscall(SYS_file_getattr, int dirfd, const char *pathname,
> > > > struct fsxattr *fsx, size_t size, unsigned int at_flags);
> > > > long syscall(SYS_file_setattr, int dirfd, const char *pathname,
> > > > struct fsxattr *fsx, size_t size, unsigned int at_flags);
> > >
> > > I don't think we can have both the "struct fsxattr" from the uapi
> > > headers, and a variable size as an additional argument. I would
> > > still prefer not having the extensible structure at all and just
> >
> > We're not going to add new interfaces that are fixed size unless for the
> > very basic cases. I don't care if we're doing that somewhere else in the
> > kernel but we're not doing that for vfs apis.
> >
> > > use fsxattr, but if you want to make it extensible in this way,
> > > it should use a different structure (name). Otherwise adding
> > > fields after fsx_pad[] would break the ioctl interface.
> >
> > Would that really be a problem? Just along the syscall simply add
> > something like:
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index c91fd2b46a77..d3943805c4be 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -868,12 +868,6 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > case FS_IOC_SETFLAGS:
> > return ioctl_setflags(filp, argp);
> >
> > - case FS_IOC_FSGETXATTR:
> > - return ioctl_fsgetxattr(filp, argp);
> > -
> > - case FS_IOC_FSSETXATTR:
> > - return ioctl_fssetxattr(filp, argp);
> > -
> > case FS_IOC_GETFSUUID:
> > return ioctl_getfsuuid(filp, argp);
> >
> > @@ -886,6 +880,20 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > break;
> > }
> >
> > + switch (_IOC_NR(cmd)) {
> > + case _IOC_NR(FS_IOC_FSGETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) != _IOC_TYPE(FS_IOC_FSGETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fsgetxattr(filp, argp);
> > +
> > + case _IOC_NR(FFS_IOC_FSSETXATTR):
> > + if (WARN_ON_ONCE(_IOC_TYPE(cmd) != _IOC_TYPE(FFS_IOC_FSSETXATTR)))
> > + return SOMETHING_SOMETHING;
> > + /* Only handle original size. */
> > + return ioctl_fssetxattr(filp, argp);
> > + }
> > +
>
> I think what Arnd means is that we will not be able to change struct
> sfxattr in uapi
> going forward, because we are not going to deprecate the ioctls and
> certainly not
> the XFS specific ioctl XFS_IOC_FSGETXATTRA.
Sure, I'm just saying this could very likely be handled without the
kernel or userspace having to care about the changed structure provided
we teach the kernel to use the ioctl number, not the command and only
ever copy v1 of the struct for the ioctls in new kernels. But anyway...
>
> This struct is part of XFS uapi:
> https://man7.org/linux/man-pages/man2/ioctl_xfs_fsgetxattr.2.html
>
> Should we will need to depart from this struct definition and we might
> as well do it for the initial release of the syscall rather than later on, e.g.:
>
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -148,6 +148,17 @@ struct fsxattr {
> unsigned char fsx_pad[8];
> };
>
> +/*
> + * Variable size structure for file_[sg]et_attr().
> + */
> +struct fsx_fileattr {
> + __u32 fsx_xflags; /* xflags field value (get/set) */
> + __u32 fsx_extsize; /* extsize field value (get/set)*/
> + __u32 fsx_nextents; /* nextents field value (get) */
> + __u32 fsx_projid; /* project identifier (get/set) */
> + __u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> +};
> +
> +#define FSXATTR_SIZE_VER0 20
> +#define FSXATTR_SIZE_LATEST FSXATTR_SIZE_VER0
> +
>
> Right?
Sure, I don't have a problem with that since I find the current name
with "fsxattr" quite problematic anyway.
^ permalink raw reply
* Re: close(2) with EINTR has been changed by POSIX.1-2024
From: Christian Brauner @ 2025-05-19 9:48 UTC (permalink / raw)
To: Jan Kara
Cc: Alejandro Colomar, Alexander Viro, linux-fsdevel, linux-api,
linux-man
In-Reply-To: <ddqmhjc2rpzk2jjvunbt3l3eukcn4xzkocqzdg3j4msihdhzko@fizekvxndg2d>
On Fri, May 16, 2025 at 12:48:56PM +0200, Jan Kara wrote:
> Hi!
>
> On Thu 15-05-25 23:33:22, Alejandro Colomar wrote:
> > I'm updating the manual pages for POSIX.1-2024, and have some doubts
> > about close(2). The manual page for close(2) says (conforming to
> > POSIX.1-2008):
> >
> > The EINTR error is a somewhat special case. Regarding the EINTR
> > error, POSIX.1‐2008 says:
> >
> > If close() is interrupted by a signal that is to be
> > caught, it shall return -1 with errno set to EINTR and
> > the state of fildes is unspecified.
> >
> > This permits the behavior that occurs on Linux and many other
> > implementations, where, as with other errors that may be re‐
> > ported by close(), the file descriptor is guaranteed to be
> > closed. However, it also permits another possibility: that the
> > implementation returns an EINTR error and keeps the file de‐
> > scriptor open. (According to its documentation, HP‐UX’s close()
> > does this.) The caller must then once more use close() to close
> > the file descriptor, to avoid file descriptor leaks. This di‐
> > vergence in implementation behaviors provides a difficult hurdle
> > for portable applications, since on many implementations,
> > close() must not be called again after an EINTR error, and on at
> > least one, close() must be called again. There are plans to ad‐
> > dress this conundrum for the next major release of the POSIX.1
> > standard.
> >
> > TL;DR: close(2) with EINTR is allowed to either leave the fd open or
> > closed, and Linux leaves it closed, while others (HP-UX only?) leaves it
> > open.
> >
> > Now, POSIX.1-2024 says:
> >
> > If close() is interrupted by a signal that is to be caught, then
> > it is unspecified whether it returns -1 with errno set to
> > [EINTR] and fildes remaining open, or returns -1 with errno set
> > to [EINPROGRESS] and fildes being closed, or returns 0 to
> > indicate successful completion; [...]
> >
> > <https://pubs.opengroup.org/onlinepubs/9799919799/functions/close.html>
> >
> > Which seems to bless HP-UX and screw all the others, requiring them to
> > report EINPROGRESS.
> >
> > Was there any discussion about what to do in the Linux kernel?
>
> I'm not aware of any discussions but indeed we are returning EINTR while
> closing the fd. Frankly, changing the error code we return in that case is
> really asking for userspace regressions so I'm of the opinion we just
> ignore the standard as in my opinion it goes against a long established
> reality.
Ignore. We've long since stopped designing apis with input from that
standard in mind. And I think that was a very wise decision.
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Alejandro Colomar @ 2025-05-17 13:46 UTC (permalink / raw)
To: Rich Felker
Cc: Vincent Lefevre, Jan Kara, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, libc-alpha
In-Reply-To: <20250517133251.GY1509@brightrain.aerifal.cx>
[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]
On Sat, May 17, 2025 at 09:32:52AM -0400, Rich Felker wrote:
> On Fri, May 16, 2025 at 04:39:57PM +0200, Vincent Lefevre wrote:
> > On 2025-05-16 09:05:47 -0400, Rich Felker wrote:
> > > FWIW musl adopted the EINPROGRESS as soon as we were made aware of the
> > > issue, and later changed it to returning 0 since applications
> > > (particularly, any written prior to this interpretation) are prone to
> > > interpret EINPROGRESS as an error condition rather than success and
> > > possibly misinterpret it as meaning the fd is still open and valid to
> > > pass to close again.
> >
> > If I understand correctly, this is a poor choice. POSIX.1-2024 says:
> >
> > ERRORS
> > The close() and posix_close() functions shall fail if:
> > [...]
> > [EINPROGRESS]
> > The function was interrupted by a signal and fildes was closed
> > but the close operation is continuing asynchronously.
> >
> > But this does not mean that the asynchronous close operation will
> > succeed.
>
> There are no asynchronous behaviors specified for there to be a
> conformance distinction here. The only observable behaviors happen
> instantly, mainly the release of the file descriptor and the process's
> handle on the underlying resource. Abstractly, there is no async
> operation that could succeed or fail.
>
> > So the application could incorrectly deduce that the close operation
> > was done without any error.
>
> This deduction is correct, not incorrect. Rather, failing with
> EINPROGRESS would make the application incorrectly deduce that there
> might be some error it missed (even if it's aware of the new error
> code), and absolutely does make all existing applications written
> prior to the new text in POSIX 2024 unable to determine if the fd was
> even released and needs to be passed to close again or not.
Hi Rich,
I think this is not correct; at least on Linux. The manual page is very
clear that close(2) should not be retried on error:
Dealing with error returns from close()
A careful programmer will check the return value of close(),
since it is quite possible that errors on a previous write(2)
operation are reported only on the final close() that releases
the open file description. Failing to check the return value
when closing a file may lead to silent loss of data. This can
especially be observed with NFS and with disk quota.
Note, however, that a failure return should be used only for di‐
agnostic purposes (i.e., a warning to the application that there
may still be I/O pending or there may have been failed I/O) or
remedial purposes (e.g., writing the file once more or creating
a backup).
Retrying the close() after a failure return is the wrong thing
to do, since this may cause a reused file descriptor from an‐
other thread to be closed. This can occur because the Linux
kernel always releases the file descriptor early in the close
operation, freeing it for reuse; the steps that may return an
error, such as flushing data to the filesystem or device, occur
only later in the close operation.
...
A careful programmer who wants to know about I/O errors may pre‐
cede close() with a call to fsync(2).
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Rich Felker @ 2025-05-17 13:43 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Theodore Ts'o, Jan Kara, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, libc-alpha
In-Reply-To: <qthuiudgbwuxh4bwwpcvpbosqrz6rl4js46atvenhmujkbjnz4@crakvrigxnz6>
On Sat, May 17, 2025 at 03:03:52PM +0200, Alejandro Colomar wrote:
> Hi,
>
> On Sat, May 17, 2025 at 07:46:48AM +0200, Alejandro Colomar wrote:
> > Hi Ted, Rich,
> >
> > On Fri, May 16, 2025 at 09:05:47AM -0400, Rich Felker wrote:
> > > FWIW musl adopted the EINPROGRESS as soon as we were made aware of the
> > > issue, and later changed it to returning 0 since applications
> > > (particularly, any written prior to this interpretation) are prone to
> > > interpret EINPROGRESS as an error condition rather than success and
> > > possibly misinterpret it as meaning the fd is still open and valid to
> > > pass to close again.
>
> BTW, I don't think that's a correct interpretation. The manual page
> clearly says after close(2), even on error, the fd is closed and not
> usable. The issue I see is a program thinking it failed and trying to
> copy the file again or reporting an error.
The authoritative source here is POSIX not the man page, assuming
you're writing a portable application and not a "Linux application".
Until the lastest issue (POSIX 2024/Issue 8), the state of the fd
after EINTR was explicitly unspecified, and after other errors was
unspecified by omission. So there is no way for a program written to
prior versions of the standard to have known how to safely handle
getting EINPROGRESS -- or any error from close for that matter.
Really, the only safe error for close to return, *ever*, is EBADF. On
valid input, it *must succeed*. This is a general principle for
"deallocation/destruction functions". Not an explicit requirement of
this or any standard; just a logical requirement for forward progress
to be possible.
> On the other hand, as Vincent said, maybe this is not so bad. For
> certain files, fsync(2) is only described for storage devices, so in
> some cases there's no clear way to make sure close(2) won't fail after
> EINTR (maybe calling sync(2)?). So, maybe considering it an error
> wouldn't be a terrible idea.
Whether data is committed to physical storage in a way that's robust
against machine faults is a completely separate issue from whether
it's committed to the abstract storage. The latter happens at the
moment of write, not close.
If an application is trying to ensure that kind of robustness, the
return value of close is not the tool. It needs the Synchronized IO
interfaces (fsync, etc.) or something specific to whatever it's
writing to.
Rich
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Rich Felker @ 2025-05-17 13:32 UTC (permalink / raw)
To: Vincent Lefevre, Alejandro Colomar, Jan Kara, Alexander Viro,
Christian Brauner, linux-fsdevel, linux-api, libc-alpha
In-Reply-To: <20250516143957.GB5388@qaa.vinc17.org>
On Fri, May 16, 2025 at 04:39:57PM +0200, Vincent Lefevre wrote:
> On 2025-05-16 09:05:47 -0400, Rich Felker wrote:
> > FWIW musl adopted the EINPROGRESS as soon as we were made aware of the
> > issue, and later changed it to returning 0 since applications
> > (particularly, any written prior to this interpretation) are prone to
> > interpret EINPROGRESS as an error condition rather than success and
> > possibly misinterpret it as meaning the fd is still open and valid to
> > pass to close again.
>
> If I understand correctly, this is a poor choice. POSIX.1-2024 says:
>
> ERRORS
> The close() and posix_close() functions shall fail if:
> [...]
> [EINPROGRESS]
> The function was interrupted by a signal and fildes was closed
> but the close operation is continuing asynchronously.
>
> But this does not mean that the asynchronous close operation will
> succeed.
There are no asynchronous behaviors specified for there to be a
conformance distinction here. The only observable behaviors happen
instantly, mainly the release of the file descriptor and the process's
handle on the underlying resource. Abstractly, there is no async
operation that could succeed or fail.
> So the application could incorrectly deduce that the close operation
> was done without any error.
This deduction is correct, not incorrect. Rather, failing with
EINPROGRESS would make the application incorrectly deduce that there
might be some error it missed (even if it's aware of the new error
code), and absolutely does make all existing applications written
prior to the new text in POSIX 2024 unable to determine if the fd was
even released and needs to be passed to close again or not.
Rich
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Alejandro Colomar @ 2025-05-17 13:03 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Rich Felker, Jan Kara, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, libc-alpha
In-Reply-To: <bsvslfjgcmzvcanxp3ay6ohitqulwuawwgzy234nfkj6ecdxbq@2uhld4vpitou>
[-- Attachment #1: Type: text/plain, Size: 2731 bytes --]
Hi,
On Sat, May 17, 2025 at 07:46:48AM +0200, Alejandro Colomar wrote:
> Hi Ted, Rich,
>
> On Fri, May 16, 2025 at 09:05:47AM -0400, Rich Felker wrote:
> > FWIW musl adopted the EINPROGRESS as soon as we were made aware of the
> > issue, and later changed it to returning 0 since applications
> > (particularly, any written prior to this interpretation) are prone to
> > interpret EINPROGRESS as an error condition rather than success and
> > possibly misinterpret it as meaning the fd is still open and valid to
> > pass to close again.
BTW, I don't think that's a correct interpretation. The manual page
clearly says after close(2), even on error, the fd is closed and not
usable. The issue I see is a program thinking it failed and trying to
copy the file again or reporting an error.
On the other hand, as Vincent said, maybe this is not so bad. For
certain files, fsync(2) is only described for storage devices, so in
some cases there's no clear way to make sure close(2) won't fail after
EINTR (maybe calling sync(2)?). So, maybe considering it an error
wouldn't be a terrible idea.
I don't know.
Cheers,
Alex
>
> Hmmm, this page will need a kernel/libc differences section where I
> should explain this.
>
> On Fri, May 16, 2025 at 10:20:24AM -0400, Theodore Ts'o wrote:
> > On Fri, May 16, 2025 at 09:05:47AM -0400, Rich Felker wrote:
> >
> > > In general, raw kernel interfaces do not conform to any version of
> > > POSIX; they're just a low-impedance-mismatch set of inferfaces that
> > > facilitate implementing POSIX at the userspace libc layer. So I don't
> > > think this should be documented as "Linux doesn't conform" but
> > > (hopefully, once glibc fixes this) "old versions of glibc did not
> > > conform".
> >
> > If glibc maintainers want to deal with breaking userspace, then as a
> > kernel developer, I'm happy to let them deal with the
> > angry/disappointed users and application programmers. :-)
>
> Which breakage do you expect from the behavior that musl has chosen?
>
> I agree that the POSIX invention of EINPROGRESS is something that would
> break users. However, in removing the error completely and making it a
> success, I don't see the same problem. That is, if a program calls
> close(2) and sees a return of 0, or sees a return of -1 with EINTR on
> Linux, both mean "the file descriptor has been closed, and the contents
> of the file will *eventually* reach the file".
>
> In which cases do you expect any existing Linux program to behave
> differently on 0 and on EINTR?
>
>
> Have a lovely day!
> Alex
>
> --
> <https://www.alejandro-colomar.es/>
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/3] Wire up the lsm_manage_policy syscall
From: John Johansen @ 2025-05-17 7:59 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Song Liu, Maxime Bélair, linux-security-module, paul,
jmorris, serge, kees, stephen.smalley.work, casey, takedakn,
penguin-kernel, linux-api, apparmor, linux-kernel, Arnd Bergmann
In-Reply-To: <20250512.Uong6eCaVuwu@digikod.net>
On 5/12/25 03:20, Mickaël Salaün wrote:
> On Sun, May 11, 2025 at 03:47:21AM -0700, John Johansen wrote:
>> On 5/9/25 03:26, Mickaël Salaün wrote:
>>> On Thu, May 08, 2025 at 01:18:20AM -0700, John Johansen wrote:
>>>> On 5/7/25 23:06, Song Liu wrote:
>>>>> On Wed, May 7, 2025 at 8:37 AM Maxime Bélair
>>>>> <maxime.belair@canonical.com> wrote:
>>>>> [...]
>
>>>>> permission check to each pseudo file. The downside of the syscall, however,
>>>>> is that all the permission checks are hard-coded in the kernel (except for
>>>>
>>>> The permission checks don't have to be hard coded. Each LSM can define how it handles
>>>> or manages the syscall. The default is that it isn't supported, but if an lsm decides
>>>> to support it, there is now reason that its policy can't determine the use of the
>>>> syscall.
>>>
>>> From an interface design point of view, it would be better to clearly
>>> specify the scope of a command (e.g. which components could be impacted
>>> by a command), and make sure the documentation reflect that as well.
>>> Even better, have a syscalls per required privileges and impact (e.g.
>>> privileged or unprivileged). Going this road, I'm not sure if a
>>> privileged syscall would make sense given the existing filesystem
>>> interface.
>>>
>>
>> uhhhmmm, not just privileged. As you well know we are looking to use
>> this for unprivileged policy. The LSM can limit to privileged if it
>> wants but it doesn't have to limit it to privileged policy.
>
> Yes, I meant to say having a syscall for unprivileged actions, and maybe
> another one for privileged ones, but this might be a hard sell. :)
>
indeed, in the apparmor case context would be important. Just exactly
what is privileged. It may be a privileged operation to load policy to one
namespace, but not to another that you are setting up for a child.
> To say it another way, for your use case, do you need this syscall(s)
> for privileged operations? Do you plan to drop (or stop extending) the
need, probably. That is to say, loading of policy have varying levels
of privilege. root within the container has privilege to load policy
to its namespace, but it might have authority to setup a child namespace
that does not require privilege for it to load policy into, and it
will determine if the child has privilege or unprivleged policy within
it.
Ideally we won't have to use the fs interface within the "privileged"
container, as there are cases where this is currently not done or
undesirable.
> filesystem interface or do you think it would be good for (AppArmor)
> privileged operations too? I know syscalls might be attractive and
> could be used for everything, but it's good to have a well-defined plan
> and semantic to avoid using such syscall as another multiplexer with
> unrelated operations and required privileges.
>
sure. But the privilege level is use case dependent, to which policy
namespace is policy being loaded, replaced, ... The privilege level
very much will depend on what is in the stack/bounding of policy.
> If this syscall should also be a way to do privileged operations, should
> we also agree on a common set of permissions (e.g. global CAP_MAC_ADMIN
> or user namespace one)?
>
I think requiring something like CAP_MAC_ADMIN would be a per LSM
decision.
> [...]
>
>>>>> Overall, I am still not convinced a syscall for all LSMs is needed. To
>>>>> justify such
>>>>
>>>> its not needed by all LSMs, just a subset of them, and some nebulous
>>>> subset of potentially future LSMs that is entirely undefinable.
>>>>
>>>> If we had had appropriate LSM syscalls landlock wouldn't have needed
>>>> to have landlock specific syscalls. Having another LSM go that route
>>>> feels wrong especially now that we have some LSM syscalls.
>>>
>>> I don't agree. Dedicated syscalls are a good thing. See my other
>>> reply.
>>>
>>
>> I think we can just disagree on this point.
>>
>>>> If a
>>>> syscall is needed by an LSM its better to try hashing something out
>>>> that might have utility for multiple LSMs or at the very least,
>>>> potentially have utility in the future.
>>>>
>>>>
>>>>> a syscall, I think we need to show that it is useful in multiple LSMs.
>>>>> Also, if we
>>>>> really want to have single set of APIs for all LSMs, we may also need
>>>>> get_policy,
>>>>
>>>> We are never going to get a single set of APIs for all LSMs. I will
>>>> settle for an api that has utility for a subset
>>>>
>>>>> remove_policy, etc. This set as-is appears to be an incomplete design. The
>>>>
>>>> To have a complete design, there needs to be feedback and discussion
>>>> from multiple LSMs. This is a starting point.
>>>>
>>>>> implementation, with call_int_hook, is also problematic. It can easily
>>>>> cause some> controversial behaviors.
>>>>>
>>>> agreed it shouldn't be doing a straight call_int_hook, it should only
>>>> call it against the lsm identified by the lsmid
>>>
>>> Yes, but then, I don't see the point of a "generic" LSM syscall.
>>
>> its not a generic LSM syscall. Its a syscall or maybe a set of syscalls
>> for a specific scoped problem of loading/managing policy.
>>
>> Can we come to something acceptable? I don't know but we are going to
>> look at it before trying for an apparmor specific syscall.
>
> I understand and it's good to have this discussion.
^ permalink raw reply
* Re: [RFC v1] man/man2/close.2: CAVEATS: Document divergence from POSIX.1-2024
From: Alejandro Colomar @ 2025-05-17 5:46 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Rich Felker, Jan Kara, Alexander Viro, Christian Brauner,
linux-fsdevel, linux-api, libc-alpha
In-Reply-To: <20250516142024.GA21503@mit.edu>
[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]
Hi Ted, Rich,
On Fri, May 16, 2025 at 09:05:47AM -0400, Rich Felker wrote:
> FWIW musl adopted the EINPROGRESS as soon as we were made aware of the
> issue, and later changed it to returning 0 since applications
> (particularly, any written prior to this interpretation) are prone to
> interpret EINPROGRESS as an error condition rather than success and
> possibly misinterpret it as meaning the fd is still open and valid to
> pass to close again.
Hmmm, this page will need a kernel/libc differences section where I
should explain this.
On Fri, May 16, 2025 at 10:20:24AM -0400, Theodore Ts'o wrote:
> On Fri, May 16, 2025 at 09:05:47AM -0400, Rich Felker wrote:
>
> > In general, raw kernel interfaces do not conform to any version of
> > POSIX; they're just a low-impedance-mismatch set of inferfaces that
> > facilitate implementing POSIX at the userspace libc layer. So I don't
> > think this should be documented as "Linux doesn't conform" but
> > (hopefully, once glibc fixes this) "old versions of glibc did not
> > conform".
>
> If glibc maintainers want to deal with breaking userspace, then as a
> kernel developer, I'm happy to let them deal with the
> angry/disappointed users and application programmers. :-)
Which breakage do you expect from the behavior that musl has chosen?
I agree that the POSIX invention of EINPROGRESS is something that would
break users. However, in removing the error completely and making it a
success, I don't see the same problem. That is, if a program calls
close(2) and sees a return of 0, or sees a return of -1 with EINTR on
Linux, both mean "the file descriptor has been closed, and the contents
of the file will *eventually* reach the file".
In which cases do you expect any existing Linux program to behave
differently on 0 and on EINTR?
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/6] tcp: copy write-data from listen socket to accept child socket
From: Jeremy Harris @ 2025-05-16 20:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-api, ncardwell
In-Reply-To: <CANn89iLNEnF1YYwOmPsuQn6n-O9N9yVxnZ_djEGxaZ11i4AsVA@mail.gmail.com>
On 2025/05/16 6:51 PM, Eric Dumazet wrote:
> I do not see any locking. I think you should run a local KASAN/syzbot
> instance and you will be shocked.
Thanks for the suggestion; I'll look into doing that.
--
Cheers,
Jeremy
^ permalink raw reply
* Re: [PATCH 0/6] tcp: support preloading data on a listening socket
From: Jeremy Harris @ 2025-05-16 20:10 UTC (permalink / raw)
To: Neal Cardwell; +Cc: netdev, linux-api, edumazet
In-Reply-To: <CADVnQymxsOGLnUfurhDLXNUaK4gpaYm2zTDEWRxy8JPqH6O6vg@mail.gmail.com>
Hi Neal,
Thanks for the initial review.
On 2025/05/16 7:19 PM, Neal Cardwell wrote:
> On Fri, May 16, 2025 at 11:55 AM Jeremy Harris <jgh@exim.org> wrote:
>>
>> Support write to a listen TCP socket, for immediate
>> transmission on passive connection establishments.
>>
>> On a normal connection transmission is triggered by the receipt of
>> the 3rd-ack. On a fastopen (with accepted cookie) connection the data
>> is sent in the synack packet.
>>
>> The data preload is done using a sendmsg with a newly-defined flag
>> (MSG_PRELOAD); the amount of data limited to a single linear sk_buff.
>> Note that this definition is the last-but-two bit available if "int"
>> is 32 bits.
>
> Can you please add a bit more context, like:
>
> + What is the motivating use case? (Accelerating Exim?)
Accelerating any server-first ULP, SMTP being the major use I
know of (and yes, Exim is my primary testcase and is operational
against a test kernel with this patch series).
One caveat: the initial server data cannot change from one passive
connection to another.
> Is this
> targeted for connections using encryption (like TLS/SSL), or just
> plain-text connections?
TLS-on-connect cannot benefit, being client-first. SMTP that uses
STARTTLS can take advantage of it, as can plaintext SMTP.
I would not expect https to be able to use it.
> + What are the exact performance improvements you are seeing in your
> benchmarks that (a) motivate this, and (b) justify any performance
> impact on the TCP stack?
Because of the lack of userland roundtrip needed for the initial server
data, there is a latency benefit. This is better for the TFO-C case,
but also significant for the non-TFO case.
Packet capture (laptop, loopback, TFO-C case) for initial SYN to first
client data packet (5 samples):
- baseline TFO_C 1064 1470 1455 1547 1595 usec
- patched non-TFO 140 150 159 144 153 usec
- patched TFO_C 142 149 149 125 125 usec
One fewer packet is sent by the server in most packet captures, sometimes
one fewer in each direction. There is one less application kernel entry/exit
on the server.
I'm hoping those differences will add up to both less cpu time (on both
endpoints) and less wire-time. However, I have not run benchmarks looking
for a change in peak rate of connection-handling.
In summary, this is the mirror of TCP Fast Open client data: the latency
benefit is probably the most useful aspect.
> + Regarding "Support write to a listen TCP socket, for immediate
> transmission on passive connection establishments.": can you please
> make it explicitly clear whether the data written to the listening
> socket is saved and transmitted on all future successful passive
> sockets that are created for the listener,
This. The data is copied for each future passive socket from this
listener,
> or is just transmitted on
> the next connection that is created?
(and not this option).
I'll copy these comments in any future v2.
As Eric says, I should run KASAN/syzbot first.
--
Cheers,
Jeremy
^ permalink raw reply
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