[PATCH 0/1] Get tracer PID without reliance on the proc FS

[PATCH 0/1] Get tracer PID without reliance on the proc FS

[Roman Kisel]

For debugging, it might be useful to run the debug trap
instruction to break into the debugger. To detect the debugger
presence, the kernel provides the `/proc/self/status` pseudo-file
that needs to be searched for the "TracerPid:" string.

Provide a prctl command that returns the PID of the tracer if any.
That allows for much simpler logic in the user land, and makes it
possible to detect tracer presence even if PROC_FS is not enabled.

As an example where this might be useful, one might refer to
the standard C++ and Rust libraries. See these links for the details:

* https://en.cppreference.com/w/cpp/utility/breakpoint_if_debugging
* https://lists.llvm.org/pipermail/libcxx-commits/2024-May/083574.html
* https://patchwork-proxy.ozlabs.org/project/gcc/patch/20240601102446.878286-1-jwakely@redhat.com/#3321542
* https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2546r0.html
* https://github.com/rust-lang/rust/pull/129019

Roman Kisel (1):
  ptrace: Get tracer PID without reliance on the proc FS

 include/uapi/linux/ptrace.h |  1 +
 kernel/ptrace.c             | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)


base-commit: ad618736883b8970f66af799e34007475fe33a68
-- 
2.34.1

[PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

For debugging, it might be useful to run the debug trap
instruction to break into the debugger. To detect the debugger
presence, the kernel provides the `/proc/self/status` pseudo-file
that needs to be searched for the "TracerPid:" string.

Provide a prctl command that returns the PID of the tracer if any.
That allows for much simpler logic in the user land, and makes it
possible to detect tracer presence even if PROC_FS is not enabled.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 include/uapi/linux/ptrace.h |  1 +
 kernel/ptrace.c             | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index 72c038fc71d0..5056f5d1df6b 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -21,6 +21,7 @@
 
 #define PTRACE_ATTACH		  16
 #define PTRACE_DETACH		  17
+#define PTRACE_TRACER		  18
 
 #define PTRACE_SYSCALL		  24
 
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index d5f89f9ef29f..91275c5c4f57 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -1258,7 +1258,7 @@ int ptrace_request(struct task_struct *child, long request,
 SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 		unsigned long, data)
 {
-	struct task_struct *child;
+	struct task_struct *child, *tracer;
 	long ret;
 
 	if (request == PTRACE_TRACEME) {
@@ -1277,6 +1277,15 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 		goto out_put_task_struct;
 	}
 
+	if (request == PTRACE_TRACER) {
+		rcu_read_lock();
+		tracer = ptrace_parent(current);
+		ret = tracer ? task_pid_nr_ns(tracer,
+					task_active_pid_ns(current->parent)) : -ESRCH;
+		rcu_read_unlock();
+		goto out;
+	}
+
 	ret = ptrace_check_attach(child, request == PTRACE_KILL ||
 				  request == PTRACE_INTERRUPT);
 	if (ret < 0)
-- 
2.34.1

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

Add cc's. Perhaps someone else can ack/nack the intent...

This (trivial) patch is obviously buggy, but fixable. I won't argue
if it can help userspace.

On 09/05, Roman Kisel wrote:
>
> For debugging, it might be useful to run the debug trap
> instruction to break into the debugger. To detect the debugger
> presence, the kernel provides the `/proc/self/status` pseudo-file
> that needs to be searched for the "TracerPid:" string.
>
> Provide a prctl command that returns the PID of the tracer if any.

prctl?

> That allows for much simpler logic in the user land, and makes it
> possible to detect tracer presence even if PROC_FS is not enabled.

You should probably move the links from 0/1 to the changelog to make
it more convincing.

> +	if (request == PTRACE_TRACER) {
> +		rcu_read_lock();
> +		tracer = ptrace_parent(current);
> +		ret = tracer ? task_pid_nr_ns(tracer,
> +					task_active_pid_ns(current->parent)) : -ESRCH;

The namespace is wrong, we need task_active_pid_ns(current). So this
code should simply do task_tgid_vnr(tracer) like sys_getppid() does.
And to me it would be better to return 0 if !current->ptrace.

> +		rcu_read_unlock();
> +		goto out;

Wrong, this code runs after "child = find_get_task_by_vpid(pid);" above.

And why? perhaps the intent was to check if this child is traced, not
current?

Oleg.

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

Forgot to ask...

Do you really want the tracer's pid or can PTRACE_TRACER/whatever
simply return the !!current->ptrace boolean? The changelog should
probably explain this too.

On 09/06, Oleg Nesterov wrote:
>
> Add cc's. Perhaps someone else can ack/nack the intent...
>
> This (trivial) patch is obviously buggy, but fixable. I won't argue
> if it can help userspace.
>
> On 09/05, Roman Kisel wrote:
> >
> > For debugging, it might be useful to run the debug trap
> > instruction to break into the debugger. To detect the debugger
> > presence, the kernel provides the `/proc/self/status` pseudo-file
> > that needs to be searched for the "TracerPid:" string.
> >
> > Provide a prctl command that returns the PID of the tracer if any.
>
> prctl?
>
> > That allows for much simpler logic in the user land, and makes it
> > possible to detect tracer presence even if PROC_FS is not enabled.
>
> You should probably move the links from 0/1 to the changelog to make
> it more convincing.
>
> > +	if (request == PTRACE_TRACER) {
> > +		rcu_read_lock();
> > +		tracer = ptrace_parent(current);
> > +		ret = tracer ? task_pid_nr_ns(tracer,
> > +					task_active_pid_ns(current->parent)) : -ESRCH;
>
> The namespace is wrong, we need task_active_pid_ns(current). So this
> code should simply do task_tgid_vnr(tracer) like sys_getppid() does.
> And to me it would be better to return 0 if !current->ptrace.
>
> > +		rcu_read_unlock();
> > +		goto out;
>
> Wrong, this code runs after "child = find_get_task_by_vpid(pid);" above.
>
> And why? perhaps the intent was to check if this child is traced, not
> current?
>
> Oleg.
>

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Linus Torvalds]

On Fri, 6 Sept 2024 at 04:24, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Add cc's. Perhaps someone else can ack/nack the intent...
>
> This (trivial) patch is obviously buggy, but fixable. I won't argue
> if it can help userspace.

I think the "what's the point for user space" is the much more important thing.

Honestly, acting differently when traced sounds like a truly
fundamentally HORRIBLE model for anything at all - much less debugging
- and I think it should not be helped in any way unless you have some
really really strong arguments for it.

Can you figure it out as-is? Sure. But that's still not a reason to
make bad behavior _easier_.

               Linus

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 9/6/2024 12:09 PM, Linus Torvalds wrote:
> On Fri, 6 Sept 2024 at 04:24, Oleg Nesterov <oleg@redhat.com> wrote:
>>
>> Add cc's. Perhaps someone else can ack/nack the intent...
>>
>> This (trivial) patch is obviously buggy, but fixable. I won't argue
>> if it can help userspace.
> 
> I think the "what's the point for user space" is the much more important thing.
> 
> Honestly, acting differently when traced sounds like a truly
> fundamentally HORRIBLE model for anything at all - much less debugging
> - and I think it should not be helped in any way unless you have some
> really really strong arguments for it.
> 
> Can you figure it out as-is? Sure. But that's still not a reason to
> make bad behavior _easier_.

No dispute that altering behavior based on whether a process is traced
or not _is_ bad behavior. To be precise, when the process is still
doing work, it undoubtedly is.

When the process has run into a fatal error and is about to exit, having
a way to break into the debugger at this exact moment wouldn't change
anything about the logic of the data processing happening in the process.
What's so horrible in that to have a way to land in the debugger to see
what exactly is going on?

Another aid of a similar kind is logging. Sure, can figure out what's
the bug without it. It is easier to with it though, and logging might
change resource consumption so much more than the check for the tracer
being present when the process is dying.

All told, let me know if I may proceed with fixing the code as Oleg
suggested, or this piece should go into the waste basket. I could make
an argument that providing the way to get the tracer PID only via
proc FS through parsing text is more like shell/Perl/Python interface
to the kernel, and for compiled languages could have what's easier in
that setting (there is an easy syscall for getting PID, and there could
be code changing the logic on the PID being odd or even for the sake
of argument).

> 
>                 Linus

-- 
Thank you,
Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Linus Torvalds]

On Fri, 6 Sept 2024 at 13:08, Roman Kisel <romank@linux.microsoft.com> wrote:
>
> When the process has run into a fatal error and is about to exit, having
> a way to break into the debugger at this exact moment wouldn't change
> anything about the logic of the data processing happening in the process.
> What's so horrible in that to have a way to land in the debugger to see
> what exactly is going on?

I don't buy it.

If you want to debug some fatal behavior, and a debugger *isn't*
attached, you want it to create a core-dump.

And if a debugger *is* attached, it will catch that.

This is basically how abort() has always worked, and it very much is
*not* doing some "let's check if we're being debugged" stuff. Exactly
because that would be a bad idea and an anti-pattern.

The other very traditional model - for example if you do *not* want to
do core-dumps for some reason, and just exit with an error message -
is to just put a breakpoint on the "fatal()" function (or whatever
your "fatal error happened" situation is) using the debugger.

Then the target will behave differently depending on whether it is
being debugged or not BECAUSE THE DEBUGGER ASKED FOR IT, not because
the program decided to act differently when debugged.

In other words, this is a long-solved problem with solid solutions
from before Linux even existed.

              Linus

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

Well, I leave this to you and Linus (and other reviewers), but if it was not
clear I too do not really like this feature, that is why I added cc's.

Perhaps it makes sense to discuss the alternatives? Say, a process can have a
please_insert_the_breakpoint_here() function implemented in asm which just does
asm(ret).

Then something like

	#define breakpoint_if_debugging()	\
		asm volatile ("call please_insert_the_breakpoint_here" : ASM_CALL_CONSTRAINT);

if the process is ptraced, debugger can insert the breakoint into
please_insert_the_breakpoint_here(). Otherwise breakpoint_if_debugging()
is a cheap nop.

Not that I think this is a good idea, but std::breakpoint_if_debugging()
looks even more strange to me...

Oleg.

On 09/06, Roman Kisel wrote:
>
> All told, let me know if I may proceed with fixing the code as Oleg
> suggested, or this piece should go into the waste basket. I could make
> an argument that providing the way to get the tracer PID only via
> proc FS through parsing text is more like shell/Perl/Python interface
> to the kernel, and for compiled languages could have what's easier in
> that setting (there is an easy syscall for getting PID, and there could
> be code changing the logic on the PID being odd or even for the sake
> of argument).
> 
> >
> >                Linus
> 
> -- 
> Thank you,
> Roman
> 

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 9/6/2024 1:26 PM, Linus Torvalds wrote:
> On Fri, 6 Sept 2024 at 13:08, Roman Kisel <romank@linux.microsoft.com> wrote:
>>
>> When the process has run into a fatal error and is about to exit, having
>> a way to break into the debugger at this exact moment wouldn't change
>> anything about the logic of the data processing happening in the process.
>> What's so horrible in that to have a way to land in the debugger to see
>> what exactly is going on?
> 
> I don't buy it.
> 
> If you want to debug some fatal behavior, and a debugger *isn't*
> attached, you want it to create a core-dump.
> 
> And if a debugger *is* attached, it will catch that.
> 
> This is basically how abort() has always worked, and it very much is
> *not* doing some "let's check if we're being debugged" stuff. Exactly
> because that would be a bad idea and an anti-pattern.
> 
> The other very traditional model - for example if you do *not* want to
> do core-dumps for some reason, and just exit with an error message -
> is to just put a breakpoint on the "fatal()" function (or whatever
> your "fatal error happened" situation is) using the debugger.
> 
> Then the target will behave differently depending on whether it is
> being debugged or not BECAUSE THE DEBUGGER ASKED FOR IT, not because
> the program decided to act differently when debugged.
> 
> In other words, this is a long-solved problem with solid solutions
> from before Linux even existed.


Writing a core-dump might not be an option as you have pointed out
hence the "fatal()" function might not be permitted to fault.

Breaking into the debugger if it is attached saves a step of setting
a breakpoint and doesn't require the knowledge of the guts of the
standard library and/or the journey of the trap exception from the
CPU to the debugger. The very name of the "fatal()" function is a
tight contract, and something akin to the onion in the varnish for
the uninitiated.

Libraries like Google Breakpad, Boost.Test, C++ std, AWS C SDK,
Unreal Engine have that "breakpoint_if_debugging" facility, folks
find that useful. They all read and parse the "/proc/self/status" file,
where as it may seem, just the ptrace syscall one liner could save
the trouble of that. The kernel helps the user space do work as I
understand, why police it? There is "fork()", and threads can deadlock.
Quite horrible, still the user space has access to that. Here,
there is evidence folks do want to detect if a debugger is present,
there is evidence how the kernel can help the user space compute that
with so much less effort, the patch is trivial. Why don't let the
userspace burn less electricity?

> 
>                Linus

-- 
Thank you,
Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 9/6/2024 1:55 PM, Oleg Nesterov wrote:
> Well, I leave this to you and Linus (and other reviewers), but if it was not
> clear I too do not really like this feature, that is why I added cc's.
> 
Appreciate you time and help in understanding the Linux kernel code
better!

> Perhaps it makes sense to discuss the alternatives? Say, a process can have a
> please_insert_the_breakpoint_here() function implemented in asm which just does
> asm(ret).
> 
> Then something like
> 
> 	#define breakpoint_if_debugging()	\
> 		asm volatile ("call please_insert_the_breakpoint_here" : ASM_CALL_CONSTRAINT);
> 
> if the process is ptraced, debugger can insert the breakoint into
> please_insert_the_breakpoint_here(). Otherwise breakpoint_if_debugging()
> is a cheap nop.
> 
> Not that I think this is a good idea, but std::breakpoint_if_debugging()
> looks even more strange to me...
Can't speak for everyone obviously, I've found that convenient
when making sense of large (unknown) codebases instead of setting
up breakpoints and adding prints/logs, and when the process
can't/doesn't fault when it encounters a fatal error.

> 
> Oleg.
> 
> On 09/06, Roman Kisel wrote:
>>
>> All told, let me know if I may proceed with fixing the code as Oleg
>> suggested, or this piece should go into the waste basket. I could make
>> an argument that providing the way to get the tracer PID only via
>> proc FS through parsing text is more like shell/Perl/Python interface
>> to the kernel, and for compiled languages could have what's easier in
>> that setting (there is an easy syscall for getting PID, and there could
>> be code changing the logic on the PID being odd or even for the sake
>> of argument).
>>
>>>
>>>                 Linus
>>
>> -- 
>> Thank you,
>> Roman
>>

-- 
Thank you,
Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[kernel test robot]

Hi Roman,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ad618736883b8970f66af799e34007475fe33a68]

url:    https://github.com/intel-lab-lkp/linux/commits/Roman-Kisel/ptrace-Get-tracer-PID-without-reliance-on-the-proc-FS/20240906-085121
base:   ad618736883b8970f66af799e34007475fe33a68
patch link:    https://lore.kernel.org/r/20240905212741.143626-2-romank%40linux.microsoft.com
patch subject: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS
config: x86_64-randconfig-122-20240907 (https://download.01.org/0day-ci/archive/20240908/202409080315.xFbJCdN5-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409080315.xFbJCdN5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409080315.xFbJCdN5-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   kernel/ptrace.c:55:22: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/ptrace.c:55:22: sparse:    struct task_struct *
   kernel/ptrace.c:55:22: sparse:    struct task_struct [noderef] __rcu *
   kernel/ptrace.c:74:23: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct task_struct [noderef] __rcu *parent @@     got struct task_struct *new_parent @@
   kernel/ptrace.c:74:23: sparse:     expected struct task_struct [noderef] __rcu *parent
   kernel/ptrace.c:74:23: sparse:     got struct task_struct *new_parent
   kernel/ptrace.c:75:29: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct cred const [noderef] __rcu *ptracer_cred @@     got struct cred const * @@
   kernel/ptrace.c:75:29: sparse:     expected struct cred const [noderef] __rcu *ptracer_cred
   kernel/ptrace.c:75:29: sparse:     got struct cred const *
   kernel/ptrace.c:129:18: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct cred const *old_cred @@     got struct cred const [noderef] __rcu *ptracer_cred @@
   kernel/ptrace.c:129:18: sparse:     expected struct cred const *old_cred
   kernel/ptrace.c:129:18: sparse:     got struct cred const [noderef] __rcu *ptracer_cred
   kernel/ptrace.c:133:25: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:133:25: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:133:25: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:160:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:160:27: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:160:27: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:192:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:192:28: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:192:28: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:198:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:198:30: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:198:30: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:251:44: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/ptrace.c:251:44: sparse:    struct task_struct [noderef] __rcu *
   kernel/ptrace.c:251:44: sparse:    struct task_struct *
   kernel/ptrace.c:494:54: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct task_struct *parent @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/ptrace.c:494:54: sparse:     expected struct task_struct *parent
   kernel/ptrace.c:494:54: sparse:     got struct task_struct [noderef] __rcu *parent
   kernel/ptrace.c:502:53: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected struct task_struct *new_parent @@     got struct task_struct [noderef] __rcu *real_parent @@
   kernel/ptrace.c:502:53: sparse:     expected struct task_struct *new_parent
   kernel/ptrace.c:502:53: sparse:     got struct task_struct [noderef] __rcu *real_parent
   kernel/ptrace.c:550:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct task_struct *p1 @@     got struct task_struct [noderef] __rcu *real_parent @@
   kernel/ptrace.c:550:41: sparse:     expected struct task_struct *p1
   kernel/ptrace.c:550:41: sparse:     got struct task_struct [noderef] __rcu *real_parent
   kernel/ptrace.c:552:50: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct sighand_struct *sigh @@     got struct sighand_struct [noderef] __rcu *sighand @@
   kernel/ptrace.c:552:50: sparse:     expected struct sighand_struct *sigh
   kernel/ptrace.c:552:50: sparse:     got struct sighand_struct [noderef] __rcu *sighand
   kernel/ptrace.c:743:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:743:37: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:743:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:751:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:751:39: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:751:39: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:862:29: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:862:29: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:862:29: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:866:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:866:31: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:866:31: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:1096:37: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:1096:37: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:1096:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:1098:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *lock @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:1098:39: sparse:     expected struct spinlock [usertype] *lock
   kernel/ptrace.c:1098:39: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c: note: in included file (through include/linux/rcuwait.h, include/linux/percpu-rwsem.h, include/linux/fs.h, ...):
   include/linux/sched/signal.h:754: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:754:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:754:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:380:30: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct spinlock [usertype] *l @@     got struct spinlock [noderef] __rcu * @@
   kernel/ptrace.c:380:30: sparse:     expected struct spinlock [usertype] *l
   kernel/ptrace.c:380:30: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:456:17: sparse: sparse: context imbalance in 'ptrace_attach' - different lock contexts for basic block
   kernel/ptrace.c:500:38: sparse: sparse: dereference of noderef expression
   include/linux/sched/signal.h:754: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:754:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:754:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:690:9: sparse: sparse: context imbalance in 'ptrace_getsiginfo' - different lock contexts for basic block
   include/linux/sched/signal.h:754: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:754:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:754:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:706:9: sparse: sparse: context imbalance in 'ptrace_setsiginfo' - different lock contexts for basic block
   include/linux/sched/signal.h:754: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:754:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:754:37: sparse:     got struct spinlock [noderef] __rcu *
   include/linux/sched/signal.h:754: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:754:37: sparse:     expected struct spinlock [usertype] *lock
   include/linux/sched/signal.h:754:37: sparse:     got struct spinlock [noderef] __rcu *
   kernel/ptrace.c:1255:9: sparse: sparse: context imbalance in 'ptrace_request' - different lock contexts for basic block
>> kernel/ptrace.c:1284:67: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct task_struct *tsk @@     got struct task_struct [noderef] __rcu *parent @@
   kernel/ptrace.c:1284:67: sparse:     expected struct task_struct *tsk
   kernel/ptrace.c:1284:67: sparse:     got struct task_struct [noderef] __rcu *parent

vim +1284 kernel/ptrace.c

  1257	
  1258	SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
  1259			unsigned long, data)
  1260	{
  1261		struct task_struct *child, *tracer;
  1262		long ret;
  1263	
  1264		if (request == PTRACE_TRACEME) {
  1265			ret = ptrace_traceme();
  1266			goto out;
  1267		}
  1268	
  1269		child = find_get_task_by_vpid(pid);
  1270		if (!child) {
  1271			ret = -ESRCH;
  1272			goto out;
  1273		}
  1274	
  1275		if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
  1276			ret = ptrace_attach(child, request, addr, data);
  1277			goto out_put_task_struct;
  1278		}
  1279	
  1280		if (request == PTRACE_TRACER) {
  1281			rcu_read_lock();
  1282			tracer = ptrace_parent(current);
  1283			ret = tracer ? task_pid_nr_ns(tracer,
> 1284						task_active_pid_ns(current->parent)) : -ESRCH;
  1285			rcu_read_unlock();
  1286			goto out;
  1287		}
  1288	
  1289		ret = ptrace_check_attach(child, request == PTRACE_KILL ||
  1290					  request == PTRACE_INTERRUPT);
  1291		if (ret < 0)
  1292			goto out_put_task_struct;
  1293	
  1294		ret = arch_ptrace(child, request, addr, data);
  1295		if (ret || request != PTRACE_DETACH)
  1296			ptrace_unfreeze_traced(child);
  1297	
  1298	 out_put_task_struct:
  1299		put_task_struct(child);
  1300	 out:
  1301		return ret;
  1302	}
  1303	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

