Skip to contentSkip to conversation

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

PATCH In review Chen Gang 2 participants 5 messages Jun 20, 2013

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

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

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

Signed-off-by Chen Gang <[email protected]>
---
 kernel/itimer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/itimer.c +1 -1
index 8d262b4..6f9d757 100644
@@ -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) {
Signature · 1 line
1.7.7.6
#1 lore raw
Quoted 2 lines · Chen Gang

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

author
#2 lore raw
Quoted 6 lines · Thomas Gleixner
Quoted 2 lines · Chen Gang
Quoted 2 lines

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 ??

Quoted 17 lines

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.

Quoted 7 lines

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-----------------------------------

kernel/itimer.c +6 -7
index 8d262b4..3b12271 100644
@@ -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.

Signature · 3 lines
Chen Gang

Asianux Corporation
#3 lore raw
Quoted 30 lines · Chen Gang
Quoted 6 lines · Thomas Gleixner
Quoted 2 lines · Chen Gang
Quoted 2 lines

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 ??

Quoted 19 lines

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.

Quoted 7 lines
Quoted 5 lines

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

_

author
#4 lore raw
Quoted 4 lines · Thomas Gleixner

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.

Signature · 3 lines
Chen Gang

Asianux Corporation