All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 22:33 kernel test robot
  0 siblings, 0 replies; 29+ messages in thread
From: kernel test robot @ 2022-04-28 22:33 UTC (permalink / raw)
  To: kbuild

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

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220426225211.308418-6-ebiederm@xmission.com>
References: <20220426225211.308418-6-ebiederm@xmission.com>
TO: "Eric W. Biederman" <ebiederm@xmission.com>
TO: linux-kernel(a)vger.kernel.org
CC: rjw(a)rjwysocki.net
CC: Oleg Nesterov <oleg@redhat.com>
CC: mingo(a)kernel.org
CC: vincent.guittot(a)linaro.org
CC: dietmar.eggemann(a)arm.com
CC: rostedt(a)goodmis.org
CC: mgorman(a)suse.de
CC: bigeasy(a)linutronix.de
CC: Will Deacon <will@kernel.org>
CC: tj(a)kernel.org
CC: linux-pm(a)vger.kernel.org
CC: Peter Zijlstra <peterz@infradead.org>
CC: Richard Weinberger <richard@nod.at>
CC: Anton Ivanov <anton.ivanov@cambridgegreys.com>
CC: Johannes Berg <johannes@sipsolutions.net>
CC: linux-um(a)lists.infradead.org
CC: Chris Zankel <chris@zankel.net>
CC: Max Filippov <jcmvbkbc@gmail.com>
CC: inux-xtensa(a)linux-xtensa.org
CC: Kees Cook <keescook@chromium.org>
CC: Jann Horn <jannh@google.com>
CC: "Eric W. Biederman" <ebiederm@xmission.com>

