[PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[Chen Gang]

For the system call getitimer(), if the parameter 'value' is NULL, need
return -EINVAL, not -EFAULT.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/itimer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/itimer.c b/kernel/itimer.c
index 8d262b4..6f9d757 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -102,7 +102,7 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error = -EINVAL;
 	struct itimerval get_buffer;
 
 	if (value) {
-- 
1.7.7.6

Re: [PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:

> For the system call getitimer(), if the parameter 'value' is NULL, need
> return -EINVAL, not -EFAULT.

Care to explain why? Because you are feeling so?

I recommend reading the man page of getitimer:

 ERRORS
       EFAULT new_value, old_value, or curr_value is not valid a pointer.

And NULL is definitely NOT a valid pointer.

The Posix spec does not specify an explicit error value for this
syscall, but the general policy is:

[EFAULT]
    Bad address. The system detected an invalid address in attempting
    to use an argument of a call. The reliable detection of this error
    cannot be guaranteed, and when not detected may result in the
    generation of a signal, indicating an address violation, which is
    sent to the process.

And we made use of this, which is correct and makes sense.

Returning EINVAL makes no sense at all, because EINVAL _IS_ a
specified error code for this syscall:

[EINVAL]
        The which argument is not recognized.

Thanks,

	tglx

Re: [PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[Chen Gang]

On 06/20/2013 02:59 PM, Thomas Gleixner wrote:
> On Thu, 20 Jun 2013, Chen Gang wrote:
> 
>> > For the system call getitimer(), if the parameter 'value' is NULL, need
>> > return -EINVAL, not -EFAULT.
> Care to explain why? Because you are feeling so?
> 

I am not feeling so, the original implementation really just checks the parameter 'value', if it is invalid, need return, is it incorrect ??


> I recommend reading the man page of getitimer:
> 
>  ERRORS
>        EFAULT new_value, old_value, or curr_value is not valid a pointer.
> 
> And NULL is definitely NOT a valid pointer.
> 
> The Posix spec does not specify an explicit error value for this
> syscall, but the general policy is:
> 
> [EFAULT]
>     Bad address. The system detected an invalid address in attempting
>     to use an argument of a call. The reliable detection of this error
>     cannot be guaranteed, and when not detected may result in the
>     generation of a signal, indicating an address violation, which is
>     sent to the process.
> 
> And we made use of this, which is correct and makes sense.
> 
> Returning EINVAL makes no sense at all, because EINVAL _IS_ a
> specified error code for this syscall:
> 
> [EINVAL]
>         The which argument is not recognized.

That means we need not check the parameter 'value' out side of copy_to_user().

And the implement need like this:

---------------------------diff begin-----------------------------------

diff --git a/kernel/itimer.c b/kernel/itimer.c
index 8d262b4..3b12271 100644
--- a/kernel/itimer.c
+++ b/kernel/itimer.c
@@ -102,15 +102,14 @@ int do_getitimer(int which, struct itimerval *value)
 
 SYSCALL_DEFINE2(getitimer, int, which, struct itimerval __user *, value)
 {
-	int error = -EFAULT;
+	int error;
 	struct itimerval get_buffer;
 
-	if (value) {
-		error = do_getitimer(which, &get_buffer);
-		if (!error &&
-		    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
-			error = -EFAULT;
-	}
+	error = do_getitimer(which, &get_buffer);
+	if (!error &&
+	    copy_to_user(value, &get_buffer, sizeof(get_buffer)))
+		error = -EFAULT;
+
 	return error;
 }


---------------------------diff end-------------------------------------


Thanks.
-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[Thomas Gleixner]

On Thu, 20 Jun 2013, Chen Gang wrote:
> On 06/20/2013 02:59 PM, Thomas Gleixner wrote:
> > On Thu, 20 Jun 2013, Chen Gang wrote:
> > 
> >> > For the system call getitimer(), if the parameter 'value' is NULL, need
> >> > return -EINVAL, not -EFAULT.
> > Care to explain why? Because you are feeling so?
> > 
> 
> I am not feeling so, the original implementation really just checks the parameter 'value', if it is invalid, need return, is it incorrect ??
> 
> 
> > I recommend reading the man page of getitimer:
> > 
> >  ERRORS
> >        EFAULT new_value, old_value, or curr_value is not valid a pointer.
> > 
> > And NULL is definitely NOT a valid pointer.
> > 
> > The Posix spec does not specify an explicit error value for this
> > syscall, but the general policy is:
> > 
> > [EFAULT]
> >     Bad address. The system detected an invalid address in attempting
> >     to use an argument of a call. The reliable detection of this error
> >     cannot be guaranteed, and when not detected may result in the
> >     generation of a signal, indicating an address violation, which is
> >     sent to the process.
> > 
> > And we made use of this, which is correct and makes sense.
> > 
> > Returning EINVAL makes no sense at all, because EINVAL _IS_ a
> > specified error code for this syscall:
> > 
> > [EINVAL]
> >         The which argument is not recognized.
> 
> That means we need not check the parameter 'value' out side of copy_to_user().

We could do that, but that makes no sense. If we can detect it before
copy_to_user() we can return the exactly same return value which we
would return via copy_to_user(). That avoids to take a trap and run
through the fixup code.

Thanks,

	tglx

_

Re: [PATCH] kernel/itimer.c: for return value, using -EINVAL instead of -EFAULT

[Chen Gang]

On 06/20/2013 03:44 PM, Thomas Gleixner wrote:
> We could do that, but that makes no sense. If we can detect it before
> copy_to_user() we can return the exactly same return value which we
> would return via copy_to_user(). That avoids to take a trap and run
> through the fixup code

I don't think "that makes no sense".

In most cases, the 'value' will not be NULL, so it is better for
performance (save one compare instruction, at least).

Also, it will make the code simpler and clearer for readers.



Thanks.
-- 
Chen Gang

Asianux Corporation