[PATCH 0/3] sched: sched_setattr/sched_getattr fixes and improvements

[PATCH 0/3] sched: sched_setattr/sched_getattr fixes and improvements

[Michael Kerrisk (man-pages)]

Peter,

While looking at your sched_setattr()/sched_getatr() documentation, 
I noticed a bug in the handling of -EFBIG, and a couple of places 
in the code where the error handling logic could be streamlined.
Patches follow.

Cheers,

Michael
-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[PATCH 1/3] sched: Make sched_setattr() correctly return -EFBIG

[Michael Kerrisk (man-pages)]

From: Michael Kerrisk <mtk.manpages@gmail.com>

The documented[1] behavior of sched_attr() in your proposed man page text is:

    sched_attr::size must be set to the size of the structure, as in
    sizeof(struct sched_attr), if the provided structure is smaller
    than the kernel structure, any additional fields are assumed
    '0'. If the provided structure is larger than the kernel structure,
    the kernel verifies all additional fields are '0' if not the
    syscall will fail with -E2BIG.

As currently implemented, sched_copy_attr() returns -EFBIG for
for this case, but the logic in sys_sched_setattr() converts that
error to -EFAULT. This patch fixes the behavior.

[1] http://thread.gmane.org/gmane.linux.kernel/1615615/focus=1697760

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
---
 kernel/sched/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 268a45e..6c9ce28 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3650,8 +3650,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	if (!uattr || pid < 0 || flags)
 		return -EINVAL;
 
-	if (sched_copy_attr(uattr, &attr))
-		return -EFAULT;
+	retval = sched_copy_attr(uattr, &attr);
+	if (retval)
+		return retval;
 
 	rcu_read_lock();
 	retval = -ESRCH;
-- 
1.9.0

Re: [PATCH 2/3] sched: Simplify return logic in sched_copy_attr()

[Michael Kerrisk (man-pages)]

From: Michael Kerrisk <mtk.manpages@gmail.com>

The logic in this function is a little contorted:
* Rather than having chained gotos for the -EFBIG case, just
  return -EFBIG directly.
* Now, the label 'out' is no longer needed, and 'ret' must be zero
  zero by the time we fall through to this point, so just return 0.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