Hi "Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tip/timers/core linus/master v5.18-rc4 next-20220428]
[cannot apply to linux/master]
[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/intel-lab-lkp/linux/commits/Eric-W-Biederman/signal-Rename-send_signal-send_signal_locked/20220427-065551
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: parisc-randconfig-s031-20220425 (https://download.01.org/0day-ci/archive/20220429/202204290612.ieU6Djcy-lkp(a)intel.com/config)
compiler: hppa-linux-gcc (GCC) 11.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/1d8ae697c0ac6bf1f99f694c9976ceac8a336f4b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eric-W-Biederman/signal-Rename-send_signal-send_signal_locked/20220427-065551
        git checkout 1d8ae697c0ac6bf1f99f694c9976ceac8a336f4b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=parisc SHELL=/bin/bash

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/signal.c: note: in included file (through arch/parisc/include/uapi/asm/signal.h, arch/parisc/include/asm/signal.h, include/uapi/linux/signal.h, ...):
   include/uapi/asm-generic/signal-defs.h:83:29: sparse: sparse: multiple address spaces given
   kernel/signal.c:195:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:195:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:195:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:198:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:198:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:198:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:480:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:480:9: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:480:9: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:484:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:484:34: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:484:34: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:542:53: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected struct k_sigaction *ka @@     got struct k_sigaction [noderef] __rcu * @@
   kernel/signal.c:542:53: sparse:     expected struct k_sigaction *ka
   kernel/signal.c:542:53: sparse:     got struct k_sigaction [noderef] __rcu *
   include/uapi/asm-generic/signal-defs.h:83:29: sparse: sparse: multiple address spaces given
   kernel/signal.c:887:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct arch_spinlock_t [usertype] *x @@     got struct arch_spinlock_t [noderef] __rcu * @@
   kernel/signal.c:887:9: sparse:     expected struct arch_spinlock_t [usertype] *x
   kernel/signal.c:887:9: sparse:     got struct arch_spinlock_t [noderef] __rcu *
   kernel/signal.c:1082:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct arch_spinlock_t [usertype] *x @@     got struct arch_spinlock_t [noderef] __rcu * @@
   kernel/signal.c:1082:9: sparse:     expected struct arch_spinlock_t [usertype] *x
   kernel/signal.c:1082:9: sparse:     got struct arch_spinlock_t [noderef] __rcu *
   kernel/signal.c:1257:9: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: cannot dereference this type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: cannot dereference this type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: cannot dereference this type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: cannot dereference this type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: cannot dereference this type
   kernel/signal.c:1263:29: sparse: sparse: no member 'ip' in struct pt_regs
   kernel/signal.c:1263:29: sparse: sparse: cast from unknown type
   kernel/signal.c:1263:29: sparse: sparse: incompatible types for 'case' statement
   kernel/signal.c:1263:29: sparse: sparse: incompatible types for 'case' statement
   kernel/signal.c:1263:29: sparse: sparse: incompatible types for 'case' statement
   kernel/signal.c:1263:29: sparse: sparse: incompatible types for 'case' statement
   kernel/signal.c:1324:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:1324:9: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:1324:9: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:1325:16: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct k_sigaction *action @@     got struct k_sigaction [noderef] __rcu * @@
   kernel/signal.c:1325:16: sparse:     expected struct k_sigaction *action
   kernel/signal.c:1325:16: sparse:     got struct k_sigaction [noderef] __rcu *
   kernel/signal.c:1345:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:1345:34: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:1345:34: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:1923:36: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:1923:36: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:1923:36: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2033:44: sparse: sparse: cast removes address space '__rcu' of expression
   kernel/signal.c:2052:65: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct task_struct *tsk @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/signal.c:2052:65: sparse:     expected struct task_struct *tsk
   kernel/signal.c:2052:65: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/signal.c:2053:40: sparse: sparse: cast removes address space '__rcu' of expression
   kernel/signal.c:2071:14: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct sighand_struct *psig @@     got struct sighand_struct [noderef] __rcu *[noderef] __rcu sighand @@
   kernel/signal.c:2071:14: sparse:     expected struct sighand_struct *psig
   kernel/signal.c:2071:14: sparse:     got struct sighand_struct [noderef] __rcu *[noderef] __rcu sighand
   kernel/signal.c:2100:53: sparse: sparse: incorrect type in argument 3 (different address spaces) @@     expected struct task_struct *t @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/signal.c:2100:53: sparse:     expected struct task_struct *t
   kernel/signal.c:2100:53: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/signal.c:2101:34: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected struct task_struct *parent @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/signal.c:2101:34: sparse:     expected struct task_struct *parent
   kernel/signal.c:2101:34: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/signal.c:2129:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct arch_spinlock_t [usertype] *x @@     got struct arch_spinlock_t [noderef] __rcu * @@
   kernel/signal.c:2129:9: sparse:     expected struct arch_spinlock_t [usertype] *x
   kernel/signal.c:2129:9: sparse:     got struct arch_spinlock_t [noderef] __rcu *
   kernel/signal.c:2132:24: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct task_struct *parent @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/signal.c:2132:24: sparse:     expected struct task_struct *parent
   kernel/signal.c:2132:24: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/signal.c:2135:24: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct task_struct *parent @@     got struct task_struct [noderef] __rcu *real_parent @@
   kernel/signal.c:2135:24: sparse:     expected struct task_struct *parent
   kernel/signal.c:2135:24: sparse:     got struct task_struct [noderef] __rcu *real_parent
   kernel/signal.c:2168:17: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct sighand_struct *sighand @@     got struct sighand_struct [noderef] __rcu *sighand @@
   kernel/signal.c:2168:17: sparse:     expected struct sighand_struct *sighand
   kernel/signal.c:2168:17: sparse:     got struct sighand_struct [noderef] __rcu *sighand
   kernel/signal.c:2169:29: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> kernel/signal.c:2169:29: sparse:    struct sighand_struct [noderef] __rcu *
   kernel/signal.c:2169:29: sparse:    struct sighand_struct *
   kernel/signal.c:2211:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2211:41: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2211:41: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2213:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2213:39: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2213:39: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2290:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2290:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2290:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2302:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2302:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2302:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2342:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2342:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2342:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2344:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2344:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2344:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2441:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2441:41: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2441:41: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2510:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2510:41: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2510:41: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2522:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2522:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2522:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2557:52: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct task_struct *tsk @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/signal.c:2557:52: sparse:     expected struct task_struct *tsk
   kernel/signal.c:2557:52: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/signal.c:2559:49: sparse: sparse: cast removes address space '__rcu' of expression
   kernel/signal.c:2597:49: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected struct sighand_struct *sighand @@     got struct sighand_struct [noderef] __rcu *sighand @@
   kernel/signal.c:2597:49: sparse:     expected struct sighand_struct *sighand
   kernel/signal.c:2597:49: sparse:     got struct sighand_struct [noderef] __rcu *sighand
   kernel/signal.c:2918:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2918:27: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2918:27: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2942:29: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2942:29: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2942:29: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:2999:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:2999:27: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:2999:27: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3001:29: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3001:29: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3001:29: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3152:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3152:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3152:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3155:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3155:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3155:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3542:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3542:27: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3542:27: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3554:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3554:37: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3554:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3559:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3559:35: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3559:35: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:3564:29: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:3564:29: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:3564:29: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:4018:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:4018:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:4018:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:4030:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:4030:33: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:4030:33: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:4048:11: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct k_sigaction *k @@     got struct k_sigaction [noderef] __rcu * @@
   kernel/signal.c:4048:11: sparse:     expected struct k_sigaction *k
   kernel/signal.c:4048:11: sparse:     got struct k_sigaction [noderef] __rcu *
   kernel/signal.c:4050:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:4050:25: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:4050:25: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:4052:35: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:4052:35: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:4052:35: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:4100:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/signal.c:4100:27: sparse:     expected struct spinlock [usertype] *lock
   kernel/signal.c:4100:27: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:69:34: sparse: sparse: dereference of noderef expression
   kernel/signal.c:529:35: sparse: sparse: dereference of noderef expression
   kernel/signal.c:557:52: sparse: sparse: dereference of noderef expression
   kernel/signal.c:1034:13: sparse: sparse: dereference of noderef expression
   kernel/signal.c: note: in included file (through include/linux/sched/cputime.h):
   include/linux/sched/signal.h:731: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:731:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:731:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/signal.c:1295:9: sparse: sparse: context imbalance in 'do_send_sig_info' - different lock contexts for basic block
   kernel/signal.c: note: in included file (through include/linux/rbtree.h, include/linux/mm_types.h, include/linux/mmzone.h, ...):
   include/linux/rcupdate.h:725:9: sparse: sparse: context imbalance in '__lock_task_sighand' - different lock contexts for basic block
   kernel/signal.c: note: in included file (through include/linux/sched/cputime.h):
   include/linux/sched/signal.h:731: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:731:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:731:37: sparse:     got struct spinlock [noderef] __rcu *
   include/linux/sched/signal.h:731: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:731:37: sparse:     expected struct spinlock [usertype] *lock

vim +2169 kernel/signal.c

^1da177e4c3f41 Linus Torvalds      2005-04-16  2106  
75b95953a56969 Tejun Heo           2011-03-23  2107  /**
75b95953a56969 Tejun Heo           2011-03-23  2108   * do_notify_parent_cldstop - notify parent of stopped/continued state change
75b95953a56969 Tejun Heo           2011-03-23  2109   * @tsk: task reporting the state change
75b95953a56969 Tejun Heo           2011-03-23  2110   * @for_ptracer: the notification is for ptracer
75b95953a56969 Tejun Heo           2011-03-23  2111   * @why: CLD_{CONTINUED|STOPPED|TRAPPED} to report
75b95953a56969 Tejun Heo           2011-03-23  2112   *
75b95953a56969 Tejun Heo           2011-03-23  2113   * Notify @tsk's parent that the stopped/continued state has changed.  If
75b95953a56969 Tejun Heo           2011-03-23  2114   * @for_ptracer is %false, @tsk's group leader notifies to its real parent.
75b95953a56969 Tejun Heo           2011-03-23  2115   * If %true, @tsk reports to @tsk->parent which should be the ptracer.
75b95953a56969 Tejun Heo           2011-03-23  2116   *
75b95953a56969 Tejun Heo           2011-03-23  2117   * CONTEXT:
75b95953a56969 Tejun Heo           2011-03-23  2118   * Must be called with tasklist_lock at least read locked.
75b95953a56969 Tejun Heo           2011-03-23  2119   */
75b95953a56969 Tejun Heo           2011-03-23  2120  static void do_notify_parent_cldstop(struct task_struct *tsk,
75b95953a56969 Tejun Heo           2011-03-23  2121  				     bool for_ptracer, int why)
^1da177e4c3f41 Linus Torvalds      2005-04-16  2122  {
ae7795bc6187a1 Eric W. Biederman   2018-09-25  2123  	struct kernel_siginfo info;
bc505a478d3fff Oleg Nesterov       2005-09-06  2124  	struct task_struct *parent;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2125  	struct sighand_struct *sighand;
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2126  	bool lock;
bde8285e5cf784 Frederic Weisbecker 2017-01-31  2127  	u64 utime, stime;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2128  
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2129  	assert_spin_locked(&tsk->sighand->siglock);
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2130  
75b95953a56969 Tejun Heo           2011-03-23  2131  	if (for_ptracer) {
bc505a478d3fff Oleg Nesterov       2005-09-06  2132  		parent = tsk->parent;
75b95953a56969 Tejun Heo           2011-03-23  2133  	} else {
bc505a478d3fff Oleg Nesterov       2005-09-06  2134  		tsk = tsk->group_leader;
bc505a478d3fff Oleg Nesterov       2005-09-06  2135  		parent = tsk->real_parent;
bc505a478d3fff Oleg Nesterov       2005-09-06  2136  	}
bc505a478d3fff Oleg Nesterov       2005-09-06  2137  
faf1f22b61f271 Eric W. Biederman   2018-01-05  2138  	clear_siginfo(&info);
^1da177e4c3f41 Linus Torvalds      2005-04-16  2139  	info.si_signo = SIGCHLD;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2140  	info.si_errno = 0;
b488893a390edf Pavel Emelyanov     2007-10-18  2141  	/*
5aba085ededa6c Randy Dunlap        2011-04-04  2142  	 * see comment in do_notify_parent() about the following 4 lines
b488893a390edf Pavel Emelyanov     2007-10-18  2143  	 */
b488893a390edf Pavel Emelyanov     2007-10-18  2144  	rcu_read_lock();
17cf22c33e1f1b Eric W. Biederman   2010-03-02  2145  	info.si_pid = task_pid_nr_ns(tsk, task_active_pid_ns(parent));
54ba47edac9009 Eric W. Biederman   2012-03-13  2146  	info.si_uid = from_kuid_munged(task_cred_xxx(parent, user_ns), task_uid(tsk));
b488893a390edf Pavel Emelyanov     2007-10-18  2147  	rcu_read_unlock();
b488893a390edf Pavel Emelyanov     2007-10-18  2148  
bde8285e5cf784 Frederic Weisbecker 2017-01-31  2149  	task_cputime(tsk, &utime, &stime);
bde8285e5cf784 Frederic Weisbecker 2017-01-31  2150  	info.si_utime = nsec_to_clock_t(utime);
bde8285e5cf784 Frederic Weisbecker 2017-01-31  2151  	info.si_stime = nsec_to_clock_t(stime);
^1da177e4c3f41 Linus Torvalds      2005-04-16  2152  
^1da177e4c3f41 Linus Torvalds      2005-04-16  2153   	info.si_code = why;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2154   	switch (why) {
^1da177e4c3f41 Linus Torvalds      2005-04-16  2155   	case CLD_CONTINUED:
^1da177e4c3f41 Linus Torvalds      2005-04-16  2156   		info.si_status = SIGCONT;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2157   		break;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2158   	case CLD_STOPPED:
^1da177e4c3f41 Linus Torvalds      2005-04-16  2159   		info.si_status = tsk->signal->group_exit_code & 0x7f;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2160   		break;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2161   	case CLD_TRAPPED:
^1da177e4c3f41 Linus Torvalds      2005-04-16  2162   		info.si_status = tsk->exit_code & 0x7f;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2163   		break;
^1da177e4c3f41 Linus Torvalds      2005-04-16  2164   	default:
^1da177e4c3f41 Linus Torvalds      2005-04-16  2165   		BUG();
^1da177e4c3f41 Linus Torvalds      2005-04-16  2166   	}
^1da177e4c3f41 Linus Torvalds      2005-04-16  2167  
^1da177e4c3f41 Linus Torvalds      2005-04-16  2168  	sighand = parent->sighand;
1d8ae697c0ac6b Eric W. Biederman   2022-04-26 @2169  	lock = tsk->sighand != sighand;
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2170  	if (lock)
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2171  		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
^1da177e4c3f41 Linus Torvalds      2005-04-16  2172  	if (sighand->action[SIGCHLD-1].sa.sa_handler != SIG_IGN &&
^1da177e4c3f41 Linus Torvalds      2005-04-16  2173  	    !(sighand->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
13adee57c025b3 Eric W. Biederman   2022-04-26  2174  		send_signal_locked(SIGCHLD, &info, parent, PIDTYPE_TGID);
^1da177e4c3f41 Linus Torvalds      2005-04-16  2175  	/*
^1da177e4c3f41 Linus Torvalds      2005-04-16  2176  	 * Even if SIGCHLD is not generated, we must wake up wait4 calls.
^1da177e4c3f41 Linus Torvalds      2005-04-16  2177  	 */
^1da177e4c3f41 Linus Torvalds      2005-04-16  2178  	__wake_up_parent(tsk, parent);
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2179  	if (lock)
1d8ae697c0ac6b Eric W. Biederman   2022-04-26  2180  		spin_unlock(&sighand->siglock);
^1da177e4c3f41 Linus Torvalds      2005-04-16  2181  }
^1da177e4c3f41 Linus Torvalds      2005-04-16  2182  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-28 20:49               ` Eric W. Biederman
@ 2022-04-28 22:19                 ` Peter Zijlstra
  -1 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 22:19 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Thu, Apr 28, 2022 at 03:49:11PM -0500, Eric W. Biederman wrote:

> static void lock_parents_siglocks(bool lock_tracer)
> 	__releases(&current->sighand->siglock)
> 	__acquires(&current->sighand->siglock)
> 	__acquires(&current->real_parent->sighand->siglock)
> 	__acquires(&current->parent->sighand->siglock)
> {
> 	struct task_struct *me = current;
> 	struct sighand_struct *m_sighand = me->sighand;
> 
> 	lockdep_assert_held(&m_sighand->siglock);
> 
> 	rcu_read_lock();
> 	for (;;) {
> 		struct task_struct *parent, *tracer;
> 		struct sighand_struct *p_sighand, *t_sighand, *s1, *s2, *s3;
> 
> 		parent = me->real_parent;
> 		tracer = lock_tracer? me->parent : parent;
> 
> 		p_sighand = rcu_dereference(parent->sighand);
> 		t_sighand = rcu_dereference(tracer->sighand);
> 
> 		/* Sort the sighands so that s1 >= s2 >= s3 */
> 		s1 = m_sighand;
> 		s2 = p_sighand;
> 		s3 = t_sighand;
> 		if (s1 > s2)
> 			swap(s1, s2);
> 		if (s1 > s3)
> 			swap(s1, s3);
> 		if (s2 > s3)
> 			swap(s2, s3);
> 
> 		if (s1 != m_sighand) {
> 			spin_unlock(&m_sighand->siglock);
> 			spin_lock(&s1->siglock);
> 		}
> 
> 		if (s1 != s2)
> 			spin_lock_nested(&s2->siglock, SIGLOCK_LOCK_SECOND);
> 		if (s2 != s3)
> 			spin_lock_nested(&s3->siglock, SIGLOCK_LOCK_THIRD);
> 

Might as well just use 1 and 2 for subclass at this point, or use
SIGLOCK_LOCK_FIRST below.

> 		if (likely((me->real_parent == parent) &&
> 			   (me->parent == tracer) &&
> 			   (parent->sighand == p_sighand) &&
> 			   (tracer->sighand == t_sighand))) {
> 			break;
> 		}
> 		spin_unlock(&p_sighand->siglock);
>                 if (t_sighand != p_sighand)
> 			spin_unlock(&t_sighand->siglock);

Indent fail above ^, also you likey need this:

		/*
		 * Since [pt]_sighand will likely change if we go
		 * around, and m_sighand is the only one held, make sure
		 * it is subclass-0, since the above 's1 != m_sighand'
		 * clause very much relies on that.
		 */
		lock_set_subclass(&m_sighand->siglock, 0, _RET_IP_);

> 		continue;
> 	}
> 	rcu_read_unlock();
> }
> 
> Eric

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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 22:19                 ` Peter Zijlstra
  0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 22:19 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Thu, Apr 28, 2022 at 03:49:11PM -0500, Eric W. Biederman wrote:

> static void lock_parents_siglocks(bool lock_tracer)
> 	__releases(&current->sighand->siglock)
> 	__acquires(&current->sighand->siglock)
> 	__acquires(&current->real_parent->sighand->siglock)
> 	__acquires(&current->parent->sighand->siglock)
> {
> 	struct task_struct *me = current;
> 	struct sighand_struct *m_sighand = me->sighand;
> 
> 	lockdep_assert_held(&m_sighand->siglock);
> 
> 	rcu_read_lock();
> 	for (;;) {
> 		struct task_struct *parent, *tracer;
> 		struct sighand_struct *p_sighand, *t_sighand, *s1, *s2, *s3;
> 
> 		parent = me->real_parent;
> 		tracer = lock_tracer? me->parent : parent;
> 
> 		p_sighand = rcu_dereference(parent->sighand);
> 		t_sighand = rcu_dereference(tracer->sighand);
> 
> 		/* Sort the sighands so that s1 >= s2 >= s3 */
> 		s1 = m_sighand;
> 		s2 = p_sighand;
> 		s3 = t_sighand;
> 		if (s1 > s2)
> 			swap(s1, s2);
> 		if (s1 > s3)
> 			swap(s1, s3);
> 		if (s2 > s3)
> 			swap(s2, s3);
> 
> 		if (s1 != m_sighand) {
> 			spin_unlock(&m_sighand->siglock);
> 			spin_lock(&s1->siglock);
> 		}
> 
> 		if (s1 != s2)
> 			spin_lock_nested(&s2->siglock, SIGLOCK_LOCK_SECOND);
> 		if (s2 != s3)
> 			spin_lock_nested(&s3->siglock, SIGLOCK_LOCK_THIRD);
> 

Might as well just use 1 and 2 for subclass at this point, or use
SIGLOCK_LOCK_FIRST below.

> 		if (likely((me->real_parent == parent) &&
> 			   (me->parent == tracer) &&
> 			   (parent->sighand == p_sighand) &&
> 			   (tracer->sighand == t_sighand))) {
> 			break;
> 		}
> 		spin_unlock(&p_sighand->siglock);
>                 if (t_sighand != p_sighand)
> 			spin_unlock(&t_sighand->siglock);

Indent fail above ^, also you likey need this:

		/*
		 * Since [pt]_sighand will likely change if we go
		 * around, and m_sighand is the only one held, make sure
		 * it is subclass-0, since the above 's1 != m_sighand'
		 * clause very much relies on that.
		 */
		lock_set_subclass(&m_sighand->siglock, 0, _RET_IP_);

> 		continue;
> 	}
> 	rcu_read_unlock();
> }
> 
> Eric

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-28 18:37             ` Eric W. Biederman
@ 2022-04-28 20:49               ` Eric W. Biederman
  -1 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-28 20:49 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Peter Zijlstra <peterz@infradead.org> writes:
>
>> On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:
>>
>>> Hmm.  If we have the following process tree.
>>> 
>>>     A
>>>      \
>>>       B
>>>        \
>>>         C
>>> 
>>> Process A, B, and C are all in the same process group.
>>> Process A and B are setup to receive SIGCHILD when
>>> their process stops.
>>> 
>>> Process C traces process A.
>>> 
>>> When a sigstop is delivered to the group we can have:
>>> 
>>> Process B takes siglock(B) siglock(A) to notify the real_parent
>>> Process C takes siglock(C) siglock(B) to notify the real_parent
>>> Process A takes siglock(A) siglock(C) to notify the tracer
>>> 
>>> If they all take their local lock at the same time there is
>>> a deadlock.
>>> 
>>> I don't think the restriction that you can never ptrace anyone
>>> up the process tree is going to fly.  So it looks like I am back to the
>>> drawing board for this one.
>>
>> I've not had time to fully appreciate the nested locking here, but if it
>> is possible to rework things to always take both locks at the same time,
>> then it would be possible to impose an arbitrary lock order on things
>> and break the cycle that way.
>>
>> That is, simply order the locks by their heap address or something:
>>
>> static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
>> {
>> 	if (sh1 > sh2)
>> 		swap(sh1, sh2)
>>
>> 	spin_lock_irq(&sh1->siglock);
>> 	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
>> }
>
> You know it might be.  Especially given that the existing code is
> already dropping siglock and grabbing tasklist_lock.
>
> It would take a potentially triple lock function to lock
> the task it's real_parent and it's tracer (aka parent).
>
> That makes this possible to consider is that notifying the ``parents''
> is a fundamental part of the operation so we know we are going to
> need the lock so we can move it up.
>
> Throw in a pinch of lock_task_sighand and the triple lock function
> gets quite interesting.
>
> It is certainly worth trying, and I will.

To my surprise it doesn't look too bad.  The locking simplifications and
not using a lock as big as tasklist_lock probably make it even worth
doing.

I need to sleep on it and look at everything again.  In the
meantime here is my function that comes in with siglock held,
possibly drops it, and grabs the other two locks all in
order.

static void lock_parents_siglocks(bool lock_tracer)
	__releases(&current->sighand->siglock)
	__acquires(&current->sighand->siglock)
	__acquires(&current->real_parent->sighand->siglock)
	__acquires(&current->parent->sighand->siglock)
{
	struct task_struct *me = current;
	struct sighand_struct *m_sighand = me->sighand;

	lockdep_assert_held(&m_sighand->siglock);

	rcu_read_lock();
	for (;;) {
		struct task_struct *parent, *tracer;
		struct sighand_struct *p_sighand, *t_sighand, *s1, *s2, *s3;

		parent = me->real_parent;
		tracer = lock_tracer? me->parent : parent;

		p_sighand = rcu_dereference(parent->sighand);
		t_sighand = rcu_dereference(tracer->sighand);

		/* Sort the sighands so that s1 >= s2 >= s3 */
		s1 = m_sighand;
		s2 = p_sighand;
		s3 = t_sighand;
		if (s1 > s2)
			swap(s1, s2);
		if (s1 > s3)
			swap(s1, s3);
		if (s2 > s3)
			swap(s2, s3);

		if (s1 != m_sighand) {
			spin_unlock(&m_sighand->siglock);
			spin_lock(&s1->siglock);
		}

		if (s1 != s2)
			spin_lock_nested(&s2->siglock, SIGLOCK_LOCK_SECOND);
		if (s2 != s3)
			spin_lock_nested(&s3->siglock, SIGLOCK_LOCK_THIRD);

		if (likely((me->real_parent == parent) &&
			   (me->parent == tracer) &&
			   (parent->sighand == p_sighand) &&
			   (tracer->sighand == t_sighand))) {
			break;
		}
		spin_unlock(&p_sighand->siglock);
                if (t_sighand != p_sighand)
			spin_unlock(&t_sighand->siglock);
		continue;
	}
	rcu_read_unlock();
}

Eric

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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 20:49               ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-28 20:49 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Peter Zijlstra <peterz@infradead.org> writes:
>
>> On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:
>>
>>> Hmm.  If we have the following process tree.
>>> 
>>>     A
>>>      \
>>>       B
>>>        \
>>>         C
>>> 
>>> Process A, B, and C are all in the same process group.
>>> Process A and B are setup to receive SIGCHILD when
>>> their process stops.
>>> 
>>> Process C traces process A.
>>> 
>>> When a sigstop is delivered to the group we can have:
>>> 
>>> Process B takes siglock(B) siglock(A) to notify the real_parent
>>> Process C takes siglock(C) siglock(B) to notify the real_parent
>>> Process A takes siglock(A) siglock(C) to notify the tracer
>>> 
>>> If they all take their local lock at the same time there is
>>> a deadlock.
>>> 
>>> I don't think the restriction that you can never ptrace anyone
>>> up the process tree is going to fly.  So it looks like I am back to the
>>> drawing board for this one.
>>
>> I've not had time to fully appreciate the nested locking here, but if it
>> is possible to rework things to always take both locks at the same time,
>> then it would be possible to impose an arbitrary lock order on things
>> and break the cycle that way.
>>
>> That is, simply order the locks by their heap address or something:
>>
>> static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
>> {
>> 	if (sh1 > sh2)
>> 		swap(sh1, sh2)
>>
>> 	spin_lock_irq(&sh1->siglock);
>> 	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
>> }
>
> You know it might be.  Especially given that the existing code is
> already dropping siglock and grabbing tasklist_lock.
>
> It would take a potentially triple lock function to lock
> the task it's real_parent and it's tracer (aka parent).
>
> That makes this possible to consider is that notifying the ``parents''
> is a fundamental part of the operation so we know we are going to
> need the lock so we can move it up.
>
> Throw in a pinch of lock_task_sighand and the triple lock function
> gets quite interesting.
>
> It is certainly worth trying, and I will.

To my surprise it doesn't look too bad.  The locking simplifications and
not using a lock as big as tasklist_lock probably make it even worth
doing.

I need to sleep on it and look at everything again.  In the
meantime here is my function that comes in with siglock held,
possibly drops it, and grabs the other two locks all in
order.

static void lock_parents_siglocks(bool lock_tracer)
	__releases(&current->sighand->siglock)
	__acquires(&current->sighand->siglock)
	__acquires(&current->real_parent->sighand->siglock)
	__acquires(&current->parent->sighand->siglock)
{
	struct task_struct *me = current;
	struct sighand_struct *m_sighand = me->sighand;

	lockdep_assert_held(&m_sighand->siglock);

	rcu_read_lock();
	for (;;) {
		struct task_struct *parent, *tracer;
		struct sighand_struct *p_sighand, *t_sighand, *s1, *s2, *s3;

		parent = me->real_parent;
		tracer = lock_tracer? me->parent : parent;

		p_sighand = rcu_dereference(parent->sighand);
		t_sighand = rcu_dereference(tracer->sighand);

		/* Sort the sighands so that s1 >= s2 >= s3 */
		s1 = m_sighand;
		s2 = p_sighand;
		s3 = t_sighand;
		if (s1 > s2)
			swap(s1, s2);
		if (s1 > s3)
			swap(s1, s3);
		if (s2 > s3)
			swap(s2, s3);

		if (s1 != m_sighand) {
			spin_unlock(&m_sighand->siglock);
			spin_lock(&s1->siglock);
		}

		if (s1 != s2)
			spin_lock_nested(&s2->siglock, SIGLOCK_LOCK_SECOND);
		if (s2 != s3)
			spin_lock_nested(&s3->siglock, SIGLOCK_LOCK_THIRD);

		if (likely((me->real_parent == parent) &&
			   (me->parent == tracer) &&
			   (parent->sighand == p_sighand) &&
			   (tracer->sighand == t_sighand))) {
			break;
		}
		spin_unlock(&p_sighand->siglock);
                if (t_sighand != p_sighand)
			spin_unlock(&t_sighand->siglock);
		continue;
	}
	rcu_read_unlock();
}

Eric

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-28 17:44           ` Peter Zijlstra
@ 2022-04-28 18:37             ` Eric W. Biederman
  -1 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-28 18:37 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Peter Zijlstra <peterz@infradead.org> writes:

> On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:
>
>> Hmm.  If we have the following process tree.
>> 
>>     A
>>      \
>>       B
>>        \
>>         C
>> 
>> Process A, B, and C are all in the same process group.
>> Process A and B are setup to receive SIGCHILD when
>> their process stops.
>> 
>> Process C traces process A.
>> 
>> When a sigstop is delivered to the group we can have:
>> 
>> Process B takes siglock(B) siglock(A) to notify the real_parent
>> Process C takes siglock(C) siglock(B) to notify the real_parent
>> Process A takes siglock(A) siglock(C) to notify the tracer
>> 
>> If they all take their local lock at the same time there is
>> a deadlock.
>> 
>> I don't think the restriction that you can never ptrace anyone
>> up the process tree is going to fly.  So it looks like I am back to the
>> drawing board for this one.
>
> I've not had time to fully appreciate the nested locking here, but if it
> is possible to rework things to always take both locks at the same time,
> then it would be possible to impose an arbitrary lock order on things
> and break the cycle that way.
>
> That is, simply order the locks by their heap address or something:
>
> static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
> {
> 	if (sh1 > sh2)
> 		swap(sh1, sh2)
>
> 	spin_lock_irq(&sh1->siglock);
> 	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
> }

You know it might be.  Especially given that the existing code is
already dropping siglock and grabbing tasklist_lock.

It would take a potentially triple lock function to lock
the task it's real_parent and it's tracer (aka parent).

That makes this possible to consider is that notifying the ``parents''
is a fundamental part of the operation so we know we are going to
need the lock so we can move it up.

Throw in a pinch of lock_task_sighand and the triple lock function
gets quite interesting.

It is certainly worth trying, and I will.

Eric


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 18:37             ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-28 18:37 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Peter Zijlstra <peterz@infradead.org> writes:

> On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:
>
>> Hmm.  If we have the following process tree.
>> 
>>     A
>>      \
>>       B
>>        \
>>         C
>> 
>> Process A, B, and C are all in the same process group.
>> Process A and B are setup to receive SIGCHILD when
>> their process stops.
>> 
>> Process C traces process A.
>> 
>> When a sigstop is delivered to the group we can have:
>> 
>> Process B takes siglock(B) siglock(A) to notify the real_parent
>> Process C takes siglock(C) siglock(B) to notify the real_parent
>> Process A takes siglock(A) siglock(C) to notify the tracer
>> 
>> If they all take their local lock at the same time there is
>> a deadlock.
>> 
>> I don't think the restriction that you can never ptrace anyone
>> up the process tree is going to fly.  So it looks like I am back to the
>> drawing board for this one.
>
> I've not had time to fully appreciate the nested locking here, but if it
> is possible to rework things to always take both locks at the same time,
> then it would be possible to impose an arbitrary lock order on things
> and break the cycle that way.
>
> That is, simply order the locks by their heap address or something:
>
> static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
> {
> 	if (sh1 > sh2)
> 		swap(sh1, sh2)
>
> 	spin_lock_irq(&sh1->siglock);
> 	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
> }

You know it might be.  Especially given that the existing code is
already dropping siglock and grabbing tasklist_lock.

It would take a potentially triple lock function to lock
the task it's real_parent and it's tracer (aka parent).

That makes this possible to consider is that notifying the ``parents''
is a fundamental part of the operation so we know we are going to
need the lock so we can move it up.

Throw in a pinch of lock_task_sighand and the triple lock function
gets quite interesting.

It is certainly worth trying, and I will.

Eric


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-28 17:44           ` Peter Zijlstra
@ 2022-04-28 18:22             ` Oleg Nesterov
  -1 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-28 18:22 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Eric W. Biederman, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/28, Peter Zijlstra wrote:
>
> I've not had time to fully appreciate the nested locking here, but if it
> is possible to rework things to always take both locks at the same time,
> then it would be possible to impose an arbitrary lock order on things
> and break the cycle that way.

This is clear, but this is not that simple.

For example (with this series at least), ptrace_stop() already holds
current->sighand->siglock which (in particular) we need to protect
current->parent, but then we need current->parent->sighand->siglock
in do_notify_parent_cldstop().

Oleg.


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 18:22             ` Oleg Nesterov
  0 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-28 18:22 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Eric W. Biederman, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/28, Peter Zijlstra wrote:
>
> I've not had time to fully appreciate the nested locking here, but if it
> is possible to rework things to always take both locks at the same time,
> then it would be possible to impose an arbitrary lock order on things
> and break the cycle that way.

This is clear, but this is not that simple.

For example (with this series at least), ptrace_stop() already holds
current->sighand->siglock which (in particular) we need to protect
current->parent, but then we need current->parent->sighand->siglock
in do_notify_parent_cldstop().

Oleg.


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 14:47         ` Eric W. Biederman
@ 2022-04-28 17:44           ` Peter Zijlstra
  -1 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 17:44 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:

> Hmm.  If we have the following process tree.
> 
>     A
>      \
>       B
>        \
>         C
> 
> Process A, B, and C are all in the same process group.
> Process A and B are setup to receive SIGCHILD when
> their process stops.
> 
> Process C traces process A.
> 
> When a sigstop is delivered to the group we can have:
> 
> Process B takes siglock(B) siglock(A) to notify the real_parent
> Process C takes siglock(C) siglock(B) to notify the real_parent
> Process A takes siglock(A) siglock(C) to notify the tracer
> 
> If they all take their local lock at the same time there is
> a deadlock.
> 
> I don't think the restriction that you can never ptrace anyone
> up the process tree is going to fly.  So it looks like I am back to the
> drawing board for this one.

I've not had time to fully appreciate the nested locking here, but if it
is possible to rework things to always take both locks at the same time,
then it would be possible to impose an arbitrary lock order on things
and break the cycle that way.

That is, simply order the locks by their heap address or something:

static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
{
	if (sh1 > sh2)
		swap(sh1, sh2)

	spin_lock_irq(&sh1->siglock);
	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
}


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 17:44           ` Peter Zijlstra
  0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 17:44 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Oleg Nesterov, linux-kernel, rjw, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Wed, Apr 27, 2022 at 09:47:10AM -0500, Eric W. Biederman wrote:

> Hmm.  If we have the following process tree.
> 
>     A
>      \
>       B
>        \
>         C
> 
> Process A, B, and C are all in the same process group.
> Process A and B are setup to receive SIGCHILD when
> their process stops.
> 
> Process C traces process A.
> 
> When a sigstop is delivered to the group we can have:
> 
> Process B takes siglock(B) siglock(A) to notify the real_parent
> Process C takes siglock(C) siglock(B) to notify the real_parent
> Process A takes siglock(A) siglock(C) to notify the tracer
> 
> If they all take their local lock at the same time there is
> a deadlock.
> 
> I don't think the restriction that you can never ptrace anyone
> up the process tree is going to fly.  So it looks like I am back to the
> drawing board for this one.

I've not had time to fully appreciate the nested locking here, but if it
is possible to rework things to always take both locks at the same time,
then it would be possible to impose an arbitrary lock order on things
and break the cycle that way.

That is, simply order the locks by their heap address or something:

static void double_siglock_irq(struct sighand *sh1, struct sighand2 *sh2)
{
	if (sh1 > sh2)
		swap(sh1, sh2)

	spin_lock_irq(&sh1->siglock);
	spin_lock_nested(&sh2->siglock, SINGLE_DEPTH_NESTING);
}


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-26 22:52   ` Eric W. Biederman
@ 2022-04-28 10:38     ` Peter Zijlstra
  -1 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 10:38 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, Oleg Nesterov, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Tue, Apr 26, 2022 at 05:52:08PM -0500, Eric W. Biederman wrote:
> Now that siglock keeps tsk->parent and tsk->real_parent constant
> require that do_notify_parent_cldstop is called with tsk->siglock held
> instead of the tasklist_lock.
> 
> As all of the callers of do_notify_parent_cldstop had to drop the
> siglock and take tasklist_lock this simplifies all of it's callers.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  kernel/signal.c | 156 +++++++++++++++++-------------------------------
>  1 file changed, 55 insertions(+), 101 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 72d96614effc..584d67deb3cb 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2121,11 +2121,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>  				     bool for_ptracer, int why)
>  {
>  	struct kernel_siginfo info;
> -	unsigned long flags;
>  	struct task_struct *parent;
>  	struct sighand_struct *sighand;
> +	bool lock;
>  	u64 utime, stime;
>  
> +	assert_spin_locked(&tsk->sighand->siglock);

lockdep_assert_held() please...

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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-28 10:38     ` Peter Zijlstra
  0 siblings, 0 replies; 29+ messages in thread
From: Peter Zijlstra @ 2022-04-28 10:38 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, Oleg Nesterov, mingo, vincent.guittot,
	dietmar.eggemann, rostedt, mgorman, bigeasy, Will Deacon, tj,
	linux-pm, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On Tue, Apr 26, 2022 at 05:52:08PM -0500, Eric W. Biederman wrote:
> Now that siglock keeps tsk->parent and tsk->real_parent constant
> require that do_notify_parent_cldstop is called with tsk->siglock held
> instead of the tasklist_lock.
> 
> As all of the callers of do_notify_parent_cldstop had to drop the
> siglock and take tasklist_lock this simplifies all of it's callers.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  kernel/signal.c | 156 +++++++++++++++++-------------------------------
>  1 file changed, 55 insertions(+), 101 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 72d96614effc..584d67deb3cb 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2121,11 +2121,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>  				     bool for_ptracer, int why)
>  {
>  	struct kernel_siginfo info;
> -	unsigned long flags;
>  	struct task_struct *parent;
>  	struct sighand_struct *sighand;
> +	bool lock;
>  	u64 utime, stime;
>  
> +	assert_spin_locked(&tsk->sighand->siglock);

lockdep_assert_held() please...

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 15:00       ` Oleg Nesterov
@ 2022-04-27 21:52         ` Eric W. Biederman
  -1 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 21:52 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/27, Oleg Nesterov wrote:
>>
>> On 04/26, Eric W. Biederman wrote:
>> >
>> > @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
>> >  		spin_lock_irq(&current->sighand->siglock);
>> >  	}
>> >
>> > +	/* Don't stop if current is not ptraced */
>> > +	if (unlikely(!current->ptrace))
>> > +		return (clear_code) ? 0 : exit_code;
>> > +
>> > +	/*
>> > +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
>> > +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
>> > +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
>> > +	 * could be clear now.  We act as if SIGCONT is received after
>> > +	 * TASK_TRACED is entered - ignore it.
>> > +	 */
>> > +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
>> > +		gstop_done = task_participate_group_stop(current);
>> > +
>> > +	/*
>> > +	 * Notify parents of the stop.
>> > +	 *
>> > +	 * While ptraced, there are two parents - the ptracer and
>> > +	 * the real_parent of the group_leader.  The ptracer should
>> > +	 * know about every stop while the real parent is only
>> > +	 * interested in the completion of group stop.  The states
>> > +	 * for the two don't interact with each other.  Notify
>> > +	 * separately unless they're gonna be duplicates.
>> > +	 */
>> > +	do_notify_parent_cldstop(current, true, why);
>> > +	if (gstop_done && ptrace_reparented(current))
>> > +		do_notify_parent_cldstop(current, false, why);
>>
>> This doesn't look right too. The parent should be notified only after
>> we set __state = TASK_TRACED and ->exit code.
>>
>> Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
>> wakes it up, debugger calls wait_task_stopped() and then it will sleep
>> again, task_stopped_code() returns 0.
>>
>> This can be probably fixed if you remove the lockless (fast path)
>> task_stopped_code() check in wait_task_stopped(), but this is not
>> nice performance-wise...

Another detail I have overlooked.  Thank you.

Or we can change task_stopped_code look something like:

static int *task_stopped_code(struct task_struct *p, bool ptrace)
{
	if (ptrace) {
-		if (task_is_traced(p) && !(p->jobctl & JOBCTL_LISTENING))
+		if (p->ptrace && !(p->jobctl & JOBCTL_LISTENING))
			return &p->exit_code;
	} else {
		if (p->signal->flags & SIGNAL_STOP_STOPPED)
			return &p->signal->group_exit_code;
	}
	return NULL;
}

I probably need to do a little bit more to ensure that it isn't an
actual process exit_code in p->exit_code.  But the we don't have to
limit ourselves to being precisely in the task_is_traced stopped place
for the fast path.


> On the other hand, I don't understand why did you move the callsite
> of do_notify_parent_cldstop() up... just don't do this?

My goal and I still think it makes sense (if not my implementation)
is to move set_special_state as close as possible to schedule().

That way we can avoid sleeping spin_locks clobbering it and making
our life difficult.

My hope is we can just clean up ptrace_stop instead of making it more
complicated and harder to follow.  Not that I am fundamentally opposed
to the quiesce bit but the code is already very hard to follow because
of all it's nuance and complexity, and I would really like to reduce
that complexity if we can possibly figure out how.

Eric



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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 21:52         ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 21:52 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/27, Oleg Nesterov wrote:
>>
>> On 04/26, Eric W. Biederman wrote:
>> >
>> > @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
>> >  		spin_lock_irq(&current->sighand->siglock);
>> >  	}
>> >
>> > +	/* Don't stop if current is not ptraced */
>> > +	if (unlikely(!current->ptrace))
>> > +		return (clear_code) ? 0 : exit_code;
>> > +
>> > +	/*
>> > +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
>> > +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
>> > +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
>> > +	 * could be clear now.  We act as if SIGCONT is received after
>> > +	 * TASK_TRACED is entered - ignore it.
>> > +	 */
>> > +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
>> > +		gstop_done = task_participate_group_stop(current);
>> > +
>> > +	/*
>> > +	 * Notify parents of the stop.
>> > +	 *
>> > +	 * While ptraced, there are two parents - the ptracer and
>> > +	 * the real_parent of the group_leader.  The ptracer should
>> > +	 * know about every stop while the real parent is only
>> > +	 * interested in the completion of group stop.  The states
>> > +	 * for the two don't interact with each other.  Notify
>> > +	 * separately unless they're gonna be duplicates.
>> > +	 */
>> > +	do_notify_parent_cldstop(current, true, why);
>> > +	if (gstop_done && ptrace_reparented(current))
>> > +		do_notify_parent_cldstop(current, false, why);
>>
>> This doesn't look right too. The parent should be notified only after
>> we set __state = TASK_TRACED and ->exit code.
>>
>> Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
>> wakes it up, debugger calls wait_task_stopped() and then it will sleep
>> again, task_stopped_code() returns 0.
>>
>> This can be probably fixed if you remove the lockless (fast path)
>> task_stopped_code() check in wait_task_stopped(), but this is not
>> nice performance-wise...

Another detail I have overlooked.  Thank you.

Or we can change task_stopped_code look something like:

static int *task_stopped_code(struct task_struct *p, bool ptrace)
{
	if (ptrace) {
-		if (task_is_traced(p) && !(p->jobctl & JOBCTL_LISTENING))
+		if (p->ptrace && !(p->jobctl & JOBCTL_LISTENING))
			return &p->exit_code;
	} else {
		if (p->signal->flags & SIGNAL_STOP_STOPPED)
			return &p->signal->group_exit_code;
	}
	return NULL;
}

I probably need to do a little bit more to ensure that it isn't an
actual process exit_code in p->exit_code.  But the we don't have to
limit ourselves to being precisely in the task_is_traced stopped place
for the fast path.


> On the other hand, I don't understand why did you move the callsite
> of do_notify_parent_cldstop() up... just don't do this?

My goal and I still think it makes sense (if not my implementation)
is to move set_special_state as close as possible to schedule().

That way we can avoid sleeping spin_locks clobbering it and making
our life difficult.

My hope is we can just clean up ptrace_stop instead of making it more
complicated and harder to follow.  Not that I am fundamentally opposed
to the quiesce bit but the code is already very hard to follow because
of all it's nuance and complexity, and I would really like to reduce
that complexity if we can possibly figure out how.

Eric



_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 14:56     ` Oleg Nesterov
@ 2022-04-27 15:00       ` Oleg Nesterov
  -1 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 15:00 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/27, Oleg Nesterov wrote:
>
> On 04/26, Eric W. Biederman wrote:
> >
> > @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> >  		spin_lock_irq(&current->sighand->siglock);
> >  	}
> >
> > +	/* Don't stop if current is not ptraced */
> > +	if (unlikely(!current->ptrace))
> > +		return (clear_code) ? 0 : exit_code;
> > +
> > +	/*
> > +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
> > +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
> > +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
> > +	 * could be clear now.  We act as if SIGCONT is received after
> > +	 * TASK_TRACED is entered - ignore it.
> > +	 */
> > +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
> > +		gstop_done = task_participate_group_stop(current);
> > +
> > +	/*
> > +	 * Notify parents of the stop.
> > +	 *
> > +	 * While ptraced, there are two parents - the ptracer and
> > +	 * the real_parent of the group_leader.  The ptracer should
> > +	 * know about every stop while the real parent is only
> > +	 * interested in the completion of group stop.  The states
> > +	 * for the two don't interact with each other.  Notify
> > +	 * separately unless they're gonna be duplicates.
> > +	 */
> > +	do_notify_parent_cldstop(current, true, why);
> > +	if (gstop_done && ptrace_reparented(current))
> > +		do_notify_parent_cldstop(current, false, why);
>
> This doesn't look right too. The parent should be notified only after
> we set __state = TASK_TRACED and ->exit code.
>
> Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
> wakes it up, debugger calls wait_task_stopped() and then it will sleep
> again, task_stopped_code() returns 0.
>
> This can be probably fixed if you remove the lockless (fast path)
> task_stopped_code() check in wait_task_stopped(), but this is not
> nice performance-wise...

On the other hand, I don't understand why did you move the callsite
of do_notify_parent_cldstop() up... just don't do this?

Oleg.


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 15:00       ` Oleg Nesterov
  0 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 15:00 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/27, Oleg Nesterov wrote:
>
> On 04/26, Eric W. Biederman wrote:
> >
> > @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
> >  		spin_lock_irq(&current->sighand->siglock);
> >  	}
> >
> > +	/* Don't stop if current is not ptraced */
> > +	if (unlikely(!current->ptrace))
> > +		return (clear_code) ? 0 : exit_code;
> > +
> > +	/*
> > +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
> > +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
> > +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
> > +	 * could be clear now.  We act as if SIGCONT is received after
> > +	 * TASK_TRACED is entered - ignore it.
> > +	 */
> > +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
> > +		gstop_done = task_participate_group_stop(current);
> > +
> > +	/*
> > +	 * Notify parents of the stop.
> > +	 *
> > +	 * While ptraced, there are two parents - the ptracer and
> > +	 * the real_parent of the group_leader.  The ptracer should
> > +	 * know about every stop while the real parent is only
> > +	 * interested in the completion of group stop.  The states
> > +	 * for the two don't interact with each other.  Notify
> > +	 * separately unless they're gonna be duplicates.
> > +	 */
> > +	do_notify_parent_cldstop(current, true, why);
> > +	if (gstop_done && ptrace_reparented(current))
> > +		do_notify_parent_cldstop(current, false, why);
>
> This doesn't look right too. The parent should be notified only after
> we set __state = TASK_TRACED and ->exit code.
>
> Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
> wakes it up, debugger calls wait_task_stopped() and then it will sleep
> again, task_stopped_code() returns 0.
>
> This can be probably fixed if you remove the lockless (fast path)
> task_stopped_code() check in wait_task_stopped(), but this is not
> nice performance-wise...

On the other hand, I don't understand why did you move the callsite
of do_notify_parent_cldstop() up... just don't do this?

Oleg.


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-26 22:52   ` Eric W. Biederman
@ 2022-04-27 14:56     ` Oleg Nesterov
  -1 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:56 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/26, Eric W. Biederman wrote:
>
> @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
>  		spin_lock_irq(&current->sighand->siglock);
>  	}
>  
> +	/* Don't stop if current is not ptraced */
> +	if (unlikely(!current->ptrace))
> +		return (clear_code) ? 0 : exit_code;
> +
> +	/*
> +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
> +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
> +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
> +	 * could be clear now.  We act as if SIGCONT is received after
> +	 * TASK_TRACED is entered - ignore it.
> +	 */
> +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
> +		gstop_done = task_participate_group_stop(current);
> +
> +	/*
> +	 * Notify parents of the stop.
> +	 *
> +	 * While ptraced, there are two parents - the ptracer and
> +	 * the real_parent of the group_leader.  The ptracer should
> +	 * know about every stop while the real parent is only
> +	 * interested in the completion of group stop.  The states
> +	 * for the two don't interact with each other.  Notify
> +	 * separately unless they're gonna be duplicates.
> +	 */
> +	do_notify_parent_cldstop(current, true, why);
> +	if (gstop_done && ptrace_reparented(current))
> +		do_notify_parent_cldstop(current, false, why);

This doesn't look right too. The parent should be notified only after
we set __state = TASK_TRACED and ->exit code.

Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
wakes it up, debugger calls wait_task_stopped() and then it will sleep
again, task_stopped_code() returns 0.

This can be probably fixed if you remove the lockless (fast path)
task_stopped_code() check in wait_task_stopped(), but this is not
nice performance-wise...

Oleg.


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 14:56     ` Oleg Nesterov
  0 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:56 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/26, Eric W. Biederman wrote:
>
> @@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
>  		spin_lock_irq(&current->sighand->siglock);
>  	}
>  
> +	/* Don't stop if current is not ptraced */
> +	if (unlikely(!current->ptrace))
> +		return (clear_code) ? 0 : exit_code;
> +
> +	/*
> +	 * If @why is CLD_STOPPED, we're trapping to participate in a group
> +	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
> +	 * across siglock relocks since INTERRUPT was scheduled, PENDING
> +	 * could be clear now.  We act as if SIGCONT is received after
> +	 * TASK_TRACED is entered - ignore it.
> +	 */
> +	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
> +		gstop_done = task_participate_group_stop(current);
> +
> +	/*
> +	 * Notify parents of the stop.
> +	 *
> +	 * While ptraced, there are two parents - the ptracer and
> +	 * the real_parent of the group_leader.  The ptracer should
> +	 * know about every stop while the real parent is only
> +	 * interested in the completion of group stop.  The states
> +	 * for the two don't interact with each other.  Notify
> +	 * separately unless they're gonna be duplicates.
> +	 */
> +	do_notify_parent_cldstop(current, true, why);
> +	if (gstop_done && ptrace_reparented(current))
> +		do_notify_parent_cldstop(current, false, why);

