Spotted by Satoru Takeuchi.
kill_pgrp(task_pgrp(current)) sends the signal to the current's thread group,
but can choose any sub-thread as a target for signal_wake_up(). This means
that job_control() and tty_check_change() may return -ERESTARTSYS without
signal_pending().
Signed-off-by: Oleg Nesterov <[email protected]>
--- t/drivers/char/n_tty.c~ 2007-04-05 12:18:26.000000000 +0400
+++ t/drivers/char/n_tty.c 2007-05-28 10:57:58.000000000 +0400
@@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
is_current_pgrp_orphaned())
return -EIO;
kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+ set_thread_flag(TIF_SIGPENDING);
return -ERESTARTSYS;
}
}
--- t/drivers/char/tty_io.c~ 2007-04-05 12:18:26.000000000 +0400
+++ t/drivers/char/tty_io.c 2007-05-29 22:15:52.000000000 +0400
@@ -1121,7 +1121,8 @@ int tty_check_change(struct tty_struct *
return 0;
if (is_current_pgrp_orphaned())
return -EIO;
- (void) kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+ kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+ set_thread_flag(TIF_SIGPENDING);
return -ERESTARTSYS;
}
On Tue, 29 May 2007 22:44:35 +0400
Oleg Nesterov <[email protected]> wrote:
> Spotted by Satoru Takeuchi.
>
> kill_pgrp(task_pgrp(current)) sends the signal to the current's thread group,
> but can choose any sub-thread as a target for signal_wake_up(). This means
> that job_control() and tty_check_change() may return -ERESTARTSYS without
> signal_pending().
>
> Signed-off-by: Oleg Nesterov <[email protected]>
>
> --- t/drivers/char/n_tty.c~ 2007-04-05 12:18:26.000000000 +0400
> +++ t/drivers/char/n_tty.c 2007-05-28 10:57:58.000000000 +0400
> @@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
> is_current_pgrp_orphaned())
> return -EIO;
> kill_pgrp(task_pgrp(current), SIGTTIN, 1);
> + set_thread_flag(TIF_SIGPENDING);
> return -ERESTARTSYS;
> }
> }
> --- t/drivers/char/tty_io.c~ 2007-04-05 12:18:26.000000000 +0400
> +++ t/drivers/char/tty_io.c 2007-05-29 22:15:52.000000000 +0400
> @@ -1121,7 +1121,8 @@ int tty_check_change(struct tty_struct *
> return 0;
> if (is_current_pgrp_orphaned())
> return -EIO;
> - (void) kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> + kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> + set_thread_flag(TIF_SIGPENDING);
> return -ERESTARTSYS;
> }
>
Are there other callers of kill_pgrp() which have the same problem?
Perhaps we should have a kill_pgrp_self() which takes care of doing
this, rather than open-coding it. Something with a comment which
explains what's going on ;)
On 05/30, Andrew Morton wrote:
> On Tue, 29 May 2007 22:44:35 +0400
> Oleg Nesterov <[email protected]> wrote:
>
> > --- t/drivers/char/n_tty.c~ 2007-04-05 12:18:26.000000000 +0400
> > +++ t/drivers/char/n_tty.c 2007-05-28 10:57:58.000000000 +0400
> > @@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
> > is_current_pgrp_orphaned())
> > return -EIO;
> > kill_pgrp(task_pgrp(current), SIGTTIN, 1);
> > + set_thread_flag(TIF_SIGPENDING);
> > return -ERESTARTSYS;
> > }
> > }
>
> Are there other callers of kill_pgrp() which have the same problem?
Hopefully no.
> Perhaps we should have a kill_pgrp_self() which takes care of doing
> this, rather than open-coding it. Something with a comment which
> explains what's going on ;)
This set_thread_flag(TIF_SIGPENDING) is "connected" to "return -ERESTARTSYS",
not to kill_pgrp(), imho the new helper is not so suitable.
Perhaps it makes sense to add the comment into include/linux/errno.h, to
explain that -ERESTART... codes are only valid when signal_pending() == true.
Oleg.
At Wed, 30 May 2007 23:18:49 +0400,
Oleg Nesterov wrote:
>
> On 05/30, Andrew Morton wrote:
> > On Tue, 29 May 2007 22:44:35 +0400
> > Oleg Nesterov <[email protected]> wrote:
> >
> > > --- t/drivers/char/n_tty.c~ 2007-04-05 12:18:26.000000000 +0400
> > > +++ t/drivers/char/n_tty.c 2007-05-28 10:57:58.000000000 +0400
> > > @@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
> > > is_current_pgrp_orphaned())
> > > return -EIO;
> > > kill_pgrp(task_pgrp(current), SIGTTIN, 1);
> > > + set_thread_flag(TIF_SIGPENDING);
> > > return -ERESTARTSYS;
> > > }
> > > }
> >
> > Are there other callers of kill_pgrp() which have the same problem?
>
> Hopefully no.
>
> > Perhaps we should have a kill_pgrp_self() which takes care of doing
> > this, rather than open-coding it. Something with a comment which
> > explains what's going on ;)
>
> This set_thread_flag(TIF_SIGPENDING) is "connected" to "return -ERESTARTSYS",
> not to kill_pgrp(), imho the new helper is not so suitable.
>
> Perhaps it makes sense to add the comment into include/linux/errno.h, to
> explain that -ERESTART... codes are only valid when signal_pending() == true.
Like this?
Satoru
---
Add comment for errnos related to restart syscall to avoid the leakage of
kernel only errnos.
Signed-off-by: Satoru Takeuchi <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Index: linux-2.6.22-rc3/include/linux/errno.h
===================================================================
--- linux-2.6.22-rc3.orig/include/linux/errno.h 2007-04-26 12:08:32.000000000 +0900
+++ linux-2.6.22-rc3/include/linux/errno.h 2007-05-31 09:44:27.000000000 +0900
@@ -5,7 +5,11 @@
#ifdef __KERNEL__
-/* Should never be seen by user programs */
+/*
+ * Should never be seen by user programs. Please note that returing
+ * `ERESTART*' errnos when `!signal_pending()' incurs the leakage of these
+ * errnos to user space.
+ */
#define ERESTARTSYS 512
#define ERESTARTNOINTR 513
#define ERESTARTNOHAND 514 /* restart if no handler.. */
Aside from typos, I think it should be more clearly and strongly worded.
"These should never be seen by user programs. To return one of these
codes, signal_pending() MUST be set. Note that ptrace can observe these at
syscall exit tracing, but they will never be left for the debugged user
process to see."
Thanks,
Roland
At Wed, 30 May 2007 18:08:55 -0700 (PDT),
Roland McGrath wrote:
>
> Aside from typos, I think it should be more clearly and strongly worded.
>
> "These should never be seen by user programs. To return one of these
> codes, signal_pending() MUST be set. Note that ptrace can observe these at
> syscall exit tracing, but they will never be left for the debugged user
> process to see."
Thanks, is that better?
Satoru
---
Add comment for errnos related to restart syscall to avoid the leakage of
them to user programs.
Signed-off-by: Satoru Takeuchi <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Roland McGrath <[email protected]>
Index: linux-2.6.22-rc3/include/linux/errno.h
===================================================================
--- linux-2.6.22-rc3.orig/include/linux/errno.h 2007-04-26 12:08:32.000000000 +0900
+++ linux-2.6.22-rc3/include/linux/errno.h 2007-05-31 10:47:40.000000000 +0900
@@ -5,7 +5,12 @@
#ifdef __KERNEL__
-/* Should never be seen by user programs */
+/*
+ * These should never be seen by user programs. To return one of ERESTART*
+ * codes, signal_pending() MUST be set. Note that ptrace can observe these
+ * at syscall exit tracing, but they will never be left for the debugged user
+ * process to see.
+ */
#define ERESTARTSYS 512
#define ERESTARTNOINTR 513
#define ERESTARTNOHAND 514 /* restart if no handler.. */
That looks good to me.
Thanks,
Roland