---
 kernel/sched/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 6c9ce28..8fa3e12 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3596,13 +3596,11 @@ static int sched_copy_attr(struct sched_attr __user *uattr,
 	 */
 	attr->sched_nice = clamp(attr->sched_nice, MIN_NICE, MAX_NICE);
 
-out:
-	return ret;
+	return 0;
 
 err_size:
 	put_user(sizeof(*attr), &uattr->size);
-	ret = -E2BIG;
-	goto out;
+	return -E2BIG;
 }
 
 /**
-- 
1.9.0

[PATCH 3/3] sched: Simplify return logic in sched_read_attr()

[Michael Kerrisk (man-pages)]

From: Michael Kerrisk <mtk.manpages@gmail.com>

Gotos are chained pointlessly here, and the 'out' label
can be dispensed with.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
---
 kernel/sched/core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8fa3e12..d21c070 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3759,7 +3759,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 
 		for (; addr < end; addr++) {
 			if (*addr)
-				goto err_size;
+				return -EFBIG;
 		}
 
 		attr->size = usize;
@@ -3769,12 +3769,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 	if (ret)
 		return -EFAULT;
 
-out:
-	return ret;
-
-err_size:
-	ret = -E2BIG;
-	goto out;
+	return 0;
 }
 
 /**
-- 
1.9.0

Re: [PATCH 0/3] sched: sched_setattr/sched_getattr fixes and improvements

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Fri, May 09, 2014 at 04:49:18PM +0200, Michael Kerrisk (man-pages) wrote:
> Peter,
> 
> While looking at your sched_setattr()/sched_getatr() documentation, 
> I noticed a bug in the handling of -EFBIG, and a couple of places 
> in the code where the error handling logic could be streamlined.
> Patches follow.

Thanks!

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] sched: sched_setattr/sched_getattr fixes and improvements

[Michael Kerrisk (man-pages)]

On 05/09/2014 07:16 PM, Peter Zijlstra wrote:
> On Fri, May 09, 2014 at 04:49:18PM +0200, Michael Kerrisk (man-pages) wrote:
>> Peter,
>>
>> While looking at your sched_setattr()/sched_getatr() documentation, 
>> I noticed a bug in the handling of -EFBIG, and a couple of places 
>> in the code where the error handling logic could be streamlined.
>> Patches follow.
> 
> Thanks!

Hi Peter,

I was a little unclear on what "Thanks" meant... Does it mean
you plan to apply all 3 patches in your tree to go upstream?

Thanks,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 0/3] sched: sched_setattr/sched_getattr fixes and improvements

[Peter Zijlstra]

On Tue, May 13, 2014 at 08:09:34PM +0200, Michael Kerrisk (man-pages) wrote:
> On 05/09/2014 07:16 PM, Peter Zijlstra wrote:
> > On Fri, May 09, 2014 at 04:49:18PM +0200, Michael Kerrisk (man-pages) wrote:
> >> Peter,
> >>
> >> While looking at your sched_setattr()/sched_getatr() documentation, 
> >> I noticed a bug in the handling of -EFBIG, and a couple of places 
> >> in the code where the error handling logic could be streamlined.
> >> Patches follow.
> > 
> > Thanks!
> 
> Hi Peter,
> 
> I was a little unclear on what "Thanks" meant... Does it mean
> you plan to apply all 3 patches in your tree to go upstream?

Yeah, I queued them. They should appear in tip sometime this week or so.

Re: [PATCH 1/3] sched: Make sched_setattr() correctly return -EFBIG

[Michael Kerrisk (man-pages)]

Peter,

In the title of this patch, EFBIG should have been E2BIG. Shall
I resubmit a corrected version?

Cheers,

Michael

On 05/09/2014 04:54 PM, Michael Kerrisk (man-pages) wrote:
> From: Michael Kerrisk <mtk.manpages@gmail.com>
> 
> The documented[1] behavior of sched_attr() in your proposed man page text is:
> 
>     sched_attr::size must be set to the size of the structure, as in
>     sizeof(struct sched_attr), if the provided structure is smaller
>     than the kernel structure, any additional fields are assumed
>     '0'. If the provided structure is larger than the kernel structure,
>     the kernel verifies all additional fields are '0' if not the
>     syscall will fail with -E2BIG.
> 
> As currently implemented, sched_copy_attr() returns -EFBIG for
> for this case, but the logic in sys_sched_setattr() converts that
> error to -EFAULT. This patch fixes the behavior.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/1615615/focus=1697760
> 
> Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
> ---
>  kernel/sched/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 268a45e..6c9ce28 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3650,8 +3650,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
>  	if (!uattr || pid < 0 || flags)
>  		return -EINVAL;
>  
> -	if (sched_copy_attr(uattr, &attr))
> -		return -EFAULT;
> +	retval = sched_copy_attr(uattr, &attr);
> +	if (retval)
> +		return retval;
>  
>  	rcu_read_lock();
>  	retval = -ESRCH;
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

[tip:sched/core] sched: Make sched_setattr() correctly return -EFBIG

[tip-bot for Michael Kerrisk]

Commit-ID:  51b2b2dc49a8a053de58b836be7efd2401a88699
Gitweb:     http://git.kernel.org/tip/51b2b2dc49a8a053de58b836be7efd2401a88699
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:15 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 May 2014 21:47:33 +0900

sched: Make sched_setattr() correctly return -EFBIG

The documented[1] behavior of sched_attr() in your proposed man page text is:

    sched_attr::size must be set to the size of the structure, as in
    sizeof(struct sched_attr), if the provided structure is smaller
    than the kernel structure, any additional fields are assumed
    '0'. If the provided structure is larger than the kernel structure,
    the kernel verifies all additional fields are '0' if not the
    syscall will fail with -E2BIG.

As currently implemented, sched_copy_attr() returns -EFBIG for
for this case, but the logic in sys_sched_setattr() converts that
error to -EFAULT. This patch fixes the behavior.

[1] http://thread.gmane.org/gmane.linux.kernel/1615615/focus=1697760

Cc: stable@vger.kernel.org
Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/536CEC17.9070903@gmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 13584f1..f2205f0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3658,8 +3658,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	if (!uattr || pid < 0 || flags)
 		return -EINVAL;
 
-	if (sched_copy_attr(uattr, &attr))
-		return -EFAULT;
+	retval = sched_copy_attr(uattr, &attr);
+	if (retval)
+		return retval;
 
 	rcu_read_lock();
 	retval = -ESRCH;

[tip:sched/core] sched: Simplify return logic in sched_copy_attr( )

[tip-bot for Michael Kerrisk]

Commit-ID:  814927325e815b667a133d9dcdf2286f88b78328
Gitweb:     http://git.kernel.org/tip/814927325e815b667a133d9dcdf2286f88b78328
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:28 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 May 2014 22:02:39 +0900

sched: Simplify return logic in sched_copy_attr()

The logic in this function is a little contorted:
* Rather than having chained gotos for the -EFBIG case, just
  return -EFBIG directly.
* Now, the label 'out' is no longer needed, and 'ret' must be zero
  zero by the time we fall through to this point, so just return 0.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/536CEC24.9080201@gmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9e1a3e0..08b6355 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3657,13 +3657,11 @@ static int sched_copy_attr(struct sched_attr __user *uattr,
 	 */
 	attr->sched_nice = clamp(attr->sched_nice, MIN_NICE, MAX_NICE);
 
-out:
-	return ret;
+	return 0;
 
 err_size:
 	put_user(sizeof(*attr), &uattr->size);