This doesn't look right too. The parent should be notified only after
we set __state = TASK_TRACED and ->exit code.

Suppose that debugger sleeps in do_wait(). do_notify_parent_cldstop()
wakes it up, debugger calls wait_task_stopped() and then it will sleep
again, task_stopped_code() returns 0.

This can be probably fixed if you remove the lockless (fast path)
task_stopped_code() check in wait_task_stopped(), but this is not
nice performance-wise...

Oleg.


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 14:20       ` Eric W. Biederman
@ 2022-04-27 14:47         ` Eric W. Biederman
  -1 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 14:47 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Oleg Nesterov <oleg@redhat.com> writes:
>
>> On 04/26, Eric W. Biederman wrote:
>>>
>>> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>>>   	}
>>>
>>>  	sighand = parent->sighand;
>>> -	spin_lock_irqsave(&sighand->siglock, flags);
>>> +	lock = tsk->sighand != sighand;
>>> +	if (lock)
>>> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
>>
>> But why is it safe?
>>
>> Suppose we have two tasks, they both trace each other, both call
>> ptrace_stop() at the same time. Of course this is ugly, they both
>> will block.
>>
>> But with this patch in this case we have the trivial ABBA deadlock,
>> no?
>
> I was thinking in terms of the process tree (which is fine).
>
> The ptrace parental relationship definitely has the potential to be a
> graph with cycles.  Which as you point out is not fine.
>
>
> The result is very nice and I don't want to give it up.  I suspect
> something ptrace cycles are always a problem and can simply be
> forbidden.  That is going to take some analsysis and some additional
> code in ptrace_attach.
>
> I will go look at that.


