Currently only the tracing thread can call ptrace on a given pid. This
patch allows any task in the tracing thread's thread group to also call
ptrace. This makes it easier and more performant to write multi-threaded
applications that use ptrace.
In our ptrace-based simulator, we currently work-around this limitation
by stopping and detaching inactive tracees, so that any thread in our
worker thread pool can later re-attach and resume control of the tracee.
This detaching and reattaching carries with it a significant performance
overhead. With this patch, detaching and reattaching is no longer
necessary.
Signed-off-by: James Newsome <[email protected]>
---
kernel/ptrace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 821cf1723814..bd9968a35784 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -50,7 +50,7 @@ int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
return 0;
if (!tsk->ptrace ||
- (current != tsk->parent) ||
+ !same_thread_group(current, tsk->parent) ||
((get_dumpable(mm) != SUID_DUMP_USER) &&
!ptracer_capable(tsk, mm->user_ns))) {
mmput(mm);
@@ -193,7 +193,7 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
if (task->state != __TASK_TRACED)
return;
- WARN_ON(!task->ptrace || task->parent != current);
+ WARN_ON(!task->ptrace || !same_thread_group(task->parent, current));
/*
* PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
@@ -238,7 +238,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
* be changed by us so it's not changing right after this.
*/
read_lock(&tasklist_lock);
- if (child->ptrace && child->parent == current) {
+ if (child->ptrace && same_thread_group(child->parent, current)) {
WARN_ON(child->state == __TASK_TRACED);
/*
* child->sighand can't be NULL, release_task()
--
2.30.1
Hi Jim,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.12-rc2 next-20210310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jim-Newsome/ptrace-Allow-other-threads-to-access-tracee/20210311-050039
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
config: x86_64-randconfig-s021-20210309 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-262-g5e674421-dirty
# https://github.com/0day-ci/linux/commit/874466f21257b7eb31b17d2bdb19da3f365436d7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jim-Newsome/ptrace-Allow-other-threads-to-access-tracee/20210311-050039
git checkout 874466f21257b7eb31b17d2bdb19da3f365436d7
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
"sparse warnings: (new ones prefixed by >>)"
>> kernel/ptrace.c:53:44: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected struct task_struct *p2 @@ got struct task_struct [noderef] __rcu *parent @@
kernel/ptrace.c:53:44: sparse: expected struct task_struct *p2
kernel/ptrace.c:53:44: sparse: got struct task_struct [noderef] __rcu *parent
kernel/ptrace.c:72:23: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct task_struct [noderef] __rcu *parent @@ got struct task_struct *new_parent @@
kernel/ptrace.c:72:23: sparse: expected struct task_struct [noderef] __rcu *parent
kernel/ptrace.c:72:23: sparse: got struct task_struct *new_parent
kernel/ptrace.c:73:29: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct cred const [noderef] __rcu *ptracer_cred @@ got struct cred const * @@
kernel/ptrace.c:73:29: sparse: expected struct cred const [noderef] __rcu *ptracer_cred
kernel/ptrace.c:73:29: sparse: got struct cred const *
kernel/ptrace.c:127:18: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct cred const *old_cred @@ got struct cred const [noderef] __rcu *ptracer_cred @@
kernel/ptrace.c:127:18: sparse: expected struct cred const *old_cred
kernel/ptrace.c:127:18: sparse: got struct cred const [noderef] __rcu *ptracer_cred
kernel/ptrace.c:131:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:131:25: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:131:25: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:169:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:169:27: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:169:27: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:181:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:181:28: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:181:28: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:186:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:186:30: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:186:30: sparse: got struct spinlock [noderef] __rcu *
>> kernel/ptrace.c:196:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct task_struct *p1 @@ got struct task_struct [noderef] __rcu *parent @@
kernel/ptrace.c:196:9: sparse: expected struct task_struct *p1
kernel/ptrace.c:196:9: sparse: got struct task_struct [noderef] __rcu *parent
kernel/ptrace.c:202:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:202:28: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:202:28: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:209:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:209:30: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:209:30: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:241:53: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct task_struct *p1 @@ got struct task_struct [noderef] __rcu *parent @@
kernel/ptrace.c:241:53: sparse: expected struct task_struct *p1
kernel/ptrace.c:241:53: sparse: got struct task_struct [noderef] __rcu *parent
kernel/ptrace.c:415:24: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:415:24: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:415:24: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:438:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:438:26: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:438:26: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:474:54: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct task_struct *parent @@ got struct task_struct [noderef] __rcu *parent @@
kernel/ptrace.c:474:54: sparse: expected struct task_struct *parent
kernel/ptrace.c:474:54: sparse: got struct task_struct [noderef] __rcu *parent
kernel/ptrace.c:482:53: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected struct task_struct *new_parent @@ got struct task_struct [noderef] __rcu *real_parent @@
kernel/ptrace.c:482:53: sparse: expected struct task_struct *new_parent
kernel/ptrace.c:482:53: sparse: got struct task_struct [noderef] __rcu *real_parent
kernel/ptrace.c:530:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct task_struct *p1 @@ got struct task_struct [noderef] __rcu *real_parent @@
kernel/ptrace.c:530:41: sparse: expected struct task_struct *p1
kernel/ptrace.c:530:41: sparse: got struct task_struct [noderef] __rcu *real_parent
kernel/ptrace.c:532:50: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct sighand_struct *sigh @@ got struct sighand_struct [noderef] __rcu *sighand @@
kernel/ptrace.c:532:50: sparse: expected struct sighand_struct *sigh
kernel/ptrace.c:532:50: sparse: got struct sighand_struct [noderef] __rcu *sighand
kernel/ptrace.c:734:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:734:37: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:734:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:742:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:742:39: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:742:39: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:847:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:847:37: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:847:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:851:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:851:39: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:851:39: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:1081:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:1081:37: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:1081:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:1083:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
kernel/ptrace.c:1083:39: sparse: expected struct spinlock [usertype] *lock
kernel/ptrace.c:1083:39: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:480:38: sparse: sparse: dereference of noderef expression
kernel/ptrace.c: note: in included file (through include/linux/rcuwait.h, include/linux/percpu-rwsem.h, include/linux/fs.h, ...):
include/linux/sched/signal.h:708:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:708:37: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:708:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:681:9: sparse: sparse: context imbalance in 'ptrace_getsiginfo' - different lock contexts for basic block
include/linux/sched/signal.h:708:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:708:37: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:708:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:697:9: sparse: sparse: context imbalance in 'ptrace_setsiginfo' - different lock contexts for basic block
kernel/ptrace.c:853:9: sparse: sparse: context imbalance in 'ptrace_resume' - different lock contexts for basic block
include/linux/sched/signal.h:708:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:708:37: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:708:37: sparse: got struct spinlock [noderef] __rcu *
include/linux/sched/signal.h:708:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
include/linux/sched/signal.h:708:37: sparse: expected struct spinlock [usertype] *lock
include/linux/sched/signal.h:708:37: sparse: got struct spinlock [noderef] __rcu *
kernel/ptrace.c:1229:9: sparse: sparse: context imbalance in 'ptrace_request' - different lock contexts for basic block
vim +53 kernel/ptrace.c
36
37 /*
38 * Access another process' address space via ptrace.
39 * Source/target buffer must be kernel space,
40 * Do not walk the page table directly, use get_user_pages
41 */
42 int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
43 void *buf, int len, unsigned int gup_flags)
44 {
45 struct mm_struct *mm;
46 int ret;
47
48 mm = get_task_mm(tsk);
49 if (!mm)
50 return 0;
51
52 if (!tsk->ptrace ||
> 53 !same_thread_group(current, tsk->parent) ||
54 ((get_dumpable(mm) != SUID_DUMP_USER) &&
55 !ptracer_capable(tsk, mm->user_ns))) {
56 mmput(mm);
57 return 0;
58 }
59
60 ret = __access_remote_vm(mm, addr, buf, len, gup_flags);
61 mmput(mm);
62
63 return ret;
64 }
65
66
67 void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
68 const struct cred *ptracer_cred)
69 {
70 BUG_ON(!list_empty(&child->ptrace_entry));
71 list_add(&child->ptrace_entry, &new_parent->ptraced);
72 child->parent = new_parent;
73 child->ptracer_cred = get_cred(ptracer_cred);
74 }
75
76 /*
77 * ptrace a task: make the debugger its new parent and
78 * move it to the ptrace list.
79 *
80 * Must be called with the tasklist lock write-held.
81 */
82 static void ptrace_link(struct task_struct *child, struct task_struct *new_parent)
83 {
84 __ptrace_link(child, new_parent, current_cred());
85 }
86
87 /**
88 * __ptrace_unlink - unlink ptracee and restore its execution state
89 * @child: ptracee to be unlinked
90 *
91 * Remove @child from the ptrace list, move it back to the original parent,
92 * and restore the execution state so that it conforms to the group stop
93 * state.
94 *
95 * Unlinking can happen via two paths - explicit PTRACE_DETACH or ptracer
96 * exiting. For PTRACE_DETACH, unless the ptracee has been killed between
97 * ptrace_check_attach() and here, it's guaranteed to be in TASK_TRACED.
98 * If the ptracer is exiting, the ptracee can be in any state.
99 *
100 * After detach, the ptracee should be in a state which conforms to the
101 * group stop. If the group is stopped or in the process of stopping, the
102 * ptracee should be put into TASK_STOPPED; otherwise, it should be woken
103 * up from TASK_TRACED.
104 *
105 * If the ptracee is in TASK_TRACED and needs to be moved to TASK_STOPPED,
106 * it goes through TRACED -> RUNNING -> STOPPED transition which is similar
107 * to but in the opposite direction of what happens while attaching to a
108 * stopped task. However, in this direction, the intermediate RUNNING
109 * state is not hidden even from the current ptracer and if it immediately
110 * re-attaches and performs a WNOHANG wait(2), it may fail.
111 *
112 * CONTEXT:
113 * write_lock_irq(tasklist_lock)
114 */
115 void __ptrace_unlink(struct task_struct *child)
116 {
117 const struct cred *old_cred;
118 BUG_ON(!child->ptrace);
119
120 clear_task_syscall_work(child, SYSCALL_TRACE);
121 #if defined(CONFIG_GENERIC_ENTRY) || defined(TIF_SYSCALL_EMU)
122 clear_task_syscall_work(child, SYSCALL_EMU);
123 #endif
124
125 child->parent = child->real_parent;
126 list_del_init(&child->ptrace_entry);
127 old_cred = child->ptracer_cred;
128 child->ptracer_cred = NULL;
129 put_cred(old_cred);
130
131 spin_lock(&child->sighand->siglock);
132 child->ptrace = 0;
133 /*
134 * Clear all pending traps and TRAPPING. TRAPPING should be
135 * cleared regardless of JOBCTL_STOP_PENDING. Do it explicitly.
136 */
137 task_clear_jobctl_pending(child, JOBCTL_TRAP_MASK);
138 task_clear_jobctl_trapping(child);
139
140 /*
141 * Reinstate JOBCTL_STOP_PENDING if group stop is in effect and
142 * @child isn't dead.
143 */
144 if (!(child->flags & PF_EXITING) &&
145 (child->signal->flags & SIGNAL_STOP_STOPPED ||
146 child->signal->group_stop_count)) {
147 child->jobctl |= JOBCTL_STOP_PENDING;
148
149 /*
150 * This is only possible if this thread was cloned by the
151 * traced task running in the stopped group, set the signal
152 * for the future reports.
153 * FIXME: we should change ptrace_init_task() to handle this
154 * case.
155 */
156 if (!(child->jobctl & JOBCTL_STOP_SIGMASK))
157 child->jobctl |= SIGSTOP;
158 }
159
160 /*
161 * If transition to TASK_STOPPED is pending or in TASK_TRACED, kick
162 * @child in the butt. Note that @resume should be used iff @child
163 * is in TASK_TRACED; otherwise, we might unduly disrupt
164 * TASK_KILLABLE sleeps.
165 */
166 if (child->jobctl & JOBCTL_STOP_PENDING || task_is_traced(child))
167 ptrace_signal_wake_up(child, true);
168
169 spin_unlock(&child->sighand->siglock);
170 }
171
172 /* Ensure that nothing can wake it up, even SIGKILL */
173 static bool ptrace_freeze_traced(struct task_struct *task)
174 {
175 bool ret = false;
176
177 /* Lockless, nobody but us can set this flag */
178 if (task->jobctl & JOBCTL_LISTENING)
179 return ret;
180
181 spin_lock_irq(&task->sighand->siglock);
182 if (task_is_traced(task) && !__fatal_signal_pending(task)) {
183 task->state = __TASK_TRACED;
184 ret = true;
185 }
186 spin_unlock_irq(&task->sighand->siglock);
187
188 return ret;
189 }
190
191 static void ptrace_unfreeze_traced(struct task_struct *task)
192 {
193 if (task->state != __TASK_TRACED)
194 return;
195
> 196 WARN_ON(!task->ptrace || !same_thread_group(task->parent, current));
197
198 /*
199 * PTRACE_LISTEN can allow ptrace_trap_notify to wake us up remotely.
200 * Recheck state under the lock to close this race.
201 */
202 spin_lock_irq(&task->sighand->siglock);
203 if (task->state == __TASK_TRACED) {
204 if (__fatal_signal_pending(task))
205 wake_up_state(task, __TASK_TRACED);
206 else
207 task->state = TASK_TRACED;
208 }
209 spin_unlock_irq(&task->sighand->siglock);
210 }
211
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 03/10, Jim Newsome wrote:
>
> @@ -238,7 +238,7 @@ static int ptrace_check_attach(struct task_struct *child, bool ignore_state)
> * be changed by us so it's not changing right after this.
> */
> read_lock(&tasklist_lock);
> - if (child->ptrace && child->parent == current) {
> + if (child->ptrace && same_thread_group(child->parent, current)) {
Cough... it is not that simple.
Just suppose that 2 threads call ptrace(tracee) at the same time. Say, the 1st
thread does PTRACE_CONT while the 2nd thread tries to change the registers.
Oleg.
On 3/11/21 09:21, Oleg Nesterov wrote:
> Cough... it is not that simple.
Yes, I was afraid of that :)
> Just suppose that 2 threads call ptrace(tracee) at the same time. Say, the 1st
> thread does PTRACE_CONT while the 2nd thread tries to change the registers.
Is it acceptable for the new register-values to be lost, or even
corrupted, in this case? From my perspective it is, if the tracer failed
to synchronize itself, but maybe there's an overarching philosophy that
syscalls should be "atomic"?
I suppose even if the corruption of the register-values-themselves is
acceptable, some synchronization may be needed to avoid the possibility
of corrupting the kernel's data structures?
Is it "just" a matter of adding some locking? Would a relatively coarse
lock on the target task over the duration of the ptrace call (which I
believe is always non-blocking?) be acceptable, or would we need finer
grained locking in places where we actually touch the target task? And
do you have a feel for whether you'd be inclined to accept such a patch
once that (or whatever actually is needed) is added?
Thanks!
-Jim
On 03/11, Jim Newsome wrote:
>
> I suppose even if the corruption of the register-values-themselves is
> acceptable, some synchronization may be needed to avoid the possibility
> of corrupting the kernel's data structures?
Yes, the kernel can crash. Just look at the comment above ptrace_freeze_traced().
The kernel assumes that the tracee is frozen, in particular it can't exit.
Say, ptrace_peek_siginfo() can crash the tracee exits and clears ->sighand,
and this can obviously happen if another thread does PTRACE_CONT + SIGKILL.
> Is it "just" a matter of adding some locking? Would a relatively coarse
> lock on the target task over the duration of the ptrace call
Yes I think needs a mutex in task_struct. But honestly I am not sure
it makes sense.... I dunno.
> (which I
> believe is always non-blocking?)
Why? It is blocking.
Oleg.