On 09/06, Roman Kisel wrote:
>
> On 9/6/2024 1:55 PM, Oleg Nesterov wrote:
> >
> >Not that I think this is a good idea, but std::breakpoint_if_debugging()
> >looks even more strange to me...
> Can't speak for everyone obviously, I've found that convenient
> when making sense of large (unknown) codebases instead of setting
> up breakpoints and adding prints/logs, and when the process
> can't/doesn't fault when it encounters a fatal error.

Sorry, I don't understand.

I fail to understand how/why people can use std::breakpoint_if_debugging().
To me it doesn't look useful at all.

But you can safely ignore me, I do not pretend I understand the userspace's
needs.

And I guess people will use it anyway, so I won't argue with, say, a trivial
patch which just adds

	case PR_GET_PTRACED:
		error = !!current->ptrace;
		break;

into sys_prctl(), even if I agree that this probably just makes bad behavior
easier.

But you need to convince Linus.

Oleg.

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 9/8/2024, Oleg Nesterov wrote:

> On 09/06, Roman Kisel wrote:
> >
> > On 9/6/2024 1:55 PM, Oleg Nesterov wrote:
> > >
> > >Not that I think this is a good idea, but std::breakpoint_if_debugging()
> > >looks even more strange to me...
> > Can't speak for everyone obviously, I've found that convenient
> > when making sense of large (unknown) codebases instead of setting
> > up breakpoints and adding prints/logs, and when the process
> > can't/doesn't fault when it encounters a fatal error.
>
> Sorry, I don't understand.
>
> I fail to understand how/why people can use std::breakpoint_if_debugging().
> To me it doesn't look useful at all.
>
> But you can safely ignore me, I do not pretend I understand the userspace's
> needs.
>
> And I guess people will use it anyway, so I won't argue with, say, a trivial
> patch which just adds
> 
> case PR_GET_PTRACED:
>     error = !!current->ptrace;
>     break;
>
> into sys_prctl(), even if I agree that this probably just makes bad behavior
> easier.