Hmm.  If we have the following process tree.

    A
     \
      B
       \
        C

Process A, B, and C are all in the same process group.
Process A and B are setup to receive SIGCHILD when
their process stops.

Process C traces process A.

When a sigstop is delivered to the group we can have:

Process B takes siglock(B) siglock(A) to notify the real_parent
Process C takes siglock(C) siglock(B) to notify the real_parent
Process A takes siglock(A) siglock(C) to notify the tracer

If they all take their local lock at the same time there is
a deadlock.

I don't think the restriction that you can never ptrace anyone
up the process tree is going to fly.  So it looks like I am back to the
drawing board for this one.

Eric




    



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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 14:47         ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 14:47 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Oleg Nesterov <oleg@redhat.com> writes:
>
>> On 04/26, Eric W. Biederman wrote:
>>>
>>> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>>>   	}
>>>
>>>  	sighand = parent->sighand;
>>> -	spin_lock_irqsave(&sighand->siglock, flags);
>>> +	lock = tsk->sighand != sighand;
>>> +	if (lock)
>>> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
>>
>> But why is it safe?
>>
>> Suppose we have two tasks, they both trace each other, both call
>> ptrace_stop() at the same time. Of course this is ugly, they both
>> will block.
>>
>> But with this patch in this case we have the trivial ABBA deadlock,
>> no?
>
> I was thinking in terms of the process tree (which is fine).
>
> The ptrace parental relationship definitely has the potential to be a
> graph with cycles.  Which as you point out is not fine.
>
>
> The result is very nice and I don't want to give it up.  I suspect
> something ptrace cycles are always a problem and can simply be
> forbidden.  That is going to take some analsysis and some additional
> code in ptrace_attach.
>
> I will go look at that.


