[PATCH] generic arch_futex_atomic_op_inuser() cleanup

[PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
 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

[PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

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 <vvs@virtuozzo.com>
---
 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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

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 <vvs@virtuozzo.com>
> ---
>  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;
>  }
> 

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

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 <vvs@virtuozzo.com>
> ---
>  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;
>  }
> 

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Arnd Bergmann]

On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <vvs@virtuozzo.com> 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 <vvs@virtuozzo.com>

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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Arnd Bergmann]

On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <vvs@virtuozzo.com> 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 <vvs@virtuozzo.com>

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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Thomas Gleixner]

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 <vvs@virtuozzo.com>
> ---
>  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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Thomas Gleixner]

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 <vvs@virtuozzo.com>
> ---
>  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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Thomas Gleixner]

On Mon, 15 Jul 2019, Arnd Bergmann wrote:

> On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <vvs@virtuozzo.com> 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 <vvs@virtuozzo.com>
> 
> 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

Re: [PATCH] generic arch_futex_atomic_op_inuser() cleanup

[Thomas Gleixner]

On Mon, 15 Jul 2019, Arnd Bergmann wrote:

> On Mon, Jul 15, 2019 at 12:29 PM Vasily Averin <vvs@virtuozzo.com> 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 <vvs@virtuozzo.com>
> 
> 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

[PATCH v2] futex: generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 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

[PATCH v2] futex: generic arch_futex_atomic_op_inuser() cleanup

[Vasily Averin]

generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS

Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 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

Re: [PATCH v2] futex: generic arch_futex_atomic_op_inuser() cleanup

[Arnd Bergmann]

On Tue, Jul 16, 2019 at 8:22 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS
>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

I see what you mean now, I got confused earlier when I looked at the
non-SMP version of the same function.

Re: [PATCH v2] futex: generic arch_futex_atomic_op_inuser() cleanup

[Arnd Bergmann]

On Tue, Jul 16, 2019 at 8:22 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> generic smp version of arch_futex_atomic_op_inuser() always returns -ENOSYS
>
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

I see what you mean now, I got confused earlier when I looked at the
non-SMP version of the same function.