Very kind of you trying to build a longer table rather than a taller fence,
I appreciate that very much! Your aproach looks very neat indeed, I've learned
a lot from all sugestions you have shared.

>
> But you need to convince Linus.

No new evidence, I rest my case. The difference seems to be a
matter of on which which set of axioms one builds the theorems,
and these sets come across as non-compatible. It might be prudent
to repack these to give this some final thoughts and move on.
Hoping this might be interesting to folks whose service to humanity
requires wearing kernel-tinted glasses.

I brought up evidence-based arguments of the change providing
benefits for the user space, and these were countered with "bad
behavior" on the grounds that changing program's behavior under
debugger is bad. Well, if it's bad for you, you won't do that.
The very notion of convincing becomes devoid of sense in this
situation.

Good for us to land in the debugger at the point of panic without
fiddling with the breakpoints; we're going to continue enjoying that.
Also good to slip in `std::breakpoint_if_debugging` in some obscure
function in someone's library to see how the execution gets there
instead of figuring out through which pointers it is called and what
full name like  `A::B::C::X::Y::Z::func::{impl #12}()` needs to be
used for the breakpoint. Not having to use/know the trap instruction
mnemonic for the target architecture feels not all that bad in the
user space.

That all is very different for the kernel: one C aka portable assembly
codebase that runs the world and where change might be hard, no deadline
or time-to-market as no one sells the kernel as a commodity. Producing
more software faster requires farming things off to the toolchain
(like memory management) or to the 3rd party libraries (to be as general
as possible they use abstractions of abstractions of abstractions cooked
on vtabless, generics and traits), and these present a complication when
debugging. Sure can use a demangled name for the "fatal()" function,
then again why bother and learn the name of that function and making that
into a hard dependency?