Hmm.  If we have the following process tree.

    A
     \
      B
       \
        C

Process A, B, and C are all in the same process group.
Process A and B are setup to receive SIGCHILD when
their process stops.

Process C traces process A.

When a sigstop is delivered to the group we can have:

Process B takes siglock(B) siglock(A) to notify the real_parent
Process C takes siglock(C) siglock(B) to notify the real_parent
Process A takes siglock(A) siglock(C) to notify the tracer

If they all take their local lock at the same time there is
a deadlock.

I don't think the restriction that you can never ptrace anyone
up the process tree is going to fly.  So it looks like I am back to the
drawing board for this one.

Eric




    



_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 14:20       ` Eric W. Biederman
@ 2022-04-27 14:43         ` Oleg Nesterov
  -1 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:43 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/27, Eric W. Biederman wrote:
>
> The ptrace parental relationship definitely has the potential to be a
> graph with cycles.  Which as you point out is not fine.
>
> The result is very nice and I don't want to give it up.  I suspect
> something ptrace cycles are always a problem and can simply be
> forbidden.

OK, please consider another case.

We have a parent P and its child C. C traces P.

This is not that unusual, I don't think we can forbid this case.

P reports an event and calls do_notify_parent_cldstop().

C receives SIGSTOP and calls do_notify_parent_cldstop() too.

