* [PATCH AUTOSEL 5.4 1/2] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly
@ 2021-05-18 1:10 Sasha Levin
2021-05-18 1:10 ` [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails Sasha Levin
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2021-05-18 1:10 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Oleg Nesterov, Simon Marchi, Eric W. Biederman, Pedro Alves,
Jan Kratochvil, Linus Torvalds, Sasha Levin
From: Oleg Nesterov <oleg@redhat.com>
[ Upstream commit dbb5afad100a828c97e012c6106566d99f041db6 ]
Suppose we have 2 threads, the group-leader L and a sub-theread T,
both parked in ptrace_stop(). Debugger tries to resume both threads
and does
ptrace(PTRACE_CONT, T);
ptrace(PTRACE_CONT, L);
If the sub-thread T execs in between, the 2nd PTRACE_CONT doesn not
resume the old leader L, it resumes the post-exec thread T which was
actually now stopped in PTHREAD_EVENT_EXEC. In this case the
PTHREAD_EVENT_EXEC event is lost, and the tracer can't know that the
tracee changed its pid.
This patch makes ptrace() fail in this case until debugger does wait()
and consumes PTHREAD_EVENT_EXEC which reports old_pid. This affects all
ptrace requests except the "asynchronous" PTRACE_INTERRUPT/KILL.
The patch doesn't add the new PTRACE_ option to not complicate the API,
and I _hope_ this won't cause any noticeable regression:
- If debugger uses PTRACE_O_TRACEEXEC and the thread did an exec
and the tracer does a ptrace request without having consumed
the exec event, it's 100% sure that the thread the ptracer
thinks it is targeting does not exist anymore, or isn't the
same as the one it thinks it is targeting.
- To some degree this patch adds nothing new. In the scenario
above ptrace(L) can fail with -ESRCH if it is called after the
execing sub-thread wakes the leader up and before it "steals"
the leader's pid.
Test-case:
#include <stdio.h>
#include <unistd.h>
#include <signal.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <errno.h>
#include <pthread.h>
#include <assert.h>
void *tf(void *arg)
{
execve("/usr/bin/true", NULL, NULL);
assert(0);
return NULL;
}
int main(void)
{
int leader = fork();
if (!leader) {
kill(getpid(), SIGSTOP);
pthread_t th;
pthread_create(&th, NULL, tf, NULL);
for (;;)
pause();
return 0;
}
waitpid(leader, NULL, WSTOPPED);
ptrace(PTRACE_SEIZE, leader, 0,
PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC);
waitpid(leader, NULL, 0);
ptrace(PTRACE_CONT, leader, 0,0);
waitpid(leader, NULL, 0);
int status, thread = waitpid(-1, &status, 0);
assert(thread > 0 && thread != leader);
assert(status == 0x80137f);
ptrace(PTRACE_CONT, thread, 0,0);
/*
* waitid() because waitpid(leader, &status, WNOWAIT) does not
* report status. Why ????
*
* Why WEXITED? because we have another kernel problem connected
* to mt-exec.
*/
siginfo_t info;
assert(waitid(P_PID, leader, &info, WSTOPPED|WEXITED|WNOWAIT) == 0);
assert(info.si_pid == leader && info.si_status == 0x0405);
/* OK, it sleeps in ptrace(PTRACE_EVENT_EXEC == 0x04) */
assert(ptrace(PTRACE_CONT, leader, 0,0) == -1);
assert(errno == ESRCH);
assert(leader == waitpid(leader, &status, WNOHANG));
assert(status == 0x04057f);
assert(ptrace(PTRACE_CONT, leader, 0,0) == 0);
return 0;
}
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reported-by: Simon Marchi <simon.marchi@efficios.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Acked-by: Pedro Alves <palves@redhat.com>
Acked-by: Simon Marchi <simon.marchi@efficios.com>
Acked-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
kernel/ptrace.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 79de1294f8eb..eb4d04cb3aaf 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -169,6 +169,21 @@ void __ptrace_unlink(struct task_struct *child)
spin_unlock(&child->sighand->siglock);
}
+static bool looks_like_a_spurious_pid(struct task_struct *task)
+{
+ if (task->exit_code != ((PTRACE_EVENT_EXEC << 8) | SIGTRAP))
+ return false;
+
+ if (task_pid_vnr(task) == task->ptrace_message)
+ return false;
+ /*
+ * The tracee changed its pid but the PTRACE_EVENT_EXEC event
+ * was not wait()'ed, most probably debugger targets the old
+ * leader which was destroyed in de_thread().
+ */
+ return true;
+}
+
/* Ensure that nothing can wake it up, even SIGKILL */
static bool ptrace_freeze_traced(struct task_struct *task)
{
@@ -179,7 +194,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
return ret;
spin_lock_irq(&task->sighand->siglock);
- if (task_is_traced(task) && !__fatal_signal_pending(task)) {
+ if (task_is_traced(task) && !looks_like_a_spurious_pid(task) &&
+ !__fatal_signal_pending(task)) {
task->state = __TASK_TRACED;
ret = true;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails
2021-05-18 1:10 [PATCH AUTOSEL 5.4 1/2] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly Sasha Levin
@ 2021-05-18 1:10 ` Sasha Levin
2021-05-18 4:27 ` Chaitanya Kulkarni
0 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2021-05-18 1:10 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Daniel Wagner, Enzo Matsumiya, Christoph Hellwig, Sasha Levin,
linux-nvme
From: Daniel Wagner <dwagner@suse.de>
[ Upstream commit 85428beac80dbcace5b146b218697c73e367dcf5 ]
Reset the ns->file value to NULL also in the error case in
nvmet_file_ns_enable().
The ns->file variable points either to file object or contains the
error code after the filp_open() call. This can lead to following
problem:
When the user first setups an invalid file backend and tries to enable
the ns, it will fail. Then the user switches over to a bdev backend
and enables successfully the ns. The first received I/O will crash the
system because the IO backend is chosen based on the ns->file value:
static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
{
[...]
if (req->ns->file)
return nvmet_file_parse_io_cmd(req);
return nvmet_bdev_parse_io_cmd(req);
}
Reported-by: Enzo Matsumiya <ematsumiya@suse.com>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/nvme/target/io-cmd-file.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 05453f5d1448..6ca17a0babae 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -38,9 +38,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
ns->file = filp_open(ns->device_path, flags, 0);
if (IS_ERR(ns->file)) {
- pr_err("failed to open file %s: (%ld)\n",
- ns->device_path, PTR_ERR(ns->file));
- return PTR_ERR(ns->file);
+ ret = PTR_ERR(ns->file);
+ pr_err("failed to open file %s: (%d)\n",
+ ns->device_path, ret);
+ ns->file = NULL;
+ return ret;
}
ret = vfs_getattr(&ns->file->f_path,
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails
2021-05-18 1:10 ` [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails Sasha Levin
@ 2021-05-18 4:27 ` Chaitanya Kulkarni
2021-05-18 6:37 ` Daniel Wagner
0 siblings, 1 reply; 4+ messages in thread
From: Chaitanya Kulkarni @ 2021-05-18 4:27 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel, stable, Daniel Wagner, Enzo Matsumiya,
Christoph Hellwig, linux-nvme
Sasha,
On 5/17/21 18:20, Sasha Levin wrote:
> From: Daniel Wagner <dwagner@suse.de>
>
> [ Upstream commit 85428beac80dbcace5b146b218697c73e367dcf5 ]
>
> Reset the ns->file value to NULL also in the error case in
> nvmet_file_ns_enable().
>
> The ns->file variable points either to file object or contains the
> error code after the filp_open() call. This can lead to following
> problem:
>
> When the user first setups an invalid file backend and tries to enable
> the ns, it will fail. Then the user switches over to a bdev backend
> and enables successfully the ns. The first received I/O will crash the
> system because the IO backend is chosen based on the ns->file value:
I think the patch subject line is being worked on since it needs to be
reset and not seset.
Not sure how we can go about fixing that.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails
2021-05-18 4:27 ` Chaitanya Kulkarni
@ 2021-05-18 6:37 ` Daniel Wagner
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Wagner @ 2021-05-18 6:37 UTC (permalink / raw)
To: Chaitanya Kulkarni
Cc: Sasha Levin, linux-kernel, stable, Enzo Matsumiya,
Christoph Hellwig, linux-nvme
Hi Chaitanya,
On Tue, May 18, 2021 at 04:27:32AM +0000, Chaitanya Kulkarni wrote:
> I think the patch subject line is being worked on since it needs to be
> reset and not seset.
>
> Not sure how we can go about fixing that.
This ship has sailed, as the commit already hit mainline. Fixing the
typo in the back ports is surely possible but I assume it's better not
do change the subject line. seset forever!
Thanks,
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-18 6:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 1:10 [PATCH AUTOSEL 5.4 1/2] ptrace: make ptrace() fail if the tracee changed its pid unexpectedly Sasha Levin
2021-05-18 1:10 ` [PATCH AUTOSEL 5.4 2/2] nvmet: seset ns->file when open fails Sasha Levin
2021-05-18 4:27 ` Chaitanya Kulkarni
2021-05-18 6:37 ` Daniel Wagner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).