[Patch] signal: let valid_signal() check more

[Patch] signal: let valid_signal() check more

[Américo Wang]

Teach valid_signal() to check sig > 0 case.

Tested on UML only.

Signed-off-by: WANG Cong <wangcong@zeuux.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Oleg Nesterov <oleg@redhat.com>

---
diff --git a/drivers/char/vt_ioctl.c b/drivers/char/vt_ioctl.c
index 8944ce5..e7568ff 100644
--- a/drivers/char/vt_ioctl.c
+++ b/drivers/char/vt_ioctl.c
@@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
 	{
 		if (!perm || !capable(CAP_KILL))
 			goto eperm;
-		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
+		if (!valid_signal((int)arg) || arg == SIGKILL)
 			ret = -EINVAL;
 		else {
 			spin_lock_irq(&vt_spawn_con.lock);
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 549daf8..32c8677 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -313,11 +313,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		break;
 	case F_SETSIG:
 		/* arg == 0 restores default behaviour. */
-		if (!valid_signal(arg)) {
-			break;
+		if (arg == 0 || valid_signal((int)arg)) {
+			err = 0;
+			filp->f_owner.signum = arg;
 		}
-		err = 0;
-		filp->f_owner.signum = arg;
 		break;
 	case F_GETLEASE:
 		err = fcntl_getlease(filp);
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 84f997f..cddf220 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -227,9 +227,9 @@ static inline void init_sigpending(struct sigpending *sig)
 extern void flush_sigqueue(struct sigpending *queue);
 
 /* Test if 'sig' is valid signal. Use this instead of testing _NSIG directly */
-static inline int valid_signal(unsigned long sig)
+static inline int valid_signal(int sig)
 {
-	return sig <= _NSIG ? 1 : 0;
+	return sig <= _NSIG ? (sig > 0) : 0;
 }
 
 extern int next_signal(struct sigpending *pending, sigset_t *mask);
diff --git a/kernel/exit.c b/kernel/exit.c
index 2d8be7e..096beb5 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -376,7 +376,7 @@ static void set_special_pids(struct pid *pid)
  */
 int allow_signal(int sig)
 {
-	if (!valid_signal(sig) || sig < 1)
+	if (!valid_signal(sig))
 		return -EINVAL;
 
 	spin_lock_irq(&current->sighand->siglock);
@@ -397,7 +397,7 @@ EXPORT_SYMBOL(allow_signal);
 
 int disallow_signal(int sig)
 {
-	if (!valid_signal(sig) || sig < 1)
+	if (!valid_signal(sig))
 		return -EINVAL;
 
 	spin_lock_irq(&current->sighand->siglock);
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4c8bcd7..e8eb715 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -369,7 +369,7 @@ static int ptrace_setsiginfo(struct task_struct *child, const siginfo_t *info)
 
 static int ptrace_resume(struct task_struct *child, long request, long data)
 {
-	if (!valid_signal(data))
+	if (!valid_signal((int)data))
 		return -EIO;
 
 	if (request == PTRACE_SYSCALL)
diff --git a/kernel/signal.c b/kernel/signal.c
index 4530fc6..8355426 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1411,7 +1411,7 @@ int do_notify_parent(struct task_struct *tsk, int sig)
 		if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
 			sig = -1;
 	}
-	if (valid_signal(sig) && sig > 0)
+	if (valid_signal(sig))
 		__group_send_sig_info(sig, &info, tsk->parent);
 	__wake_up_parent(tsk, tsk->parent);
 	spin_unlock_irqrestore(&psig->siglock, flags);
@@ -2308,7 +2308,7 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
 	struct k_sigaction *k;
 	sigset_t mask;
 
-	if (!valid_signal(sig) || sig < 1 || (act && sig_kernel_only(sig)))
+	if (!valid_signal(sig) || (act && sig_kernel_only(sig)))
 		return -EINVAL;
 
 	k = &t->sighand->action[sig-1];
diff --git a/kernel/sys.c b/kernel/sys.c
index 31deba8..06dc092 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1631,7 +1631,7 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3,
 
 	switch (option) {
 		case PR_SET_PDEATHSIG:
-			if (!valid_signal(arg2)) {
+			if (!valid_signal((int)arg2)) {
 				error = -EINVAL;
 				break;
 			}

Re: [Patch] signal: let valid_signal() check more

[Oleg Nesterov]

On 12/26, Américo Wang wrote:
>
> Teach valid_signal() to check sig > 0 case.

Why?

> @@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
>  	{
>  		if (!perm || !capable(CAP_KILL))
>  			goto eperm;
> -		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
> +		if (!valid_signal((int)arg) || arg == SIGKILL)
                                  ^^^^^

The patch adds a lot of unnecessary typecasts like this.

> -static inline int valid_signal(unsigned long sig)
> +static inline int valid_signal(int sig)
>  {
> -	return sig <= _NSIG ? 1 : 0;
> +	return sig <= _NSIG ? (sig > 0) : 0;
>  }

This looks a bit strange, why not

	return sig > 0 && sig <= _NSIG;

?

But, more importantly, I don't think the patch is correct.

Unless I misread the patch, now kill(pid, 0) returns -EINVAL, no?

And we have other users of valid_signal() which assume that sig == 0
is OK, for example arch_ptrace().


Imho, the patch has a point, but perhaps it is better to add the
new helper and then convert the users which do something like

	if (valid_signal(sig) && sig)
		...

What do you think?

Oleg.

Re: [Patch] signal: let valid_signal() check more

[Américo Wang]

On Thu, Dec 25, 2008 at 07:00:54PM +0100, Oleg Nesterov wrote:
>On 12/26, Américo Wang wrote:
>>
>> Teach valid_signal() to check sig > 0 case.
>
>Why?

Just to simplify the checking.

>
>> @@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
>>  	{
>>  		if (!perm || !capable(CAP_KILL))
>>  			goto eperm;
>> -		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
>> +		if (!valid_signal((int)arg) || arg == SIGKILL)
>                                  ^^^^^
>
>The patch adds a lot of unnecessary typecasts like this.


because it's inline?

>
>> -static inline int valid_signal(unsigned long sig)
>> +static inline int valid_signal(int sig)
>>  {
>> -	return sig <= _NSIG ? 1 : 0;
>> +	return sig <= _NSIG ? (sig > 0) : 0;
>>  }
>
>This looks a bit strange, why not
>
>	return sig > 0 && sig <= _NSIG;
>
>?


Yes, this one is better.

>
>But, more importantly, I don't think the patch is correct.
>
>Unless I misread the patch, now kill(pid, 0) returns -EINVAL, no?
>
>And we have other users of valid_signal() which assume that sig == 0
>is OK, for example arch_ptrace().


Oh, thanks for pointing this out. I didn't know this. Sorry.

>
>Imho, the patch has a point, but perhaps it is better to add the
>new helper and then convert the users which do something like
>
>	if (valid_signal(sig) && sig)
>		...
>
>What do you think?

I think this is a good idea. I will do it.

Thanks.

-- 
"Against stupidity, the gods themselves, contend in vain."

Re: [Patch] signal: let valid_signal() check more

[Ingo Molnar]

* Américo Wang <xiyou.wangcong@gmail.com> wrote:

> >> @@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
> >>  	{
> >>  		if (!perm || !capable(CAP_KILL))
> >>  			goto eperm;
> >> -		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
> >> +		if (!valid_signal((int)arg) || arg == SIGKILL)
> >                                  ^^^^^
> >
> >The patch adds a lot of unnecessary typecasts like this.
> 
> because it's inline?

Why does your patch add a lot of seemingly unnecessary typecasts? [if your 
short reply was supposed to be an answer to that question then please 
explain it in more detail.]

	Ingo

Re: [Patch] signal: let valid_signal() check more

[Américo Wang]

On Fri, Dec 26, 2008 at 09:56:54AM +0100, Ingo Molnar wrote:
>
>* Américo Wang <xiyou.wangcong@gmail.com> wrote:
>
>> >> @@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
>> >>  	{
>> >>  		if (!perm || !capable(CAP_KILL))
>> >>  			goto eperm;
>> >> -		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
>> >> +		if (!valid_signal((int)arg) || arg == SIGKILL)
>> >                                  ^^^^^
>> >
>> >The patch adds a lot of unnecessary typecasts like this.
>> 
>> because it's inline?
>
>Why does your patch add a lot of seemingly unnecessary typecasts? [if your 
>short reply was supposed to be an answer to that question then please 
>explain it in more detail.]

Hi, Ingo.

because I also changed the type of valid_signal():

 -static inline int valid_signal(unsigned long sig)
 +static inline int valid_signal(int sig)

I noticed that gcc put this kind of warning into
-Wtraditional-conversion recently, but it is still useful to use
explicit cast, isn't it?

Thanks.

-- 
"Against stupidity, the gods themselves, contend in vain."

Re: [Patch] signal: let valid_signal() check more

[Oleg Nesterov]

On 12/26, Américo Wang wrote:
>
> On Fri, Dec 26, 2008 at 09:56:54AM +0100, Ingo Molnar wrote:
> >
> >* Américo Wang <xiyou.wangcong@gmail.com> wrote:
> >
> >> >> @@ -727,7 +727,7 @@ int vt_ioctl(struct tty_struct *tty, struct file * file,
> >> >>  	{
> >> >>  		if (!perm || !capable(CAP_KILL))
> >> >>  			goto eperm;
> >> >> -		if (!valid_signal(arg) || arg < 1 || arg == SIGKILL)
> >> >> +		if (!valid_signal((int)arg) || arg == SIGKILL)
> >> >                                  ^^^^^
> >> >
> >> >The patch adds a lot of unnecessary typecasts like this.
> >> 
> >> because it's inline?
> >
> >Why does your patch add a lot of seemingly unnecessary typecasts? [if your 
> >short reply was supposed to be an answer to that question then please 
> >explain it in more detail.]
> 
> Hi, Ingo.
> 
> because I also changed the type of valid_signal():
> 
>  -static inline int valid_signal(unsigned long sig)
>  +static inline int valid_signal(int sig)

and please note that this change itself is a bit dangerous. Suppose that
a bad user does sys_prctl(PR_SET_PDEATHSIG, LONG_MIN | SIGCHLD), and now
valid_signal(arg2) (or valid_signal((int)arg2)) returns T.

(nothing really bad happens because ->pdeath_signal is "int", but still).

Note also that it is correct to do prctl(PR_SET_PDEATHSIG, 0), so the
patch was doubly wrong here.

So, please check very carefully every change you are going to do. Make
sure you don't change the behaviour, unless you think the current code
is buggy. In that case please document the fix.

OTOH, I don't understand why sys_mq_notify() accepts sigev_signo == 0,
perhaps this is oversight. good_sigevent() looks correct, but should
use the helper.

> I noticed that gcc put this kind of warning into
> -Wtraditional-conversion recently, but it is still useful to use
> explicit cast, isn't it?

I'd say explicit casts should be avoided as much as possible.

As for this particular case. Note that valid_signa((int)long_arg)
only helps to hide the problem.

Oleg.