Oleg.


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 14:43         ` Oleg Nesterov
  0 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:43 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/27, Eric W. Biederman wrote:
>
> The ptrace parental relationship definitely has the potential to be a
> graph with cycles.  Which as you point out is not fine.
>
> The result is very nice and I don't want to give it up.  I suspect
> something ptrace cycles are always a problem and can simply be
> forbidden.

OK, please consider another case.

We have a parent P and its child C. C traces P.

This is not that unusual, I don't think we can forbid this case.

P reports an event and calls do_notify_parent_cldstop().

C receives SIGSTOP and calls do_notify_parent_cldstop() too.

Oleg.


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-27 14:10     ` Oleg Nesterov
@ 2022-04-27 14:20       ` Eric W. Biederman
  -1 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 14:20 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/26, Eric W. Biederman wrote:
>>
>> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>>   	}
>>
>>  	sighand = parent->sighand;
>> -	spin_lock_irqsave(&sighand->siglock, flags);
>> +	lock = tsk->sighand != sighand;
>> +	if (lock)
>> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
>
> But why is it safe?
>
> Suppose we have two tasks, they both trace each other, both call
> ptrace_stop() at the same time. Of course this is ugly, they both
> will block.
>
> But with this patch in this case we have the trivial ABBA deadlock,
> no?

I was thinking in terms of the process tree (which is fine).

The ptrace parental relationship definitely has the potential to be a
graph with cycles.  Which as you point out is not fine.


The result is very nice and I don't want to give it up.  I suspect
something ptrace cycles are always a problem and can simply be
forbidden.  That is going to take some analsysis and some additional
code in ptrace_attach.

I will go look at that.

Eric


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 14:20       ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-27 14:20 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/26, Eric W. Biederman wrote:
>>
>> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>>   	}
>>
>>  	sighand = parent->sighand;
>> -	spin_lock_irqsave(&sighand->siglock, flags);
>> +	lock = tsk->sighand != sighand;
>> +	if (lock)
>> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
>
> But why is it safe?
>
> Suppose we have two tasks, they both trace each other, both call
> ptrace_stop() at the same time. Of course this is ugly, they both
> will block.
>
> But with this patch in this case we have the trivial ABBA deadlock,
> no?

I was thinking in terms of the process tree (which is fine).

The ptrace parental relationship definitely has the potential to be a
graph with cycles.  Which as you point out is not fine.


The result is very nice and I don't want to give it up.  I suspect
something ptrace cycles are always a problem and can simply be
forbidden.  That is going to take some analsysis and some additional
code in ptrace_attach.

I will go look at that.

Eric


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-26 22:52   ` Eric W. Biederman
@ 2022-04-27 14:10     ` Oleg Nesterov
  -1 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:10 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/26, Eric W. Biederman wrote:
>
> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>   	}
>
>  	sighand = parent->sighand;
> -	spin_lock_irqsave(&sighand->siglock, flags);
> +	lock = tsk->sighand != sighand;
> +	if (lock)
> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);

But why is it safe?

Suppose we have two tasks, they both trace each other, both call
ptrace_stop() at the same time. Of course this is ugly, they both
will block.

But with this patch in this case we have the trivial ABBA deadlock,
no?

Oleg.


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

* Re: [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-27 14:10     ` Oleg Nesterov
  0 siblings, 0 replies; 29+ messages in thread
From: Oleg Nesterov @ 2022-04-27 14:10 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: linux-kernel, rjw, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn

On 04/26, Eric W. Biederman wrote:
>
> @@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
>   	}
>
>  	sighand = parent->sighand;
> -	spin_lock_irqsave(&sighand->siglock, flags);
> +	lock = tsk->sighand != sighand;
> +	if (lock)
> +		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);

But why is it safe?

Suppose we have two tasks, they both trace each other, both call
ptrace_stop() at the same time. Of course this is ugly, they both
will block.

But with this patch in this case we have the trivial ABBA deadlock,
no?

Oleg.


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

