Skip to contentSkip to conversation

ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK

PATCH Superseded Andrei Vagin 3 participants 7 messages Nov 20, 2018 – Feb 2, 2019 ptrace support

lore
Share & tools
lore

Andrei Vagin

author
#0 lore raw

PATCH 1/1 ptrace: take into account saved_sigmask in PTRACE_{GET,SET}SIGMASK

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(-)
include/linux/sched/signal.h +18 -0
index 1be35729c2c5..660d78c9af6c 100644
@@ -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)
kernel/ptrace.c +13 -2
index 21fec73d45d4..fc0d667f5792 100644
@@ -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;
 	}
Signature · 1 line
2.17.2
#1 lore raw
Quoted 6 lines · 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.

Quoted 7 lines

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

include/linux/sched/signal.h static inline void set_restore_sigmask(void) · line 424

Quoted 11 lines
include/linux/sched/signal.h +5 -0
@@ -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.

author
#2 lore raw

PATCH 1/1 include: replace tsk to task in linux/sched/signal.h

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(-)
include/linux/sched/signal.h +26 -25
index 76b8399b17f6..0c3e396dca04 100644
@@ -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)
Signature · 1 line
2.17.2
author
#3 lore raw
In reply to Andrew Morton
Quoted 13 lines · Andrew Morton
Quoted 11 lines · 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.

Quoted 5 lines
Quoted 2 lines

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

Quoted 17 lines

include/linux/sched/signal.h static inline void set_restore_sigmask(void) · line 424

Quoted 12 lines
include/linux/sched/signal.h +5 -0
@@ -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.

maintainer
#4 lore raw
In reply to Andrei Vagin

kernel/ptrace.c int ptrace_request(struct task_struct *child, long request, · line 975

Quoted 9 lines · Andrei Vagin
kernel/ptrace.c +2 -0
 	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.

author
#5 lore raw
Quoted 8 lines · Oleg Nesterov
Quoted 7 lines · Andrei Vagin
kernel/ptrace.c +0 -0
 	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);
Quoted 8 lines

kernel/ptrace.c int ptrace_request(struct task_struct *child, long request, · line 975

Quoted 2 lines
kernel/ptrace.c +2 -0
+		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.

Quoted 4 lines

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.

Quoted 6 lines
	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.

Quoted 3 lines

Oleg.

maintainer
#6 lore raw
Quoted 9 lines · Andrei Vagin
Quoted 4 lines

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.