All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
@ 2018-11-20  6:06 Andrei Vagin
  2018-11-22  2:16 ` Andrew Morton
  2018-11-22 11:47 ` Oleg Nesterov
  0 siblings, 2 replies; 7+ messages in thread
From: Andrei Vagin @ 2018-11-20  6:06 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, Andrei Vagin, Eric W. Biederman, Andrew Morton

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 <oleg@redhat.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 29000caecbe8 ("ptrace: add ability to get/set signal-blocked mask")
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
  2018-11-20  6:06 [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
@ 2018-11-22  2:16 ` Andrew Morton
  2018-11-29 18:05   ` [PATCH] include: replace tsk to task in linux/sched/signal.h Andrei Vagin
  2019-02-02 10:04   ` [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
  2018-11-22 11:47 ` Oleg Nesterov
  1 sibling, 2 replies; 7+ messages in thread
From: Andrew Morton @ 2018-11-22  2:16 UTC (permalink / raw)
  To: Andrei Vagin; +Cc: Oleg Nesterov, linux-kernel, Eric W. Biederman

On Mon, 19 Nov 2018 22:06:16 -0800 Andrei Vagin <avagin@gmail.com> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
  2018-11-20  6:06 [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
  2018-11-22  2:16 ` Andrew Morton
@ 2018-11-22 11:47 ` Oleg Nesterov
  2018-11-27  6:38   ` Andrei Vagin
  1 sibling, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2018-11-22 11:47 UTC (permalink / raw)
  To: Andrei Vagin; +Cc: linux-kernel, Eric W. Biederman, Andrew Morton

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
  2018-11-22 11:47 ` Oleg Nesterov
@ 2018-11-27  6:38   ` Andrei Vagin
  2018-11-27 16:23     ` Oleg Nesterov
  0 siblings, 1 reply; 7+ messages in thread
From: Andrei Vagin @ 2018-11-27  6:38 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: linux-kernel, Eric W. Biederman, Andrew Morton

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
  2018-11-27  6:38   ` Andrei Vagin
@ 2018-11-27 16:23     ` Oleg Nesterov
  0 siblings, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2018-11-27 16:23 UTC (permalink / raw)
  To: Andrei Vagin; +Cc: linux-kernel, Eric W. Biederman, Andrew Morton

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] include: replace tsk to task in linux/sched/signal.h
  2018-11-22  2:16 ` Andrew Morton
@ 2018-11-29 18:05   ` Andrei Vagin
  2019-02-02 10:04   ` [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
  1 sibling, 0 replies; 7+ messages in thread
From: Andrei Vagin @ 2018-11-29 18:05 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Oleg Nesterov, linux-kernel, Eric W. Biederman, Andrei Vagin

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 <avagin@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK
  2018-11-22  2:16 ` Andrew Morton
  2018-11-29 18:05   ` [PATCH] include: replace tsk to task in linux/sched/signal.h Andrei Vagin
@ 2019-02-02 10:04   ` Andrei Vagin
  1 sibling, 0 replies; 7+ messages in thread
From: Andrei Vagin @ 2019-02-02 10:04 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Oleg Nesterov, linux-kernel, Eric W. Biederman

On Wed, Nov 21, 2018 at 06:16:50PM -0800, Andrew Morton wrote:
> On Mon, 19 Nov 2018 22:06:16 -0800 Andrei Vagin <avagin@gmail.com> 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-02-02 10:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20  6:06 [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
2018-11-22  2:16 ` Andrew Morton
2018-11-29 18:05   ` [PATCH] include: replace tsk to task in linux/sched/signal.h Andrei Vagin
2019-02-02 10:04   ` [PATCH] ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK Andrei Vagin
2018-11-22 11:47 ` Oleg Nesterov
2018-11-27  6:38   ` Andrei Vagin
2018-11-27 16:23     ` Oleg Nesterov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.