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

[Chen Gang <gang.chen@asianux.com>]

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