Hello,
This series aims to make some small code improvements that I found in
futex.c, removing some lines and trying to make the code easier to read
and understand.
All commits tested with futex tests from kselftest.
Thanks,
André
André Almeida (4):
futex: Remove put_futex_key()
futex: Remove needless goto's
futex: Remove unused or redundant includes
futex: Consistently use fshared as boolean
kernel/futex.c | 114 +++++++++++--------------------------------------
1 file changed, 26 insertions(+), 88 deletions(-)
--
2.27.0
Since 4b39f99c ("futex: Remove {get,drop}_futex_key_refs()"),
function put_futex_key() is empty. Remove all references for this
function and redundant labels.
Signed-off-by: André Almeida <[email protected]>
---
kernel/futex.c | 61 ++++++++++----------------------------------------
1 file changed, 12 insertions(+), 49 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index e646661f6282..bd9adfca5d51 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -677,10 +677,6 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
return err;
}
-static inline void put_futex_key(union futex_key *key)
-{
-}
-
/**
* fault_in_user_writeable() - Fault in user address and verify RW access
* @uaddr: pointer to faulting user space address
@@ -1617,7 +1613,7 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
/* Make sure we really have tasks to wakeup */
if (!hb_waiters_pending(hb))
- goto out_put_key;
+ goto out;
spin_lock(&hb->lock);
@@ -1640,8 +1636,6 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
spin_unlock(&hb->lock);
wake_up_q(&wake_q);
-out_put_key:
- put_futex_key(&key);
out:
return ret;
}
@@ -1712,7 +1706,7 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
goto out;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, FUTEX_WRITE);
if (unlikely(ret != 0))
- goto out_put_key1;
+ goto out;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -1730,13 +1724,13 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
* an MMU, but we might get them from range checking
*/
ret = op_ret;
- goto out_put_keys;
+ goto out;
}
if (op_ret == -EFAULT) {
ret = fault_in_user_writeable(uaddr2);
if (ret)
- goto out_put_keys;
+ goto out;
}
if (!(flags & FLAGS_SHARED)) {
@@ -1744,8 +1738,6 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
goto retry_private;
}
- put_futex_key(&key2);
- put_futex_key(&key1);
cond_resched();
goto retry;
}
@@ -1781,10 +1773,6 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
-out_put_keys:
- put_futex_key(&key2);
-out_put_key1:
- put_futex_key(&key1);
out:
return ret;
}
@@ -1996,7 +1984,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2,
requeue_pi ? FUTEX_WRITE : FUTEX_READ);
if (unlikely(ret != 0))
- goto out_put_key1;
+ goto out;
/*
* The check above which compares uaddrs is not sufficient for
@@ -2004,7 +1992,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
*/
if (requeue_pi && match_futex(&key1, &key2)) {
ret = -EINVAL;
- goto out_put_keys;
+ goto out;
}
hb1 = hash_futex(&key1);
@@ -2025,13 +2013,11 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
ret = get_user(curval, uaddr1);
if (ret)
- goto out_put_keys;
+ goto out;
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&key2);
- put_futex_key(&key1);
goto retry;
}
if (curval != *cmpval) {
@@ -2090,8 +2076,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
case -EFAULT:
double_unlock_hb(hb1, hb2);
hb_waiters_dec(hb2);
- put_futex_key(&key2);
- put_futex_key(&key1);
ret = fault_in_user_writeable(uaddr2);
if (!ret)
goto retry;
@@ -2106,8 +2090,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
*/
double_unlock_hb(hb1, hb2);
hb_waiters_dec(hb2);
- put_futex_key(&key2);
- put_futex_key(&key1);
/*
* Handle the case where the owner is in the middle of
* exiting. Wait for the exit to complete otherwise
@@ -2217,10 +2199,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
wake_up_q(&wake_q);
hb_waiters_dec(hb2);
-out_put_keys:
- put_futex_key(&key2);
-out_put_key1:
- put_futex_key(&key1);
out:
return ret ? ret : task_count;
}
@@ -2697,7 +2675,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&q->key);
goto retry;
}
@@ -2707,8 +2684,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
}
out:
- if (ret)
- put_futex_key(&q->key);
return ret;
}
@@ -2853,7 +2828,6 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags,
* - EAGAIN: The user space value changed.
*/
queue_unlock(hb);
- put_futex_key(&q.key);
/*
* Handle the case where the owner is in the middle of
* exiting. Wait for the exit to complete otherwise
@@ -2961,13 +2935,11 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags,
put_pi_state(pi_state);
}
- goto out_put_key;
+ goto out;
out_unlock_put_key:
queue_unlock(hb);
-out_put_key:
- put_futex_key(&q.key);
out:
if (to) {
hrtimer_cancel(&to->timer);
@@ -2980,12 +2952,11 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags,
ret = fault_in_user_writeable(uaddr);
if (ret)
- goto out_put_key;
+ goto out;
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&q.key);
goto retry;
}
@@ -3114,16 +3085,13 @@ static int futex_unlock_pi(u32 __user *uaddr, unsigned int flags)
out_unlock:
spin_unlock(&hb->lock);
out_putkey:
- put_futex_key(&key);
return ret;
pi_retry:
- put_futex_key(&key);
cond_resched();
goto retry;
pi_faulted:
- put_futex_key(&key);
ret = fault_in_user_writeable(uaddr);
if (!ret)
@@ -3265,7 +3233,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
*/
ret = futex_wait_setup(uaddr, val, flags, &q, &hb);
if (ret)
- goto out_key2;
+ goto out;
/*
* The check above which compares uaddrs is not sufficient for
@@ -3274,7 +3242,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
if (match_futex(&q.key, &key2)) {
queue_unlock(hb);
ret = -EINVAL;
- goto out_put_keys;
+ goto out;
}
/* Queue the futex_q, drop the hb lock, wait for wakeup. */
@@ -3284,7 +3252,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
ret = handle_early_requeue_pi_wakeup(hb, &q, &key2, to);
spin_unlock(&hb->lock);
if (ret)
- goto out_put_keys;
+ goto out;
/*
* In order for us to be here, we know our q.key == key2, and since
@@ -3374,11 +3342,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
ret = -EWOULDBLOCK;
}
-out_put_keys:
- put_futex_key(&q.key);
-out_key2:
- put_futex_key(&key2);
-
out:
if (to) {
hrtimer_cancel(&to->timer);
--
2.27.0
As stated in the coding style documentation[1], "if there is no cleanup
needed then just return directly", instead of jumping to a label and
then returning. Remove such goto's and replace with a return statement.
When there's a ternary operator on the return value, replace with the
result of the operation when is logically possible to determine it by
the control flow.
[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
Signed-off-by: André Almeida <[email protected]>
---
kernel/futex.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index bd9adfca5d51..362fbca6d614 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1607,13 +1607,13 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
hb = hash_futex(&key);
/* Make sure we really have tasks to wakeup */
if (!hb_waiters_pending(hb))
- goto out;
+ return ret;
spin_lock(&hb->lock);
@@ -1636,7 +1636,6 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
spin_unlock(&hb->lock);
wake_up_q(&wake_q);
-out:
return ret;
}
@@ -1703,10 +1702,10 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
retry:
ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, FUTEX_WRITE);
if (unlikely(ret != 0))
- goto out;
+ return ret;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -1724,13 +1723,13 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
* an MMU, but we might get them from range checking
*/
ret = op_ret;
- goto out;
+ return ret;
}
if (op_ret == -EFAULT) {
ret = fault_in_user_writeable(uaddr2);
if (ret)
- goto out;
+ return ret;
}
if (!(flags & FLAGS_SHARED)) {
@@ -1773,7 +1772,6 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
-out:
return ret;
}
@@ -1980,20 +1978,18 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
retry:
ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2,
requeue_pi ? FUTEX_WRITE : FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
/*
* The check above which compares uaddrs is not sufficient for
* shared futexes. We need to compare the keys:
*/
- if (requeue_pi && match_futex(&key1, &key2)) {
- ret = -EINVAL;
- goto out;
- }
+ if (requeue_pi && match_futex(&key1, &key2))
+ return -EINVAL;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -2013,7 +2009,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
ret = get_user(curval, uaddr1);
if (ret)
- goto out;
+ return ret;
if (!(flags & FLAGS_SHARED))
goto retry_private;
@@ -2079,7 +2075,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
ret = fault_in_user_writeable(uaddr2);
if (!ret)
goto retry;
- goto out;
+ return ret;
case -EBUSY:
case -EAGAIN:
/*
@@ -2198,8 +2194,6 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
hb_waiters_dec(hb2);
-
-out:
return ret ? ret : task_count;
}
@@ -2545,7 +2539,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
*/
if (q->pi_state->owner != current)
ret = fixup_pi_state_owner(uaddr, q, current);
- goto out;
+ return ret ? ret : locked;
}
/*
@@ -2558,7 +2552,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
*/
if (q->pi_state->owner == current) {
ret = fixup_pi_state_owner(uaddr, q, NULL);
- goto out;
+ return ret;
}
/*
@@ -2572,8 +2566,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
q->pi_state->owner);
}
-out:
- return ret ? ret : locked;
+ return ret;
}
/**
@@ -2670,7 +2663,7 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
ret = get_user(uval, uaddr);
if (ret)
- goto out;
+ return ret;
if (!(flags & FLAGS_SHARED))
goto retry_private;
@@ -2683,7 +2676,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
ret = -EWOULDBLOCK;
}
-out:
return ret;
}
--
2.27.0
Since 82af7aca ("Removal of FUTEX_FD"), some includes related to file
operations aren't needed anymore. More investigation around the includes
showed that a lot of includes aren't required for compilation, possible
due to redundant includes. Simplify the code by removing unused
includes.
Signed-off-by: André Almeida <[email protected]>
---
To test this code, I compiled with different configurations (x86_64,
i386, with x32 ABI supported enabled/disabled), and ran futex selftests.
---
kernel/futex.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 362fbca6d614..697835ad5bff 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -31,31 +31,13 @@
* "The futexes are also cursed."
* "But they come in a choice of three flavours!"
*/
-#include <linux/compat.h>
-#include <linux/slab.h>
-#include <linux/poll.h>
-#include <linux/fs.h>
-#include <linux/file.h>
#include <linux/jhash.h>
-#include <linux/init.h>
-#include <linux/futex.h>
-#include <linux/mount.h>
#include <linux/pagemap.h>
#include <linux/syscalls.h>
-#include <linux/signal.h>
-#include <linux/export.h>
-#include <linux/magic.h>
-#include <linux/pid.h>
-#include <linux/nsproxy.h>
-#include <linux/ptrace.h>
-#include <linux/sched/rt.h>
-#include <linux/sched/wake_q.h>
-#include <linux/sched/mm.h>
#include <linux/hugetlb.h>
#include <linux/freezer.h>
#include <linux/memblock.h>
#include <linux/fault-inject.h>
-#include <linux/refcount.h>
#include <asm/futex.h>
--
2.27.0
Since fshared is meant to true/false values, declare it as bool. If the
code ever reaches the code beneath again label, we are sure that the
futex is shared, so we can use the true value instead of the variable.
Signed-off-by: André Almeida <[email protected]>
---
kernel/futex.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 697835ad5bff..d56c9310d734 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -458,7 +458,7 @@ static u64 get_inode_sequence_number(struct inode *inode)
/**
* get_futex_key() - Get parameters which are the keys for a futex
* @uaddr: virtual address of the futex
- * @fshared: 0 for a PROCESS_PRIVATE futex, 1 for PROCESS_SHARED
+ * @fshared: false for a PROCESS_PRIVATE futex, true for PROCESS_SHARED
* @key: address where result is stored.
* @rw: mapping needs to be read/write (values: FUTEX_READ,
* FUTEX_WRITE)
@@ -483,7 +483,8 @@ static u64 get_inode_sequence_number(struct inode *inode)
* lock_page() might sleep, the caller should not hold a spinlock.
*/
static int
-get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_access rw)
+get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key,
+ enum futex_access rw)
{
unsigned long address = (unsigned long)uaddr;
struct mm_struct *mm = current->mm;
@@ -520,7 +521,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
again:
/* Ignore any VERIFY_READ mapping (futex common case) */
- if (unlikely(should_fail_futex(fshared)))
+ if (unlikely(should_fail_futex(true)))
return -EFAULT;
err = get_user_pages_fast(address, 1, FOLL_WRITE, &page);
@@ -608,7 +609,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
* A RO anonymous page will never change and thus doesn't make
* sense for futex operations.
*/
- if (unlikely(should_fail_futex(fshared)) || ro) {
+ if (unlikely(should_fail_futex(true)) || ro) {
err = -EFAULT;
goto out;
}
--
2.27.0
André Almeida <[email protected]> writes:
> Since 82af7aca ("Removal of FUTEX_FD"), some includes related to file
> operations aren't needed anymore. More investigation around the includes
> showed that a lot of includes aren't required for compilation, possible
> due to redundant includes. Simplify the code by removing unused
> includes.
>
> Signed-off-by: André Almeida <[email protected]>
> ---
> To test this code, I compiled with different configurations (x86_64,
> i386, with x32 ABI supported enabled/disabled), and ran futex
> selftests.
I agree fully with the FD related ones, but are you sure that all of the
others are included on all architectures magically? x86 is just one
piece of the puzzle. We'll see ...
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 2f3cc106873cb8234300c381a96b0b0e4ba63ab9
Gitweb: https://git.kernel.org/tip/2f3cc106873cb8234300c381a96b0b0e4ba63ab9
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:43 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 17 Jul 2020 23:58:50 +02:00
futex: Consistently use fshared as boolean
Since fshared is only conveying true/false values, declare it as bool.
In get_futex_key() the usage of fshared can be restricted to the first part
of the function. If fshared is false the function is terminated early and
the subsequent code can use a constant 'true' instead of the variable.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 697835a..f483bc5 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -458,7 +458,7 @@ static u64 get_inode_sequence_number(struct inode *inode)
/**
* get_futex_key() - Get parameters which are the keys for a futex
* @uaddr: virtual address of the futex
- * @fshared: 0 for a PROCESS_PRIVATE futex, 1 for PROCESS_SHARED
+ * @fshared: false for a PROCESS_PRIVATE futex, true for PROCESS_SHARED
* @key: address where result is stored.
* @rw: mapping needs to be read/write (values: FUTEX_READ,
* FUTEX_WRITE)
@@ -482,8 +482,8 @@ static u64 get_inode_sequence_number(struct inode *inode)
*
* lock_page() might sleep, the caller should not hold a spinlock.
*/
-static int
-get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_access rw)
+static int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key,
+ enum futex_access rw)
{
unsigned long address = (unsigned long)uaddr;
struct mm_struct *mm = current->mm;
@@ -520,7 +520,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
again:
/* Ignore any VERIFY_READ mapping (futex common case) */
- if (unlikely(should_fail_futex(fshared)))
+ if (unlikely(should_fail_futex(true)))
return -EFAULT;
err = get_user_pages_fast(address, 1, FOLL_WRITE, &page);
@@ -608,7 +608,7 @@ again:
* A RO anonymous page will never change and thus doesn't make
* sense for futex operations.
*/
- if (unlikely(should_fail_futex(fshared)) || ro) {
+ if (unlikely(should_fail_futex(true)) || ro) {
err = -EFAULT;
goto out;
}
The following commit has been merged into the locking/core branch of tip:
Commit-ID: d7c5ed73b19c4640426d9c106f70ec2cb532034d
Gitweb: https://git.kernel.org/tip/d7c5ed73b19c4640426d9c106f70ec2cb532034d
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:41 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 17 Jul 2020 23:58:49 +02:00
futex: Remove needless goto's
As stated in the coding style documentation, "if there is no cleanup
needed then just return directly", instead of jumping to a label and
then returning.
Remove such goto's and replace with a return statement. When there's a
ternary operator on the return value, replace it with the result of the
operation when it is logically possible to determine it by the control
flow.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 40 ++++++++++++++++------------------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index bd9adfc..362fbca 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1607,13 +1607,13 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &key, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
hb = hash_futex(&key);
/* Make sure we really have tasks to wakeup */
if (!hb_waiters_pending(hb))
- goto out;
+ return ret;
spin_lock(&hb->lock);
@@ -1636,7 +1636,6 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
spin_unlock(&hb->lock);
wake_up_q(&wake_q);
-out:
return ret;
}
@@ -1703,10 +1702,10 @@ futex_wake_op(u32 __user *uaddr1, unsigned int flags, u32 __user *uaddr2,
retry:
ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, FUTEX_WRITE);
if (unlikely(ret != 0))
- goto out;
+ return ret;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -1724,13 +1723,13 @@ retry_private:
* an MMU, but we might get them from range checking
*/
ret = op_ret;
- goto out;
+ return ret;
}
if (op_ret == -EFAULT) {
ret = fault_in_user_writeable(uaddr2);
if (ret)
- goto out;
+ return ret;
}
if (!(flags & FLAGS_SHARED)) {
@@ -1773,7 +1772,6 @@ retry_private:
out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
-out:
return ret;
}
@@ -1980,20 +1978,18 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
retry:
ret = get_futex_key(uaddr1, flags & FLAGS_SHARED, &key1, FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2,
requeue_pi ? FUTEX_WRITE : FUTEX_READ);
if (unlikely(ret != 0))
- goto out;
+ return ret;
/*
* The check above which compares uaddrs is not sufficient for
* shared futexes. We need to compare the keys:
*/
- if (requeue_pi && match_futex(&key1, &key2)) {
- ret = -EINVAL;
- goto out;
- }
+ if (requeue_pi && match_futex(&key1, &key2))
+ return -EINVAL;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -2013,7 +2009,7 @@ retry_private:
ret = get_user(curval, uaddr1);
if (ret)
- goto out;
+ return ret;
if (!(flags & FLAGS_SHARED))
goto retry_private;
@@ -2079,7 +2075,7 @@ retry_private:
ret = fault_in_user_writeable(uaddr2);
if (!ret)
goto retry;
- goto out;
+ return ret;
case -EBUSY:
case -EAGAIN:
/*
@@ -2198,8 +2194,6 @@ out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
hb_waiters_dec(hb2);
-
-out:
return ret ? ret : task_count;
}
@@ -2545,7 +2539,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
*/
if (q->pi_state->owner != current)
ret = fixup_pi_state_owner(uaddr, q, current);
- goto out;
+ return ret ? ret : locked;
}
/*
@@ -2558,7 +2552,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
*/
if (q->pi_state->owner == current) {
ret = fixup_pi_state_owner(uaddr, q, NULL);
- goto out;
+ return ret;
}
/*
@@ -2572,8 +2566,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
q->pi_state->owner);
}
-out:
- return ret ? ret : locked;
+ return ret;
}
/**
@@ -2670,7 +2663,7 @@ retry_private:
ret = get_user(uval, uaddr);
if (ret)
- goto out;
+ return ret;
if (!(flags & FLAGS_SHARED))
goto retry_private;
@@ -2683,7 +2676,6 @@ retry_private:
ret = -EWOULDBLOCK;
}
-out:
return ret;
}
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 9180bd467f9abdb44afde650d07e3b9dd66d837c
Gitweb: https://git.kernel.org/tip/9180bd467f9abdb44afde650d07e3b9dd66d837c
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:40 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 17 Jul 2020 23:58:49 +02:00
futex: Remove put_futex_key()
Since 4b39f99c ("futex: Remove {get,drop}_futex_key_refs()"),
put_futex_key() is empty.
Remove all references for this function and the then redundant labels.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 61 +++++++++----------------------------------------
1 file changed, 12 insertions(+), 49 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index e646661..bd9adfc 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -677,10 +677,6 @@ out:
return err;
}
-static inline void put_futex_key(union futex_key *key)
-{
-}
-
/**
* fault_in_user_writeable() - Fault in user address and verify RW access
* @uaddr: pointer to faulting user space address
@@ -1617,7 +1613,7 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
/* Make sure we really have tasks to wakeup */
if (!hb_waiters_pending(hb))
- goto out_put_key;
+ goto out;
spin_lock(&hb->lock);
@@ -1640,8 +1636,6 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
spin_unlock(&hb->lock);
wake_up_q(&wake_q);
-out_put_key:
- put_futex_key(&key);
out:
return ret;
}
@@ -1712,7 +1706,7 @@ retry:
goto out;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2, FUTEX_WRITE);
if (unlikely(ret != 0))
- goto out_put_key1;
+ goto out;
hb1 = hash_futex(&key1);
hb2 = hash_futex(&key2);
@@ -1730,13 +1724,13 @@ retry_private:
* an MMU, but we might get them from range checking
*/
ret = op_ret;
- goto out_put_keys;
+ goto out;
}
if (op_ret == -EFAULT) {
ret = fault_in_user_writeable(uaddr2);
if (ret)
- goto out_put_keys;
+ goto out;
}
if (!(flags & FLAGS_SHARED)) {
@@ -1744,8 +1738,6 @@ retry_private:
goto retry_private;
}
- put_futex_key(&key2);
- put_futex_key(&key1);
cond_resched();
goto retry;
}
@@ -1781,10 +1773,6 @@ retry_private:
out_unlock:
double_unlock_hb(hb1, hb2);
wake_up_q(&wake_q);
-out_put_keys:
- put_futex_key(&key2);
-out_put_key1:
- put_futex_key(&key1);
out:
return ret;
}
@@ -1996,7 +1984,7 @@ retry:
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2,
requeue_pi ? FUTEX_WRITE : FUTEX_READ);
if (unlikely(ret != 0))
- goto out_put_key1;
+ goto out;
/*
* The check above which compares uaddrs is not sufficient for
@@ -2004,7 +1992,7 @@ retry:
*/
if (requeue_pi && match_futex(&key1, &key2)) {
ret = -EINVAL;
- goto out_put_keys;
+ goto out;
}
hb1 = hash_futex(&key1);
@@ -2025,13 +2013,11 @@ retry_private:
ret = get_user(curval, uaddr1);
if (ret)
- goto out_put_keys;
+ goto out;
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&key2);
- put_futex_key(&key1);
goto retry;
}
if (curval != *cmpval) {
@@ -2090,8 +2076,6 @@ retry_private:
case -EFAULT:
double_unlock_hb(hb1, hb2);
hb_waiters_dec(hb2);
- put_futex_key(&key2);
- put_futex_key(&key1);
ret = fault_in_user_writeable(uaddr2);
if (!ret)
goto retry;
@@ -2106,8 +2090,6 @@ retry_private:
*/
double_unlock_hb(hb1, hb2);
hb_waiters_dec(hb2);
- put_futex_key(&key2);
- put_futex_key(&key1);
/*
* Handle the case where the owner is in the middle of
* exiting. Wait for the exit to complete otherwise
@@ -2217,10 +2199,6 @@ out_unlock:
wake_up_q(&wake_q);
hb_waiters_dec(hb2);
-out_put_keys:
- put_futex_key(&key2);
-out_put_key1:
- put_futex_key(&key1);
out:
return ret ? ret : task_count;
}
@@ -2697,7 +2675,6 @@ retry_private:
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&q->key);
goto retry;
}
@@ -2707,8 +2684,6 @@ retry_private:
}
out:
- if (ret)
- put_futex_key(&q->key);
return ret;
}
@@ -2853,7 +2828,6 @@ retry_private:
* - EAGAIN: The user space value changed.
*/
queue_unlock(hb);
- put_futex_key(&q.key);
/*
* Handle the case where the owner is in the middle of
* exiting. Wait for the exit to complete otherwise
@@ -2961,13 +2935,11 @@ no_block:
put_pi_state(pi_state);
}
- goto out_put_key;
+ goto out;
out_unlock_put_key:
queue_unlock(hb);
-out_put_key:
- put_futex_key(&q.key);
out:
if (to) {
hrtimer_cancel(&to->timer);
@@ -2980,12 +2952,11 @@ uaddr_faulted:
ret = fault_in_user_writeable(uaddr);
if (ret)
- goto out_put_key;
+ goto out;
if (!(flags & FLAGS_SHARED))
goto retry_private;
- put_futex_key(&q.key);
goto retry;
}
@@ -3114,16 +3085,13 @@ retry:
out_unlock:
spin_unlock(&hb->lock);
out_putkey:
- put_futex_key(&key);
return ret;
pi_retry:
- put_futex_key(&key);
cond_resched();
goto retry;
pi_faulted:
- put_futex_key(&key);
ret = fault_in_user_writeable(uaddr);
if (!ret)
@@ -3265,7 +3233,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
*/
ret = futex_wait_setup(uaddr, val, flags, &q, &hb);
if (ret)
- goto out_key2;
+ goto out;
/*
* The check above which compares uaddrs is not sufficient for
@@ -3274,7 +3242,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
if (match_futex(&q.key, &key2)) {
queue_unlock(hb);
ret = -EINVAL;
- goto out_put_keys;
+ goto out;
}
/* Queue the futex_q, drop the hb lock, wait for wakeup. */
@@ -3284,7 +3252,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
ret = handle_early_requeue_pi_wakeup(hb, &q, &key2, to);
spin_unlock(&hb->lock);
if (ret)
- goto out_put_keys;
+ goto out;
/*
* In order for us to be here, we know our q.key == key2, and since
@@ -3374,11 +3342,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
ret = -EWOULDBLOCK;
}
-out_put_keys:
- put_futex_key(&q.key);
-out_key2:
- put_futex_key(&key2);
-
out:
if (to) {
hrtimer_cancel(&to->timer);
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 38aa3c15b3a422e117a1f687a6dbcb5afd92cfc2
Gitweb: https://git.kernel.org/tip/38aa3c15b3a422e117a1f687a6dbcb5afd92cfc2
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:42 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Fri, 17 Jul 2020 23:58:50 +02:00
futex: Remove unused or redundant includes
Since 82af7aca ("Removal of FUTEX_FD"), some includes related to file
operations aren't needed anymore. More investigation around the includes
showed that a lot of includes aren't required for compilation, possible
due to redundant includes. Simplify the code by removing unused
includes.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 362fbca..697835a 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -31,31 +31,13 @@
* "The futexes are also cursed."
* "But they come in a choice of three flavours!"
*/
-#include <linux/compat.h>
-#include <linux/slab.h>
-#include <linux/poll.h>
-#include <linux/fs.h>
-#include <linux/file.h>
#include <linux/jhash.h>
-#include <linux/init.h>
-#include <linux/futex.h>
-#include <linux/mount.h>
#include <linux/pagemap.h>
#include <linux/syscalls.h>
-#include <linux/signal.h>
-#include <linux/export.h>
-#include <linux/magic.h>
-#include <linux/pid.h>
-#include <linux/nsproxy.h>
-#include <linux/ptrace.h>
-#include <linux/sched/rt.h>
-#include <linux/sched/wake_q.h>
-#include <linux/sched/mm.h>
#include <linux/hugetlb.h>
#include <linux/freezer.h>
#include <linux/memblock.h>
#include <linux/fault-inject.h>
-#include <linux/refcount.h>
#include <asm/futex.h>
On 7/17/20 6:46 PM, Thomas Gleixner wrote:
> André Almeida <[email protected]> writes:
>
>> Since 82af7aca ("Removal of FUTEX_FD"), some includes related to file
>> operations aren't needed anymore. More investigation around the includes
>> showed that a lot of includes aren't required for compilation, possible
>> due to redundant includes. Simplify the code by removing unused
>> includes.
>>
>> Signed-off-by: André Almeida <[email protected]>
>> ---
>> To test this code, I compiled with different configurations (x86_64,
>> i386, with x32 ABI supported enabled/disabled), and ran futex
>> selftests.
>
> I agree fully with the FD related ones, but are you sure that all of the
> others are included on all architectures magically? x86 is just one
> piece of the puzzle. We'll see ...
>
I just retested compiling tip/locking/core, and it seems something
changed the last time I wrote the patch. Removing <linux/compat.h> will
indeed break the compilation now, the patch bellow should fix it. Sorry
for the noise.
-- >8 --
Subject: [PATCH 1/1] futex: Add compat include
Commit 38aa3c15b3a4 removed includes that seemed to be unused.
However, removing <linux/compat.h> makes the compilation fail if the
build configuration supports compatibility ABIs. Fix that by
reinserting the include.
Fixes: 38aa3c15b3a4 ("futex: Remove unused or redundant includes")
Signed-off-by: André Almeida <[email protected]>
---
kernel/futex.c | 1 +
1 file changed, 1 insertion(+)
=======================================
diff --git a/kernel/futex.c b/kernel/futex.c
index f483bc52dbac..4616d4ad609d 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -31,6 +31,7 @@
31 * "The futexes are also cursed."
32 * "But they come in a choice of three flavours!"
33 */
+34#include <linux/compat.h>
35#include <linux/jhash.h>
36#include <linux/pagemap.h>
37#include <linux/syscalls.h>
--
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 9261308598ad28b9a8a2237d881833e9f217244e
Gitweb: https://git.kernel.org/tip/9261308598ad28b9a8a2237d881833e9f217244e
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:43 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Sat, 18 Jul 2020 01:56:08 +02:00
futex: Consistently use fshared as boolean
Since fshared is only conveying true/false values, declare it as bool.
In get_futex_key() the usage of fshared can be restricted to the first part
of the function. If fshared is false the function is terminated early and
the subsequent code can use a constant 'true' instead of the variable.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index 362fbca..cda9175 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -476,7 +476,7 @@ static u64 get_inode_sequence_number(struct inode *inode)
/**
* get_futex_key() - Get parameters which are the keys for a futex
* @uaddr: virtual address of the futex
- * @fshared: 0 for a PROCESS_PRIVATE futex, 1 for PROCESS_SHARED
+ * @fshared: false for a PROCESS_PRIVATE futex, true for PROCESS_SHARED
* @key: address where result is stored.
* @rw: mapping needs to be read/write (values: FUTEX_READ,
* FUTEX_WRITE)
@@ -500,8 +500,8 @@ static u64 get_inode_sequence_number(struct inode *inode)
*
* lock_page() might sleep, the caller should not hold a spinlock.
*/
-static int
-get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_access rw)
+static int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key,
+ enum futex_access rw)
{
unsigned long address = (unsigned long)uaddr;
struct mm_struct *mm = current->mm;
@@ -538,7 +538,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union futex_key *key, enum futex_a
again:
/* Ignore any VERIFY_READ mapping (futex common case) */
- if (unlikely(should_fail_futex(fshared)))
+ if (unlikely(should_fail_futex(true)))
return -EFAULT;
err = get_user_pages_fast(address, 1, FOLL_WRITE, &page);
@@ -626,7 +626,7 @@ again:
* A RO anonymous page will never change and thus doesn't make
* sense for futex operations.
*/
- if (unlikely(should_fail_futex(fshared)) || ro) {
+ if (unlikely(should_fail_futex(true)) || ro) {
err = -EFAULT;
goto out;
}
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 9a71df495c3d29dab596bb590e73fd8b20106e2d
Gitweb: https://git.kernel.org/tip/9a71df495c3d29dab596bb590e73fd8b20106e2d
Author: André Almeida <[email protected]>
AuthorDate: Thu, 02 Jul 2020 17:28:42 -03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Sat, 18 Jul 2020 01:56:09 +02:00
futex: Remove unused or redundant includes
Since 82af7aca ("Removal of FUTEX_FD"), some includes related to file
operations aren't needed anymore. More investigation around the includes
showed that a lot of includes aren't required for compilation, possible
due to redundant includes. Simplify the code by removing unused
includes.
Signed-off-by: André Almeida <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/futex.c | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index cda9175..4616d4a 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -32,30 +32,13 @@
* "But they come in a choice of three flavours!"
*/
#include <linux/compat.h>
-#include <linux/slab.h>
-#include <linux/poll.h>
-#include <linux/fs.h>
-#include <linux/file.h>
#include <linux/jhash.h>
-#include <linux/init.h>
-#include <linux/futex.h>
-#include <linux/mount.h>
#include <linux/pagemap.h>
#include <linux/syscalls.h>
-#include <linux/signal.h>
-#include <linux/export.h>
-#include <linux/magic.h>
-#include <linux/pid.h>
-#include <linux/nsproxy.h>
-#include <linux/ptrace.h>
-#include <linux/sched/rt.h>
-#include <linux/sched/wake_q.h>
-#include <linux/sched/mm.h>
#include <linux/hugetlb.h>
#include <linux/freezer.h>
#include <linux/memblock.h>
#include <linux/fault-inject.h>
-#include <linux/refcount.h>
#include <asm/futex.h>
André Almeida <[email protected]> writes:
> I just retested compiling tip/locking/core, and it seems something
> changed the last time I wrote the patch. Removing <linux/compat.h> will
> indeed break the compilation now, the patch bellow should fix it. Sorry
> for the noise.
I just folded it back manually and force pushed the result. Manually
because the below:
> =======================================
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index f483bc52dbac..4616d4ad609d 100644
>
> --- a/kernel/futex.c
>
> +++ b/kernel/futex.c
>
> @@ -31,6 +31,7 @@
>
> 31 * "The futexes are also cursed."
> 32 * "But they come in a choice of three flavours!"
> 33 */
> +34#include <linux/compat.h>
> 35#include <linux/jhash.h>
> 36#include <linux/pagemap.h>
> 37#include <linux/syscalls.h>
does not really qualify as a valid patch.