* [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
  2022-04-26 22:50 [PATCH 0/9] ptrace: cleaning up ptrace_stop Eric W. Biederman
@ 2022-04-26 22:52   ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-26 22:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: rjw, Oleg Nesterov, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn, Eric W. Biederman

Now that siglock keeps tsk->parent and tsk->real_parent constant
require that do_notify_parent_cldstop is called with tsk->siglock held
instead of the tasklist_lock.

As all of the callers of do_notify_parent_cldstop had to drop the
siglock and take tasklist_lock this simplifies all of it's callers.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 156 +++++++++++++++++-------------------------------
 1 file changed, 55 insertions(+), 101 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 72d96614effc..584d67deb3cb 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2121,11 +2121,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 				     bool for_ptracer, int why)
 {
 	struct kernel_siginfo info;
-	unsigned long flags;
 	struct task_struct *parent;
 	struct sighand_struct *sighand;
+	bool lock;
 	u64 utime, stime;
 
+	assert_spin_locked(&tsk->sighand->siglock);
+
 	if (for_ptracer) {
 		parent = tsk->parent;
 	} else {
@@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
  	}
 
 	sighand = parent->sighand;
-	spin_lock_irqsave(&sighand->siglock, flags);
+	lock = tsk->sighand != sighand;
+	if (lock)
+		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
 	if (sighand->action[SIGCHLD-1].sa.sa_handler != SIG_IGN &&
 	    !(sighand->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
 		send_signal_locked(SIGCHLD, &info, parent, PIDTYPE_TGID);
@@ -2172,7 +2176,8 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 	 * Even if SIGCHLD is not generated, we must wake up wait4 calls.
 	 */
 	__wake_up_parent(tsk, parent);
-	spin_unlock_irqrestore(&sighand->siglock, flags);
+	if (lock)
+		spin_unlock(&sighand->siglock);
 }
 
 /*
@@ -2193,7 +2198,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	__acquires(&current->sighand->siglock)
 {
 	bool gstop_done = false;
-	bool read_code = true;
 
 	if (arch_ptrace_stop_needed()) {
 		/*
@@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 		spin_lock_irq(&current->sighand->siglock);
 	}
 
+	/* Don't stop if current is not ptraced */
+	if (unlikely(!current->ptrace))
+		return (clear_code) ? 0 : exit_code;
+
+	/*
+	 * If @why is CLD_STOPPED, we're trapping to participate in a group
+	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
+	 * across siglock relocks since INTERRUPT was scheduled, PENDING
+	 * could be clear now.  We act as if SIGCONT is received after
+	 * TASK_TRACED is entered - ignore it.
+	 */
+	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
+		gstop_done = task_participate_group_stop(current);
+
+	/*
+	 * Notify parents of the stop.
+	 *
+	 * While ptraced, there are two parents - the ptracer and
+	 * the real_parent of the group_leader.  The ptracer should
+	 * know about every stop while the real parent is only
+	 * interested in the completion of group stop.  The states
+	 * for the two don't interact with each other.  Notify
+	 * separately unless they're gonna be duplicates.
+	 */
+	do_notify_parent_cldstop(current, true, why);
+	if (gstop_done && ptrace_reparented(current))
+		do_notify_parent_cldstop(current, false, why);
+
 	/*
 	 * schedule() will not sleep if there is a pending signal that
 	 * can awaken the task.
@@ -2239,15 +2271,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	current->last_siginfo = info;
 	current->exit_code = exit_code;
 
-	/*
-	 * If @why is CLD_STOPPED, we're trapping to participate in a group
-	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
-	 * across siglock relocks since INTERRUPT was scheduled, PENDING
-	 * could be clear now.  We act as if SIGCONT is received after
-	 * TASK_TRACED is entered - ignore it.
-	 */
-	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
-		gstop_done = task_participate_group_stop(current);
 
 	/* any trap clears pending STOP trap, STOP trap clears NOTIFY */
 	task_clear_jobctl_pending(current, JOBCTL_TRAP_STOP);
@@ -2257,56 +2280,19 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	/* entering a trap, clear TRAPPING */
 	task_clear_jobctl_trapping(current);
 
+	/*
+	 * Don't want to allow preemption here, because
+	 * sys_ptrace() needs this task to be inactive.
+	 *
+	 * XXX: implement spin_unlock_no_resched().
+	 */
+	preempt_disable();
 	spin_unlock_irq(&current->sighand->siglock);
-	read_lock(&tasklist_lock);
-	if (likely(current->ptrace)) {
-		/*
-		 * Notify parents of the stop.
-		 *
-		 * While ptraced, there are two parents - the ptracer and
-		 * the real_parent of the group_leader.  The ptracer should
-		 * know about every stop while the real parent is only
-		 * interested in the completion of group stop.  The states
-		 * for the two don't interact with each other.  Notify
-		 * separately unless they're gonna be duplicates.
-		 */
-		do_notify_parent_cldstop(current, true, why);
-		if (gstop_done && ptrace_reparented(current))
-			do_notify_parent_cldstop(current, false, why);
 
-		/*
-		 * Don't want to allow preemption here, because
-		 * sys_ptrace() needs this task to be inactive.
-		 *
-		 * XXX: implement read_unlock_no_resched().
-		 */
-		preempt_disable();
-		read_unlock(&tasklist_lock);
-		cgroup_enter_frozen();
-		preempt_enable_no_resched();
-		freezable_schedule();
-		cgroup_leave_frozen(true);
-	} else {
-		/*
-		 * By the time we got the lock, our tracer went away.
-		 * Don't drop the lock yet, another tracer may come.
-		 *
-		 * If @gstop_done, the ptracer went away between group stop
-		 * completion and here.  During detach, it would have set
-		 * JOBCTL_STOP_PENDING on us and we'll re-enter
-		 * TASK_STOPPED in do_signal_stop() on return, so notifying
-		 * the real parent of the group stop completion is enough.
-		 */
-		if (gstop_done)
-			do_notify_parent_cldstop(current, false, why);
-
-		/* tasklist protects us from ptrace_freeze_traced() */
-		__set_current_state(TASK_RUNNING);
-		read_code = false;
-		if (clear_code)
-			exit_code = 0;
-		read_unlock(&tasklist_lock);
-	}
+	cgroup_enter_frozen();
+	preempt_enable_no_resched();
+	freezable_schedule();
+	cgroup_leave_frozen(true);
 
 	/*
 	 * We are back.  Now reacquire the siglock before touching
@@ -2314,8 +2300,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	 * any signal-sending on another CPU that wants to examine it.
 	 */
 	spin_lock_irq(&current->sighand->siglock);
-	if (read_code)
-		exit_code = current->exit_code;
+	exit_code = current->exit_code;
 	current->last_siginfo = NULL;
 	current->ptrace_message = 0;
 	current->exit_code = 0;
@@ -2444,34 +2429,17 @@ static bool do_signal_stop(int signr)
 	}
 
 	if (likely(!current->ptrace)) {
-		int notify = 0;
-
 		/*
 		 * If there are no other threads in the group, or if there
 		 * is a group stop in progress and we are the last to stop,
-		 * report to the parent.
+		 * report to the real_parent.
 		 */
 		if (task_participate_group_stop(current))
-			notify = CLD_STOPPED;
+			do_notify_parent_cldstop(current, false, CLD_STOPPED);
 
 		set_special_state(TASK_STOPPED);
 		spin_unlock_irq(&current->sighand->siglock);
 
-		/*
-		 * Notify the parent of the group stop completion.  Because
-		 * we're not holding either the siglock or tasklist_lock
-		 * here, ptracer may attach inbetween; however, this is for
-		 * group stop and should always be delivered to the real
-		 * parent of the group leader.  The new ptracer will get
-		 * its notification when this task transitions into
-		 * TASK_TRACED.
-		 */
-		if (notify) {
-			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current, false, notify);
-			read_unlock(&tasklist_lock);
-		}
-
 		/* Now we don't run again until woken by SIGCONT or SIGKILL */
 		cgroup_enter_frozen();
 		freezable_schedule();
@@ -2665,8 +2633,6 @@ bool get_signal(struct ksignal *ksig)
 
 		signal->flags &= ~SIGNAL_CLD_MASK;
 
-		spin_unlock_irq(&sighand->siglock);
-
 		/*
 		 * Notify the parent that we're continuing.  This event is
 		 * always per-process and doesn't make whole lot of sense
@@ -2675,15 +2641,10 @@ bool get_signal(struct ksignal *ksig)
 		 * the ptracer of the group leader too unless it's gonna be
 		 * a duplicate.
 		 */
-		read_lock(&tasklist_lock);
 		do_notify_parent_cldstop(current, false, why);
-
 		if (ptrace_reparented(current->group_leader))
 			do_notify_parent_cldstop(current->group_leader,
 						true, why);
-		read_unlock(&tasklist_lock);
-
-		goto relock;
 	}
 
 	for (;;) {
@@ -2940,7 +2901,6 @@ static void retarget_shared_pending(struct task_struct *tsk, sigset_t *which)
 
 void exit_signals(struct task_struct *tsk)
 {
-	int group_stop = 0;
 	sigset_t unblocked;
 
 	/*
@@ -2971,21 +2931,15 @@ void exit_signals(struct task_struct *tsk)
 	signotset(&unblocked);
 	retarget_shared_pending(tsk, &unblocked);
 
-	if (unlikely(tsk->jobctl & JOBCTL_STOP_PENDING) &&
-	    task_participate_group_stop(tsk))
-		group_stop = CLD_STOPPED;
-out:
-	spin_unlock_irq(&tsk->sighand->siglock);
-
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.
 	 */
-	if (unlikely(group_stop)) {
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(tsk, false, group_stop);
-		read_unlock(&tasklist_lock);
-	}
+	if (unlikely(tsk->jobctl & JOBCTL_STOP_PENDING) &&
+	    task_participate_group_stop(tsk))
+		do_notify_parent_cldstop(tsk, false, CLD_STOPPED);
+out:
+	spin_unlock_irq(&tsk->sighand->siglock);
 }
 
 /*
-- 
2.35.3


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

* [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held
@ 2022-04-26 22:52   ` Eric W. Biederman
  0 siblings, 0 replies; 29+ messages in thread
From: Eric W. Biederman @ 2022-04-26 22:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: rjw, Oleg Nesterov, mingo, vincent.guittot, dietmar.eggemann,
	rostedt, mgorman, bigeasy, Will Deacon, tj, linux-pm,
	Peter Zijlstra, Richard Weinberger, Anton Ivanov, Johannes Berg,
	linux-um, Chris Zankel, Max Filippov, inux-xtensa, Kees Cook,
	Jann Horn, Eric W. Biederman

Now that siglock keeps tsk->parent and tsk->real_parent constant
require that do_notify_parent_cldstop is called with tsk->siglock held
instead of the tasklist_lock.

As all of the callers of do_notify_parent_cldstop had to drop the
siglock and take tasklist_lock this simplifies all of it's callers.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/signal.c | 156 +++++++++++++++++-------------------------------
 1 file changed, 55 insertions(+), 101 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 72d96614effc..584d67deb3cb 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2121,11 +2121,13 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 				     bool for_ptracer, int why)
 {
 	struct kernel_siginfo info;
-	unsigned long flags;
 	struct task_struct *parent;
 	struct sighand_struct *sighand;
+	bool lock;
 	u64 utime, stime;
 
+	assert_spin_locked(&tsk->sighand->siglock);
+
 	if (for_ptracer) {
 		parent = tsk->parent;
 	} else {
@@ -2164,7 +2166,9 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
  	}
 
 	sighand = parent->sighand;
-	spin_lock_irqsave(&sighand->siglock, flags);
+	lock = tsk->sighand != sighand;
+	if (lock)
+		spin_lock_nested(&sighand->siglock, SINGLE_DEPTH_NESTING);
 	if (sighand->action[SIGCHLD-1].sa.sa_handler != SIG_IGN &&
 	    !(sighand->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDSTOP))
 		send_signal_locked(SIGCHLD, &info, parent, PIDTYPE_TGID);
@@ -2172,7 +2176,8 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 	 * Even if SIGCHLD is not generated, we must wake up wait4 calls.
 	 */
 	__wake_up_parent(tsk, parent);
-	spin_unlock_irqrestore(&sighand->siglock, flags);
+	if (lock)
+		spin_unlock(&sighand->siglock);
 }
 
 /*
@@ -2193,7 +2198,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	__acquires(&current->sighand->siglock)
 {
 	bool gstop_done = false;
-	bool read_code = true;
 
 	if (arch_ptrace_stop_needed()) {
 		/*
@@ -2209,6 +2213,34 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 		spin_lock_irq(&current->sighand->siglock);
 	}
 
+	/* Don't stop if current is not ptraced */
+	if (unlikely(!current->ptrace))
+		return (clear_code) ? 0 : exit_code;
+
+	/*
+	 * If @why is CLD_STOPPED, we're trapping to participate in a group
+	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
+	 * across siglock relocks since INTERRUPT was scheduled, PENDING
+	 * could be clear now.  We act as if SIGCONT is received after
+	 * TASK_TRACED is entered - ignore it.
+	 */
+	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
+		gstop_done = task_participate_group_stop(current);
+
+	/*
+	 * Notify parents of the stop.
+	 *
+	 * While ptraced, there are two parents - the ptracer and
+	 * the real_parent of the group_leader.  The ptracer should
+	 * know about every stop while the real parent is only
+	 * interested in the completion of group stop.  The states
+	 * for the two don't interact with each other.  Notify
+	 * separately unless they're gonna be duplicates.
+	 */
+	do_notify_parent_cldstop(current, true, why);
+	if (gstop_done && ptrace_reparented(current))
+		do_notify_parent_cldstop(current, false, why);
+
 	/*
 	 * schedule() will not sleep if there is a pending signal that
 	 * can awaken the task.
@@ -2239,15 +2271,6 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	current->last_siginfo = info;
 	current->exit_code = exit_code;
 
-	/*
-	 * If @why is CLD_STOPPED, we're trapping to participate in a group
-	 * stop.  Do the bookkeeping.  Note that if SIGCONT was delievered
-	 * across siglock relocks since INTERRUPT was scheduled, PENDING
-	 * could be clear now.  We act as if SIGCONT is received after
-	 * TASK_TRACED is entered - ignore it.
-	 */
-	if (why == CLD_STOPPED && (current->jobctl & JOBCTL_STOP_PENDING))
-		gstop_done = task_participate_group_stop(current);
 
 	/* any trap clears pending STOP trap, STOP trap clears NOTIFY */
 	task_clear_jobctl_pending(current, JOBCTL_TRAP_STOP);
