Access to 'op' variable does not require pagefault_disable(),
'ret' variable should be initialized before using,
'oldval' variable can be replaced by constant.
Signed-off-by: Vasily Averin <[email protected]>
---
include/asm-generic/futex.h | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
index 8666fe7f35d7..e9a9655d786d 100644
--- a/include/asm-generic/futex.h
+++ b/include/asm-generic/futex.h
@@ -118,9 +118,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
static inline int
arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
{
- int oldval = 0, ret;
-
- pagefault_disable();
+ int ret = 0;
switch (op) {
case FUTEX_OP_SET:
@@ -132,10 +130,8 @@ arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
ret = -ENOSYS;
}
- pagefault_enable();
-
if (!ret)
- *oval = oldval;
+ *oval = 0;
return ret;
}
--
2.17.1
Looks like this code is dead and therefore looks strange.
I've found it during manual code review and decided to send patch
to pay your attention to this problem.
Probably it's better to remove this code at all?
On 7/15/19 1:27 PM, Vasily Averin wrote:
> Access to 'op' variable does not require pagefault_disable(),
> 'ret' variable should be initialized before using,
> 'oldval' variable can be replaced by constant.
>
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> include/asm-generic/futex.h | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
> index 8666fe7f35d7..e9a9655d786d 100644
> --- a/include/asm-generic/futex.h
> +++ b/include/asm-generic/futex.h
> @@ -118,9 +118,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> static inline int
> arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> {
> - int oldval = 0, ret;
> -
> - pagefault_disable();
> + int ret = 0;
>
> switch (op) {
> case FUTEX_OP_SET:
> @@ -132,10 +130,8 @@ arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> ret = -ENOSYS;
> }
>
> - pagefault_enable();
> -
> if (!ret)
> - *oval = oldval;
> + *oval = 0;
>
> return ret;
> }
>
On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <[email protected]> wrote:
>
> Looks like this code is dead and therefore looks strange.
> I've found it during manual code review and decided to send patch
> to pay your attention to this problem.
> Probably it's better to remove this code at all?
>
> On 7/15/19 1:27 PM, Vasily Averin wrote:
> > Access to 'op' variable does not require pagefault_disable(),
> > 'ret' variable should be initialized before using,
> > 'oldval' variable can be replaced by constant.
> >
> > Signed-off-by: Vasily Averin <[email protected]>
I'm not following the reasoning for any of the changes here. Why do you
think we don't need the pagefault_disable() around get_user()/put_user(),
and which part of the funtion is dead code?
Arnd
On Mon, 15 Jul 2019, Vasily Averin wrote:
> Access to 'op' variable does not require pagefault_disable(),
> 'ret' variable should be initialized before using,
> 'oldval' variable can be replaced by constant.
>
> Signed-off-by: Vasily Averin <[email protected]>
> ---
> include/asm-generic/futex.h | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
> index 8666fe7f35d7..e9a9655d786d 100644
> --- a/include/asm-generic/futex.h
> +++ b/include/asm-generic/futex.h
> @@ -118,9 +118,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> static inline int
> arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> {
> - int oldval = 0, ret;
> -
> - pagefault_disable();
> + int ret = 0;
The variable is clearly initialized before using in 'default:', but the
whole function is pretty useless. It's guaranteed to return -ENOSYS and
does nothing else than disabling/enabling pagefaults for no reason.
Thanks,
tglx
On Mon, 15 Jul 2019, Arnd Bergmann wrote:
> On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <[email protected]> wrote:
> >
> > Looks like this code is dead and therefore looks strange.
> > I've found it during manual code review and decided to send patch
> > to pay your attention to this problem.
> > Probably it's better to remove this code at all?
> >
> > On 7/15/19 1:27 PM, Vasily Averin wrote:
> > > Access to 'op' variable does not require pagefault_disable(),
> > > 'ret' variable should be initialized before using,
> > > 'oldval' variable can be replaced by constant.
> > >
> > > Signed-off-by: Vasily Averin <[email protected]>
>
> I'm not following the reasoning for any of the changes here. Why do you
> think we don't need the pagefault_disable() around get_user()/put_user(),
> and which part of the funtion is dead code?
All of it. If you change the function to
{
return -ENOSYS;
}
then it is equivalent (except for the pointless pagefault_disable/enable()
pair which protects absolutely nothing).
Thanks,
tglx
generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS
Signed-off-by: Vasily Averin <[email protected]>
---
include/asm-generic/futex.h | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
index 8666fe7f35d7..02970b11f71f 100644
--- a/include/asm-generic/futex.h
+++ b/include/asm-generic/futex.h
@@ -118,26 +118,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
static inline int
arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
{
- int oldval = 0, ret;
-
- pagefault_disable();
-
- switch (op) {
- case FUTEX_OP_SET:
- case FUTEX_OP_ADD:
- case FUTEX_OP_OR:
- case FUTEX_OP_ANDN:
- case FUTEX_OP_XOR:
- default:
- ret = -ENOSYS;
- }
-
- pagefault_enable();
-
- if (!ret)
- *oval = oldval;
-
- return ret;
+ return -ENOSYS;
}
static inline int
--
2.17.1
On Tue, Jul 16, 2019 at 8:22 AM Vasily Averin <[email protected]> wrote:
>
> generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS
>
> Signed-off-by: Vasily Averin <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
I see what you mean now, I got confused earlier when I looked at the
non-SMP version of the same function.
Commit-ID: f9adc23ee91e6f561bb70c6147d8d45bd164d62f
Gitweb: https://git.kernel.org/tip/f9adc23ee91e6f561bb70c6147d8d45bd164d62f
Author: Vasily Averin <[email protected]>
AuthorDate: Tue, 16 Jul 2019 09:22:03 +0300
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 22 Jul 2019 11:20:10 +0200
futex: Cleanup generic SMP variant of arch_futex_atomic_op_inuser()
The generic SMP variant of arch_futex_atomic_op_inuser() returns always
-ENOSYS so the switch case and surrounding code are pointless. Remove it
and just return -ENOSYS.
Signed-off-by: Vasily Averin <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
include/asm-generic/futex.h | 21 +--------------------
1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/include/asm-generic/futex.h b/include/asm-generic/futex.h
index 8666fe7f35d7..02970b11f71f 100644
--- a/include/asm-generic/futex.h
+++ b/include/asm-generic/futex.h
@@ -118,26 +118,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
static inline int
arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
{
- int oldval = 0, ret;
-
- pagefault_disable();
-
- switch (op) {
- case FUTEX_OP_SET:
- case FUTEX_OP_ADD:
- case FUTEX_OP_OR:
- case FUTEX_OP_ANDN:
- case FUTEX_OP_XOR:
- default:
- ret = -ENOSYS;
- }
-
- pagefault_enable();
-
- if (!ret)
- *oval = oldval;
-
- return ret;
+ return -ENOSYS;
}
static inline int