There are a few system calls (pselect, ppoll, etc) which replace a task
sigmask while they are running in a kernel-space
When a task calls one of these syscalls, the kernel saves a current
sigmask in task->saved_sigmask and sets a syscall sigmask.
On syscall-exit-stop, ptrace traps a task before restoring the
saved_sigmask, so PTRACE_GETSIGMASK returns the syscall sigmask and
PTRACE_SETSIGMASK does nothing, because its sigmask is replaced by
saved_sigmask, when the task returns to user-space.
This patch fixes this problem. PTRACE_GET_SIGMASK returns saved_sigmask
is it's set. PTRACE_SETSIGMASK drops the TIF_RESTORE_SIGMASK flag.
Cc: Oleg Nesterov <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Andrew Morton <[email protected]>
Fixes: 29000caecbe8 ("ptrace: add ability to get/set signal-blocked mask")
Signed-off-by: Andrei Vagin <[email protected]>
---
include/linux/sched/signal.h | 18 ++++++++++++++++++
kernel/ptrace.c | 15 +++++++++++++--
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 1be35729c2c5..660d78c9af6c 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -417,10 +417,20 @@ static inline void set_restore_sigmask(void)
set_thread_flag(TIF_RESTORE_SIGMASK);
WARN_ON(!test_thread_flag(TIF_SIGPENDING));
}
+
+static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
+{
+ clear_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
+}
+
static inline void clear_restore_sigmask(void)
{
clear_thread_flag(TIF_RESTORE_SIGMASK);
}
+static inline bool test_tsk_restore_sigmask(struct task_struct *tsk)
+{
+ return test_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
+}
static inline bool test_restore_sigmask(void)
{
return test_thread_flag(TIF_RESTORE_SIGMASK);
@@ -438,6 +448,10 @@ static inline void set_restore_sigmask(void)
current->restore_sigmask = true;
WARN_ON(!test_thread_flag(TIF_SIGPENDING));
}
+static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
+{
+ tsk->restore_sigmask = false;
+}
static inline void clear_restore_sigmask(void)
{
current->restore_sigmask = false;
@@ -446,6 +460,10 @@ static inline bool test_restore_sigmask(void)
{
return current->restore_sigmask;
}
+static inline bool test_tsk_restore_sigmask(struct task_struct *tsk)
+{
+ return tsk->restore_sigmask;
+}
static inline bool test_and_clear_restore_sigmask(void)
{
if (!current->restore_sigmask)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 21fec73d45d4..fc0d667f5792 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -29,6 +29,7 @@
#include <linux/hw_breakpoint.h>
#include <linux/cn_proc.h>
#include <linux/compat.h>
+#include <linux/sched/signal.h>
/*
* Access another process' address space via ptrace.
@@ -925,18 +926,26 @@ int ptrace_request(struct task_struct *child, long request,
ret = ptrace_setsiginfo(child, &siginfo);
break;
- case PTRACE_GETSIGMASK:
+ case PTRACE_GETSIGMASK: {
+ sigset_t *mask;
+
if (addr != sizeof(sigset_t)) {
ret = -EINVAL;
break;
}
- if (copy_to_user(datavp, &child->blocked, sizeof(sigset_t)))
+ if (test_tsk_restore_sigmask(child))
+ mask = &child->saved_sigmask;
+ else
+ mask = &child->blocked;
+
+ if (copy_to_user(datavp, mask, sizeof(sigset_t)))
ret = -EFAULT;
else
ret = 0;
break;
+ }
case PTRACE_SETSIGMASK: {
sigset_t new_set;
@@ -962,6 +971,8 @@ int ptrace_request(struct task_struct *child, long request,
child->blocked = new_set;
spin_unlock_irq(&child->sighand->siglock);
+ clear_tsk_restore_sigmask(child);
+
ret = 0;
break;
}
--
2.17.2
On Mon, 19 Nov 2018 22:06:16 -0800 Andrei Vagin <[email protected]> wrote:
> There are a few system calls (pselect, ppoll, etc) which replace a task
> sigmask while they are running in a kernel-space
>
> When a task calls one of these syscalls, the kernel saves a current
> sigmask in task->saved_sigmask and sets a syscall sigmask.
>
> On syscall-exit-stop, ptrace traps a task before restoring the
> saved_sigmask, so PTRACE_GETSIGMASK returns the syscall sigmask and
> PTRACE_SETSIGMASK does nothing, because its sigmask is replaced by
> saved_sigmask, when the task returns to user-space.
>
> This patch fixes this problem. PTRACE_GET_SIGMASK returns saved_sigmask
> is it's set. PTRACE_SETSIGMASK drops the TIF_RESTORE_SIGMASK flag.
Looks good to me, but what would I know. I'll await input from Eric
and/or Oleg (please).
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -417,10 +417,20 @@ static inline void set_restore_sigmask(void)
> set_thread_flag(TIF_RESTORE_SIGMASK);
> WARN_ON(!test_thread_flag(TIF_SIGPENDING));
> }
> +
> +static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
> +{
> + clear_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
> +}
How irritating is it that this file uses "task" 85 times and "tsk" 19
times? What did that gain us? This patch worsens things.
Oh well.
On 11/19, Andrei Vagin wrote:
>
> case PTRACE_SETSIGMASK: {
> sigset_t new_set;
> @@ -962,6 +971,8 @@ int ptrace_request(struct task_struct *child, long request,
> child->blocked = new_set;
> spin_unlock_irq(&child->sighand->siglock);
>
> + clear_tsk_restore_sigmask(child);
> +
I am not sure I understand this change...
I forgot everything I knew about criu, but iiuc PTRACE_SETSIGMASK is used
at "restore" time, doesn't this mean that TIF_RESTORE_SIGMASK/restore_sigmask
can not be set?
IOW, could you please explain how PTRACE_SETSIGMASK should be used, and why
it doesn't do something like
if (test_tsk_restore_sigmask(child))
child->saved_sigmask = new_set;
else
child->blocked = new_set;
which looks symmetrical to PTRACE_GETSIGMASK?
Oleg.
On Thu, Nov 22, 2018 at 12:47:52PM +0100, Oleg Nesterov wrote:
> On 11/19, Andrei Vagin wrote:
> >
> > case PTRACE_SETSIGMASK: {
> > sigset_t new_set;
> > @@ -962,6 +971,8 @@ int ptrace_request(struct task_struct *child, long request,
> > child->blocked = new_set;
> > spin_unlock_irq(&child->sighand->siglock);
> >
> > + clear_tsk_restore_sigmask(child);
> > +
>
> I am not sure I understand this change...
>
> I forgot everything I knew about criu, but iiuc PTRACE_SETSIGMASK is used
> at "restore" time, doesn't this mean that TIF_RESTORE_SIGMASK/restore_sigmask
> can not be set?
PTRACE_SETSIGMASK isn't used on restore. On restore, criu generates
sigframe and calls sigreturn to restore registers, fpu state, sigmask
and resume a process. ?When the kernel constructs a signal frame, it
calls sigmask_to_save() to get a process signal mask. With this patch,
PTRACE_GETSIGMASK returns the same signal mask what is returned by
sigmask_to_save().
In CRIU, we don't need to set TIF_RESTORE_SIGMASK, because all processes
are dumped when they are in user-space.
>
> IOW, could you please explain how PTRACE_SETSIGMASK should be used, and why
> it doesn't do something like
>
CRIU uses PTRACE_SETSIGMASK?when it injects a parasite code into a
target process. In this case, we have to be sure that when the process
is resumed by PTRACE_CONT, it will not start handling signals and
executing signal handlers.
> if (test_tsk_restore_sigmask(child))
> child->saved_sigmask = new_set;
> else
> child->blocked = new_set;
>
> which looks symmetrical to PTRACE_GETSIGMASK?
If we set child->saved_sigmask, the child can start handling signals
which are not set in child->blocked.
>
> Oleg.
>
On 11/26, Andrei Vagin wrote:
>
> > IOW, could you please explain how PTRACE_SETSIGMASK should be used, and why
> > it doesn't do something like
> >
>
> CRIU uses PTRACE_SETSIGMASK?when it injects a parasite code into a
> target process. In this case, we have to be sure that when the process
> is resumed by PTRACE_CONT, it will not start handling signals and
> executing signal handlers.
So iiuc CRUI uses PTRACE_SETSIGMASK to block all signals, run the parasite
code, then restore the original sigmask.
Assuming that CRIU also turns ERESTARTNOHAND into syscall-restart (I think
it does ;) everything looks correct...
OK, I think the patch is fine.
Oleg.
This file uses "task" 85 times and "tsk" 25 times. It should be better to
choose one of these variants.
Signed-off-by: Andrei Vagin <[email protected]>
---
include/linux/sched/signal.h | 51 ++++++++++++++++++------------------
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 76b8399b17f6..0c3e396dca04 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -270,17 +270,18 @@ static inline int signal_group_exit(const struct signal_struct *sig)
extern void flush_signals(struct task_struct *);
extern void ignore_signals(struct task_struct *);
extern void flush_signal_handlers(struct task_struct *, int force_default);
-extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask, kernel_siginfo_t *info);
+extern int dequeue_signal(struct task_struct *task,
+ sigset_t *mask, kernel_siginfo_t *info);
static inline int kernel_dequeue_signal(void)
{
- struct task_struct *tsk = current;
+ struct task_struct *task = current;
kernel_siginfo_t __info;
int ret;
- spin_lock_irq(&tsk->sighand->siglock);
- ret = dequeue_signal(tsk, &tsk->blocked, &__info);
- spin_unlock_irq(&tsk->sighand->siglock);
+ spin_lock_irq(&task->sighand->siglock);
+ ret = dequeue_signal(task, &task->blocked, &__info);
+ spin_unlock_irq(&task->sighand->siglock);
return ret;
}
@@ -418,18 +419,18 @@ static inline void set_restore_sigmask(void)
WARN_ON(!test_thread_flag(TIF_SIGPENDING));
}
-static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
+static inline void clear_tsk_restore_sigmask(struct task_struct *task)
{
- clear_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
+ clear_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
}
static inline void clear_restore_sigmask(void)
{
clear_thread_flag(TIF_RESTORE_SIGMASK);
}
-static inline bool test_tsk_restore_sigmask(struct task_struct *tsk)
+static inline bool test_tsk_restore_sigmask(struct task_struct *task)
{
- return test_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
+ return test_tsk_thread_flag(task, TIF_RESTORE_SIGMASK);
}
static inline bool test_restore_sigmask(void)
{
@@ -448,9 +449,9 @@ static inline void set_restore_sigmask(void)
current->restore_sigmask = true;
WARN_ON(!test_thread_flag(TIF_SIGPENDING));
}
-static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
+static inline void clear_tsk_restore_sigmask(struct task_struct *task)
{
- tsk->restore_sigmask = false;
+ task->restore_sigmask = false;
}
static inline void clear_restore_sigmask(void)
{
@@ -460,9 +461,9 @@ static inline bool test_restore_sigmask(void)
{
return current->restore_sigmask;
}
-static inline bool test_tsk_restore_sigmask(struct task_struct *tsk)
+static inline bool test_tsk_restore_sigmask(struct task_struct *task)
{
- return tsk->restore_sigmask;
+ return task->restore_sigmask;
}
static inline bool test_and_clear_restore_sigmask(void)
{
@@ -616,9 +617,9 @@ static inline struct pid *task_session(struct task_struct *task)
return task->signal->pids[PIDTYPE_SID];
}
-static inline int get_nr_threads(struct task_struct *tsk)
+static inline int get_nr_threads(struct task_struct *task)
{
- return tsk->signal->nr_threads;
+ return task->signal->nr_threads;
}
static inline bool thread_group_leader(struct task_struct *p)
@@ -657,35 +658,35 @@ static inline int thread_group_empty(struct task_struct *p)
#define delay_group_leader(p) \
(thread_group_leader(p) && !thread_group_empty(p))
-extern struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
+extern struct sighand_struct *__lock_task_sighand(struct task_struct *task,
unsigned long *flags);
-static inline struct sighand_struct *lock_task_sighand(struct task_struct *tsk,
+static inline struct sighand_struct *lock_task_sighand(struct task_struct *task,
unsigned long *flags)
{
struct sighand_struct *ret;
- ret = __lock_task_sighand(tsk, flags);
- (void)__cond_lock(&tsk->sighand->siglock, ret);
+ ret = __lock_task_sighand(task, flags);
+ (void)__cond_lock(&task->sighand->siglock, ret);
return ret;
}
-static inline void unlock_task_sighand(struct task_struct *tsk,
+static inline void unlock_task_sighand(struct task_struct *task,
unsigned long *flags)
{
- spin_unlock_irqrestore(&tsk->sighand->siglock, *flags);
+ spin_unlock_irqrestore(&task->sighand->siglock, *flags);
}
-static inline unsigned long task_rlimit(const struct task_struct *tsk,
+static inline unsigned long task_rlimit(const struct task_struct *task,
unsigned int limit)
{
- return READ_ONCE(tsk->signal->rlim[limit].rlim_cur);
+ return READ_ONCE(task->signal->rlim[limit].rlim_cur);
}
-static inline unsigned long task_rlimit_max(const struct task_struct *tsk,
+static inline unsigned long task_rlimit_max(const struct task_struct *task,
unsigned int limit)
{
- return READ_ONCE(tsk->signal->rlim[limit].rlim_max);
+ return READ_ONCE(task->signal->rlim[limit].rlim_max);
}
static inline unsigned long rlimit(unsigned int limit)
--
2.17.2
On Wed, Nov 21, 2018 at 06:16:50PM -0800, Andrew Morton wrote:
> On Mon, 19 Nov 2018 22:06:16 -0800 Andrei Vagin <[email protected]> wrote:
>
> > There are a few system calls (pselect, ppoll, etc) which replace a task
> > sigmask while they are running in a kernel-space
> >
> > When a task calls one of these syscalls, the kernel saves a current
> > sigmask in task->saved_sigmask and sets a syscall sigmask.
> >
> > On syscall-exit-stop, ptrace traps a task before restoring the
> > saved_sigmask, so PTRACE_GETSIGMASK returns the syscall sigmask and
> > PTRACE_SETSIGMASK does nothing, because its sigmask is replaced by
> > saved_sigmask, when the task returns to user-space.
> >
> > This patch fixes this problem. PTRACE_GET_SIGMASK returns saved_sigmask
> > is it's set. PTRACE_SETSIGMASK drops the TIF_RESTORE_SIGMASK flag.
>
> Looks good to me, but what would I know. I'll await input from Eric
> and/or Oleg (please).
Hi Andrew,
What is your plan for this patch? In the ~akpm/mmotm/series, I see a
comment of waiting for ack from Oleg.
Here was a feedback from Oleg:
https://www.spinics.net/lists/kernel/msg2972154.html
where he said: "Ok, I think the patch is fine".
Is it enough or should I convince him to send a "real" ack?
Thanks,
Andrei
>
> > --- a/include/linux/sched/signal.h
> > +++ b/include/linux/sched/signal.h
> > @@ -417,10 +417,20 @@ static inline void set_restore_sigmask(void)
> > set_thread_flag(TIF_RESTORE_SIGMASK);
> > WARN_ON(!test_thread_flag(TIF_SIGPENDING));
> > }
> > +
> > +static inline void clear_tsk_restore_sigmask(struct task_struct *tsk)
> > +{
> > + clear_tsk_thread_flag(tsk, TIF_RESTORE_SIGMASK);
> > +}
>
> How irritating is it that this file uses "task" 85 times and "tsk" 19
> times? What did that gain us? This patch worsens things.
>
> Oh well.