@@ -2257,56 +2280,19 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	/* entering a trap, clear TRAPPING */
 	task_clear_jobctl_trapping(current);
 
+	/*
+	 * Don't want to allow preemption here, because
+	 * sys_ptrace() needs this task to be inactive.
+	 *
+	 * XXX: implement spin_unlock_no_resched().
+	 */
+	preempt_disable();
 	spin_unlock_irq(&current->sighand->siglock);
-	read_lock(&tasklist_lock);
-	if (likely(current->ptrace)) {
-		/*
-		 * Notify parents of the stop.
-		 *
-		 * While ptraced, there are two parents - the ptracer and
-		 * the real_parent of the group_leader.  The ptracer should
-		 * know about every stop while the real parent is only
-		 * interested in the completion of group stop.  The states
-		 * for the two don't interact with each other.  Notify
-		 * separately unless they're gonna be duplicates.
-		 */
-		do_notify_parent_cldstop(current, true, why);
-		if (gstop_done && ptrace_reparented(current))
-			do_notify_parent_cldstop(current, false, why);
 
-		/*
-		 * Don't want to allow preemption here, because
-		 * sys_ptrace() needs this task to be inactive.
-		 *
-		 * XXX: implement read_unlock_no_resched().
-		 */
-		preempt_disable();
-		read_unlock(&tasklist_lock);
-		cgroup_enter_frozen();
-		preempt_enable_no_resched();
-		freezable_schedule();
-		cgroup_leave_frozen(true);
-	} else {
-		/*
-		 * By the time we got the lock, our tracer went away.
-		 * Don't drop the lock yet, another tracer may come.
-		 *
-		 * If @gstop_done, the ptracer went away between group stop
-		 * completion and here.  During detach, it would have set
-		 * JOBCTL_STOP_PENDING on us and we'll re-enter
-		 * TASK_STOPPED in do_signal_stop() on return, so notifying
-		 * the real parent of the group stop completion is enough.
-		 */
-		if (gstop_done)
-			do_notify_parent_cldstop(current, false, why);
-
-		/* tasklist protects us from ptrace_freeze_traced() */
-		__set_current_state(TASK_RUNNING);
-		read_code = false;
-		if (clear_code)
-			exit_code = 0;
-		read_unlock(&tasklist_lock);
-	}
+	cgroup_enter_frozen();
+	preempt_enable_no_resched();
+	freezable_schedule();
+	cgroup_leave_frozen(true);
 
 	/*
 	 * We are back.  Now reacquire the siglock before touching
@@ -2314,8 +2300,7 @@ static int ptrace_stop(int exit_code, int why, int clear_code,
 	 * any signal-sending on another CPU that wants to examine it.
 	 */
 	spin_lock_irq(&current->sighand->siglock);
-	if (read_code)
-		exit_code = current->exit_code;
+	exit_code = current->exit_code;
 	current->last_siginfo = NULL;
 	current->ptrace_message = 0;
 	current->exit_code = 0;
@@ -2444,34 +2429,17 @@ static bool do_signal_stop(int signr)
 	}
 
 	if (likely(!current->ptrace)) {
-		int notify = 0;
-
 		/*
 		 * If there are no other threads in the group, or if there
 		 * is a group stop in progress and we are the last to stop,
-		 * report to the parent.
+		 * report to the real_parent.
 		 */
 		if (task_participate_group_stop(current))
-			notify = CLD_STOPPED;
+			do_notify_parent_cldstop(current, false, CLD_STOPPED);
 
 		set_special_state(TASK_STOPPED);
 		spin_unlock_irq(&current->sighand->siglock);
 
-		/*
-		 * Notify the parent of the group stop completion.  Because
-		 * we're not holding either the siglock or tasklist_lock
-		 * here, ptracer may attach inbetween; however, this is for
-		 * group stop and should always be delivered to the real
-		 * parent of the group leader.  The new ptracer will get
-		 * its notification when this task transitions into
-		 * TASK_TRACED.
-		 */
-		if (notify) {
-			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current, false, notify);
-			read_unlock(&tasklist_lock);
-		}
-
 		/* Now we don't run again until woken by SIGCONT or SIGKILL */
 		cgroup_enter_frozen();
 		freezable_schedule();
@@ -2665,8 +2633,6 @@ bool get_signal(struct ksignal *ksig)
 
 		signal->flags &= ~SIGNAL_CLD_MASK;
 
-		spin_unlock_irq(&sighand->siglock);
-
 		/*
 		 * Notify the parent that we're continuing.  This event is
 		 * always per-process and doesn't make whole lot of sense
@@ -2675,15 +2641,10 @@ bool get_signal(struct ksignal *ksig)
 		 * the ptracer of the group leader too unless it's gonna be
 		 * a duplicate.
 		 */
-		read_lock(&tasklist_lock);
 		do_notify_parent_cldstop(current, false, why);
-
 		if (ptrace_reparented(current->group_leader))
 			do_notify_parent_cldstop(current->group_leader,
 						true, why);
-		read_unlock(&tasklist_lock);
-
-		goto relock;
 	}
 
 	for (;;) {
@@ -2940,7 +2901,6 @@ static void retarget_shared_pending(struct task_struct *tsk, sigset_t *which)
 
 void exit_signals(struct task_struct *tsk)
 {
-	int group_stop = 0;
 	sigset_t unblocked;
 
 	/*
@@ -2971,21 +2931,15 @@ void exit_signals(struct task_struct *tsk)
 	signotset(&unblocked);
 	retarget_shared_pending(tsk, &unblocked);
 
-	if (unlikely(tsk->jobctl & JOBCTL_STOP_PENDING) &&
-	    task_participate_group_stop(tsk))
-		group_stop = CLD_STOPPED;
-out:
-	spin_unlock_irq(&tsk->sighand->siglock);
-
 	/*
 	 * If group stop has completed, deliver the notification.  This
 	 * should always go to the real parent of the group leader.
 	 */
-	if (unlikely(group_stop)) {
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(tsk, false, group_stop);
-		read_unlock(&tasklist_lock);
-	}
+	if (unlikely(tsk->jobctl & JOBCTL_STOP_PENDING) &&
+	    task_participate_group_stop(tsk))
+		do_notify_parent_cldstop(tsk, false, CLD_STOPPED);
+out:
+	spin_unlock_irq(&tsk->sighand->siglock);
 }
 
 /*
-- 
2.35.3


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um


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

end of thread, other threads:[~2022-04-28 22:33 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-28 22:33 [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2022-04-26 22:50 [PATCH 0/9] ptrace: cleaning up ptrace_stop Eric W. Biederman
2022-04-26 22:52 ` [PATCH 6/9] signal: Always call do_notify_parent_cldstop with siglock held Eric W. Biederman
2022-04-26 22:52   ` Eric W. Biederman
2022-04-27 14:10   ` Oleg Nesterov
2022-04-27 14:10     ` Oleg Nesterov
2022-04-27 14:20     ` Eric W. Biederman
2022-04-27 14:20       ` Eric W. Biederman
2022-04-27 14:43       ` Oleg Nesterov
2022-04-27 14:43         ` Oleg Nesterov
2022-04-27 14:47       ` Eric W. Biederman
2022-04-27 14:47         ` Eric W. Biederman
2022-04-28 17:44         ` Peter Zijlstra
2022-04-28 17:44           ` Peter Zijlstra
2022-04-28 18:22           ` Oleg Nesterov
2022-04-28 18:22             ` Oleg Nesterov
2022-04-28 18:37           ` Eric W. Biederman
2022-04-28 18:37             ` Eric W. Biederman
2022-04-28 20:49             ` Eric W. Biederman
2022-04-28 20:49               ` Eric W. Biederman
2022-04-28 22:19               ` Peter Zijlstra
2022-04-28 22:19                 ` Peter Zijlstra
2022-04-27 14:56   ` Oleg Nesterov
2022-04-27 14:56     ` Oleg Nesterov
2022-04-27 15:00     ` Oleg Nesterov
2022-04-27 15:00       ` Oleg Nesterov
2022-04-27 21:52       ` Eric W. Biederman
2022-04-27 21:52         ` Eric W. Biederman
2022-04-28 10:38   ` Peter Zijlstra
2022-04-28 10:38     ` Peter Zijlstra

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.