-	ret = -E2BIG;
-	goto out;
+	return -E2BIG;
 }
 
 /**

[tip:sched/core] sched: Simplify return logic in sched_read_attr( )

[tip-bot for Michael Kerrisk]

Commit-ID:  55854e657f9890a1f76105c698d1d52cf6c43661
Gitweb:     http://git.kernel.org/tip/55854e657f9890a1f76105c698d1d52cf6c43661
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:33 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 May 2014 22:02:40 +0900

sched: Simplify return logic in sched_read_attr()

Gotos are chained pointlessly here, and the 'out' label
can be dispensed with.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/536CEC29.9090503@gmail.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 08b6355..29c542e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3821,7 +3821,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 
 		for (; addr < end; addr++) {
 			if (*addr)
-				goto err_size;
+				return -EFBIG;
 		}
 
 		attr->size = usize;
@@ -3831,12 +3831,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 	if (ret)
 		return -EFAULT;
 
-out:
-	return ret;
-
-err_size:
-	ret = -E2BIG;
-	goto out;
+	return 0;
 }
 
 /**

[tip:sched/core] sched: Make sched_setattr() correctly return -EFBIG

[tip-bot for Michael Kerrisk]

Commit-ID:  143cf23df25b7082cd706c3c53188e741e7881c3
Gitweb:     http://git.kernel.org/tip/143cf23df25b7082cd706c3c53188e741e7881c3
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:15 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 22 May 2014 10:21:25 +0200

sched: Make sched_setattr() correctly return -EFBIG

The documented[1] behavior of sched_attr() in the proposed man page text is:

    sched_attr::size must be set to the size of the structure, as in
    sizeof(struct sched_attr), if the provided structure is smaller
    than the kernel structure, any additional fields are assumed
    '0'. If the provided structure is larger than the kernel structure,
    the kernel verifies all additional fields are '0' if not the
    syscall will fail with -E2BIG.

As currently implemented, sched_copy_attr() returns -EFBIG for
for this case, but the logic in sys_sched_setattr() converts that
error to -EFAULT. This patch fixes the behavior.

[1] http://thread.gmane.org/gmane.linux.kernel/1615615/focus=1697760

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: <stable@vger.kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/536CEC17.9070903@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 13584f1..f2205f0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3658,8 +3658,9 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr,
 	if (!uattr || pid < 0 || flags)
 		return -EINVAL;
 
-	if (sched_copy_attr(uattr, &attr))
-		return -EFAULT;
+	retval = sched_copy_attr(uattr, &attr);
+	if (retval)
+		return retval;
 
 	rcu_read_lock();
 	retval = -ESRCH;

[tip:sched/core] sched: Simplify return logic in sched_copy_attr( )

[tip-bot for Michael Kerrisk]

Commit-ID:  e78c7bca56dab5ce4f22694f99115ec07e4935f6
Gitweb:     http://git.kernel.org/tip/e78c7bca56dab5ce4f22694f99115ec07e4935f6
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:28 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 22 May 2014 11:16:26 +0200

sched: Simplify return logic in sched_copy_attr()

The logic in this function is a little contorted, clean it up:

  * Rather than having chained gotos for the -EFBIG case, just
    return -EFBIG directly.

  * Now, the label 'out' is no longer needed, and 'ret' must be zero
    zero by the time we fall through to this point, so just return 0.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/536CEC24.9080201@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2551b6d..2318fc4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3657,13 +3657,11 @@ static int sched_copy_attr(struct sched_attr __user *uattr,
 	 */
 	attr->sched_nice = clamp(attr->sched_nice, MIN_NICE, MAX_NICE);
 
-out:
-	return ret;
+	return 0;
 
 err_size:
 	put_user(sizeof(*attr), &uattr->size);
-	ret = -E2BIG;
-	goto out;
+	return -E2BIG;
 }
 
 /**

[tip:sched/core] sched: Simplify return logic in sched_read_attr( )

[tip-bot for Michael Kerrisk]

Commit-ID:  22400674945c562cf3c176d6c4178cd545e2dec9
Gitweb:     http://git.kernel.org/tip/22400674945c562cf3c176d6c4178cd545e2dec9
Author:     Michael Kerrisk <mtk.manpages@gmail.com>
AuthorDate: Fri, 9 May 2014 16:54:33 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 22 May 2014 11:16:27 +0200

sched: Simplify return logic in sched_read_attr()

Gotos are chained pointlessly here, and the 'out' label
can be dispensed with.

Signed-off-by: Michael Kerrisk <mtk.manpages@gmail.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/536CEC29.9090503@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2318fc4..a78c5b6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3821,7 +3821,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 
 		for (; addr < end; addr++) {
 			if (*addr)
-				goto err_size;
+				return -EFBIG;
 		}
 
 		attr->size = usize;
@@ -3831,12 +3831,7 @@ static int sched_read_attr(struct sched_attr __user *uattr,
 	if (ret)
 		return -EFAULT;
 
-out:
-	return ret;
-
-err_size:
-	ret = -E2BIG;
-	goto out;
+	return 0;
 }
 
 /**