Gdb has got aids to land at the `main` function (the `start` command),
why so much ink is being spent of the aids for panic?

It was told in the discussion that all these problems had been solved
before Linux existed. I dare to say the more precise statement would
be that they were solved for a different world. Some 40-30 years
after we live in the world eaten by the software and automation and
figuring out where the bug is can be helped by different means better.

Besides bugs in the patch, why would the kernel even _care_ if/how
the user land uses that tracer PID? That is not sensitive data, and
the user land has access to that already, and uses it in numeruous
large libraries (not just some scrawny pet project of mine) but only
via proc FS and parsing the "/proc/self/status" text pseudo-file.

The kernel could be like "dear user land, here is your sweet one liner
for getting the tracer PID, go knock yourself out reading the tracer
PIDs all day long". And the user land be like "oh, thank you, dear
kernel, one can always count on you!". Win-win. Easy. Joyous.

If folks are still reading or jumped over this long wall of text to
the last paragraph, color me clueless as to why providing a simpler
interface for such an inconsequntial thing as the tracer PID instead
of the existing convoluted one (nailed to proc FS, too) needs to be
resolved by the project's BDFL. Excuse my sudden loss of eloquence
but LOL WUT!

>
> Oleg.

Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Eric W. Biederman]

Roman Kisel <romank@linux.microsoft.com> writes:

