linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ptrace: Allow other threads to access tracee
@ 2021-03-10 20:59 Jim Newsome
  2021-03-11  1:31 ` kernel test robot
  2021-03-11 15:21 ` Oleg Nesterov
  0 siblings, 2 replies; 5+ messages in thread
From: Jim Newsome @ 2021-03-10 20:59 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: linux-kernel, Jim Newsome

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 <jnewsome@torproject.org>
---
 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


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

* Re: [PATCH] ptrace: Allow other threads to access tracee
  2021-03-10 20:59 [PATCH] ptrace: Allow other threads to access tracee Jim Newsome
@ 2021-03-11  1:31 ` kernel test robot
  2021-03-11 15:21 ` Oleg Nesterov
  1 sibling, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-03-11  1:31 UTC (permalink / raw)
  To: Jim Newsome, Oleg Nesterov; +Cc: kbuild-all, linux-kernel, Jim Newsome

[-- Attachment #1: Type: text/plain, Size: 18838 bytes --]

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 <lkp@intel.com>


"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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30044 bytes --]

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

* Re: [PATCH] ptrace: Allow other threads to access tracee
  2021-03-10 20:59 [PATCH] ptrace: Allow other threads to access tracee Jim Newsome
  2021-03-11  1:31 ` kernel test robot
@ 2021-03-11 15:21 ` Oleg Nesterov
  2021-03-11 16:49   ` Jim Newsome
  1 sibling, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2021-03-11 15:21 UTC (permalink / raw)
  To: Jim Newsome; +Cc: linux-kernel

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.


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

* Re: [PATCH] ptrace: Allow other threads to access tracee
  2021-03-11 15:21 ` Oleg Nesterov
@ 2021-03-11 16:49   ` Jim Newsome
  2021-03-12 17:07     ` Oleg Nesterov
  0 siblings, 1 reply; 5+ messages in thread
From: Jim Newsome @ 2021-03-11 16:49 UTC (permalink / raw)
  To: Oleg Nesterov; +Cc: linux-kernel

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


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

* Re: [PATCH] ptrace: Allow other threads to access tracee
  2021-03-11 16:49   ` Jim Newsome
@ 2021-03-12 17:07     ` Oleg Nesterov
  0 siblings, 0 replies; 5+ messages in thread
From: Oleg Nesterov @ 2021-03-12 17:07 UTC (permalink / raw)
  To: Jim Newsome; +Cc: linux-kernel

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.


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

end of thread, other threads:[~2021-03-12 17:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10 20:59 [PATCH] ptrace: Allow other threads to access tracee Jim Newsome
2021-03-11  1:31 ` kernel test robot
2021-03-11 15:21 ` Oleg Nesterov
2021-03-11 16:49   ` Jim Newsome
2021-03-12 17:07     ` Oleg Nesterov

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