> On 9/6/2024 1:26 PM, Linus Torvalds wrote:
>> On Fri, 6 Sept 2024 at 13:08, Roman Kisel <romank@linux.microsoft.com> wrote:
>>>
>>> When the process has run into a fatal error and is about to exit, having
>>> a way to break into the debugger at this exact moment wouldn't change
>>> anything about the logic of the data processing happening in the process.
>>> What's so horrible in that to have a way to land in the debugger to see
>>> what exactly is going on?
>> I don't buy it.
>> If you want to debug some fatal behavior, and a debugger *isn't*
>> attached, you want it to create a core-dump.
>> And if a debugger *is* attached, it will catch that.
>> This is basically how abort() has always worked, and it very much is
>> *not* doing some "let's check if we're being debugged" stuff. Exactly
>> because that would be a bad idea and an anti-pattern.
>> The other very traditional model - for example if you do *not* want to
>> do core-dumps for some reason, and just exit with an error message -
>> is to just put a breakpoint on the "fatal()" function (or whatever
>> your "fatal error happened" situation is) using the debugger.
>> Then the target will behave differently depending on whether it is
>> being debugged or not BECAUSE THE DEBUGGER ASKED FOR IT, not because
>> the program decided to act differently when debugged.
>> In other words, this is a long-solved problem with solid solutions
>> from before Linux even existed.
>
>
> Writing a core-dump might not be an option as you have pointed out
> hence the "fatal()" function might not be permitted to fault.

For that you just need to set core file size rlimit to 0.
Then you can safely raise SIGABRT to terminate your process.

A debugger can also stop at PTRACE_EVENT_EXIT.  The process
is still available (not cleaned up), but already fatally dead.

So I think the scenario of a process exiting is safely handled.

So if that is the case you care about I would say please look at
PTRACE_EVENT_EXIT.

> Breaking into the debugger if it is attached saves a step of setting
> a breakpoint and doesn't require the knowledge of the guts of the
> standard library and/or the journey of the trap exception from the
> CPU to the debugger. The very name of the "fatal()" function is a
> tight contract, and something akin to the onion in the varnish for
> the uninitiated.
>
> Libraries like Google Breakpad, Boost.Test, C++ std, AWS C SDK,
> Unreal Engine have that "breakpoint_if_debugging" facility, folks
> find that useful. They all read and parse the "/proc/self/status" file,
> where as it may seem, just the ptrace syscall one liner could save
> the trouble of that. The kernel helps the user space do work as I
> understand, why police it? There is "fork()", and threads can deadlock.
> Quite horrible, still the user space has access to that. Here,
> there is evidence folks do want to detect if a debugger is present,
> there is evidence how the kernel can help the user space compute that
> with so much less effort, the patch is trivial. Why don't let the
> userspace burn less electricity?

If you want more than just stopping at when a process exits, such as the
not yet standardized std:is_debugger_present and
std:breakpoint_if_debugging calls.  Then a real world justification
needs to be shown why the kernel should optimize for the uncommon case.

Especially because it will take time and energy and maintenance to keep
that going, all for to support a set of facilities that seem highly
dubious.

I suspect the best way to support breakpoint_if_debugging (in the
general case) is to make them nop functions, and then have something
scan the source code collect those locations, and feed those locations
into the debugger as break points.  Perhaps the compiler can be that
scanner and record the list of suggest break points somewhere that
a debugger can read.

As for std:is_debugger_present I suspect the best way to handle that
is to keep the current implementation, making it expensive to use so
people won't use it unthinkingly.  If people are using this facility
enough that they are wasting electricity, an expensive operation at
least has the potential to make people to stop and think about what they
are doing.

From a don't introduce heisenbug's perspective I think optimizing any of
this in the kernel looks like a bad idea.

Am I missing something?

Eric

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

On 09/09, Roman Kisel wrote:
>
> On 9/8/2024, Oleg Nesterov wrote:
>
> > But you can safely ignore me, I do not pretend I understand the userspace's
> > needs.
> >
> > And I guess people will use it anyway, so I won't argue with, say, a trivial
> > patch which just adds
> >
> > case PR_GET_PTRACED:
> >     error = !!current->ptrace;
> >     break;
> >
> > into sys_prctl(), even if I agree that this probably just makes bad behavior
> > easier.
>
> Very kind of you trying to build a longer table rather than a taller fence,
> I appreciate that very much! Your aproach looks very neat indeed,

Well, you didn't answer my question in
https://lore.kernel.org/all/20240906114819.GA20831@redhat.com/
so I decided that a simpler change which returns !!current->ptrace instead
of the tracer's pid might work as well.

Sorry for annoying you.

Oleg.

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 09/09, Oleg Nesterov wrote:

> On 09/09, Roman Kisel wrote:
> >
> > On 9/8/2024, Oleg Nesterov wrote:
> >
> > > But you can safely ignore me, I do not pretend I understand the userspace's
> > > needs.
> > >
> > > And I guess people will use it anyway, so I won't argue with, say, a trivial
> > > patch which just adds
> > >
> > > case PR_GET_PTRACED:
> > >     error = !!current->ptrace;
> > >     break;
> > >
> > > into sys_prctl(), even if I agree that this probably just makes bad behavior
> > > easier.
> >
> > Very kind of you trying to build a longer table rather than a taller fence,
> > I appreciate that very much! Your aproach looks very neat indeed,
> 
> Well, you didn't answer my question in
> https://lore.kernel.org/all/20240906114819.GA20831@redhat.com/
> so I decided that a simpler change which returns !!current->ptrace instead
> of the tracer's pid might work as well.
> 

Apologies for that! After Linus had been added, I braced for the impact as
obviously I was not fixing anything urgent or making some breakthrough
deserving such attention. I guess I got my 101 on adding code to "./kernel" :D

> Sorry for annoying you.
> 
Sorry if my response carried that connotation. I indeed learned a lot from
your suggestions. I'll make sure to write better.

> Oleg.

Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Oleg Nesterov]

On 09/09, Eric W. Biederman wrote:
>
> I suspect the best way to support breakpoint_if_debugging (in the
> general case) is to make them nop functions,

or may be make it call a single function which can be used as a
breakpoint placeholder.

Either way, at least the program with breakpoint_if_debugging() will
survive under /usr/bin/strace.

Oleg.

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Roman Kisel]

On 9/9/2024, Eric wrote:

> > On 9/6/2024 1:26 PM, Linus Torvalds wrote:
> >> On Fri, 6 Sept 2024 at 13:08, Roman Kisel <romank@linux.microsoft.com> wrote:
> >>>
> >>> When the process has run into a fatal error and is about to exit, having
> >>> a way to break into the debugger at this exact moment wouldn't change
> >>> anything about the logic of the data processing happening in the process.
> >>> What's so horrible in that to have a way to land in the debugger to see
> >>> what exactly is going on?
> >> I don't buy it.
> >> If you want to debug some fatal behavior, and a debugger *isn't*
> >> attached, you want it to create a core-dump.
> >> And if a debugger *is* attached, it will catch that.
> >> This is basically how abort() has always worked, and it very much is
> >> *not* doing some "let's check if we're being debugged" stuff. Exactly
> >> because that would be a bad idea and an anti-pattern.
> >> The other very traditional model - for example if you do *not* want to
> >> do core-dumps for some reason, and just exit with an error message -
> >> is to just put a breakpoint on the "fatal()" function (or whatever
> >> your "fatal error happened" situation is) using the debugger.
> >> Then the target will behave differently depending on whether it is
> >> being debugged or not BECAUSE THE DEBUGGER ASKED FOR IT, not because
> >> the program decided to act differently when debugged.
> >> In other words, this is a long-solved problem with solid solutions
> >> from before Linux even existed.
> >
> >
> > Writing a core-dump might not be an option as you have pointed out
> > hence the "fatal()" function might not be permitted to fault.
> 
> For that you just need to set core file size rlimit to 0.
> Then you can safely raise SIGABRT to terminate your process.
> 
> A debugger can also stop at PTRACE_EVENT_EXIT.  The process
> is still available (not cleaned up), but already fatally dead.
> 
> So I think the scenario of a process exiting is safely handled.
> 
> So if that is the case you care about I would say please look at
> PTRACE_EVENT_EXIT.
> 
> > Breaking into the debugger if it is attached saves a step of setting
> > a breakpoint and doesn't require the knowledge of the guts of the
> > standard library and/or the journey of the trap exception from the
> > CPU to the debugger. The very name of the "fatal()" function is a
> > tight contract, and something akin to the onion in the varnish for
> > the uninitiated.
> >
> > Libraries like Google Breakpad, Boost.Test, C++ std, AWS C SDK,
> > Unreal Engine have that "breakpoint_if_debugging" facility, folks
> > find that useful. They all read and parse the "/proc/self/status" file,
> > where as it may seem, just the ptrace syscall one liner could save
> > the trouble of that. The kernel helps the user space do work as I
> > understand, why police it? There is "fork()", and threads can deadlock.
> > Quite horrible, still the user space has access to that. Here,
> > there is evidence folks do want to detect if a debugger is present,
> > there is evidence how the kernel can help the user space compute that
> > with so much less effort, the patch is trivial. Why don't let the
> > userspace burn less electricity?
> 
> If you want more than just stopping at when a process exits, such as the
> not yet standardized std:is_debugger_present and
> std:breakpoint_if_debugging calls.  Then a real world justification
> needs to be shown why the kernel should optimize for the uncommon case.
> 
> Especially because it will take time and energy and maintenance to keep
> that going, all for to support a set of facilities that seem highly
> dubious.
> 
> I suspect the best way to support breakpoint_if_debugging (in the
> general case) is to make them nop functions, and then have something
> scan the source code collect those locations, and feed those locations
> into the debugger as break points.  Perhaps the compiler can be that
> scanner and record the list of suggest break points somewhere that
> a debugger can read.
> 
> As for std:is_debugger_present I suspect the best way to handle that
> is to keep the current implementation, making it expensive to use so
> people won't use it unthinkingly.  If people are using this facility
> enough that they are wasting electricity, an expensive operation at
> least has the potential to make people to stop and think about what they
> are doing.
> 
> From a don't introduce heisenbug's perspective I think optimizing any of
> this in the kernel looks like a bad idea.
> 
> Am I missing something?

This part "people won't use it unthinkingly" does not look too convincing
to me: why not add an expensive spin loop or a 1 second wait to all parts
of the kernel that must be used with caution? I am imagining flashing
L.E.D.s in the modern smartphone, sorry, couldn't help but imagine that :D.
I believe that's a matter of documentation and perfromance evaluation.

I see that the vote has been 3:1 in favor of not merging anything like that,
and no new ideas have surfaced so far to change this. Looks like could close
on this. Appreciate helping me see your points!

> 
> Eric

Roman

Re: [PATCH 1/1] ptrace: Get tracer PID without reliance on the proc FS

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 09/09, Eric W. Biederman wrote:
>>
>> I suspect the best way to support breakpoint_if_debugging (in the
>> general case) is to make them nop functions,
>
> or may be make it call a single function which can be used as a
> breakpoint placeholder.

That would be much simpler to implement.

> Either way, at least the program with breakpoint_if_debugging() will
> survive under /usr/bin/strace.

Yes, I can't imagine wanting any of that functionality to trigger
when it is just strace using ptrace on a program.

Eric