[DTrace-devel] [PATCH 08/10] proc: don't ptrace shortlived-grabbed processes if someone else is ptracing them

[Kris Van Hees]

On Wed, Aug 02, 2023 at 02:26:58PM +0100, Nick Alcock via DTrace-devel wrote:
> Shortlived grabs, by definition, are grabs that won't last long and where
> we don't expect things like dlopen monitoring to be useful.  So it probably
> makes sense not to require such grabs to ptrace(): if they *can*, that's
> good because it means we can extract dynamic loader scope info and take
> things like symbol interposition into accout when doing symbol lookup,
> but if someone else is already tracing the process we should probably just
> try not ptracing it.
> 
> This makes it practical to have multiple dtraces USDT-probing the same
> process at the same time: one of them will ptrace it, the other one won't,
> and both will get the mapping info they need for a reliable trace.

This description seems to be (at least partly) conflicting... Maybe also due
to the language use 'probably' etc.  Either it does or it doesn't, right?  If
it is good to be able to ptrace() because it affects symbol lookup, then how
can you say that it doesn't matter if one DTrace can pytrace() the process
while others cannot.  It would seem that the one that can ptrace() will get
better information than the others.

I think we need more specifics here.  Also, is the lack of this patch visible
in any form of test?

> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>  libdtrace/dt_proc.c | 18 +++++++++++++-----
>  libproc/Pcontrol.c  | 18 ++++++++++++++++++
>  libproc/libproc.h   |  1 +
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/libdtrace/dt_proc.c b/libdtrace/dt_proc.c
> index bb0253f1b6b2..f531329db2cd 100644
> --- a/libdtrace/dt_proc.c
> +++ b/libdtrace/dt_proc.c
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> @@ -894,16 +894,24 @@ dt_proc_control(void *arg)
>  		 * be worth ptracing it so that we get better symbol resolution,
>  		 * but if the process is a crucial system daemon, avoid ptracing
>  		 * it entirely, to avoid rtld_db dropping breakpoints in crucial
> -		 * system daemons unless specifically demanded.  No death
> -		 * notification is ever sent.
> +		 * system daemons unless specifically demanded.  Also avoid
> +		 * ptracing if the process is already being traced by someone
> +		 * else (like another DTrace instance).  No death notification
> +		 * is ever sent.
>  		 *
>  		 * Also, obviously enough, never drop breakpoints in ourself!
>  		 */
>  		if (datap->dpcd_flags & DTRACE_PROC_SHORTLIVED) {
> -			noninvasive = 1;
> +			pid_t tracer_pid;
> +
> +                        noninvasive = 1;
>  			dpr->dpr_notifiable = 0;
> -			if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
> +			tracer_pid = Ptracer_pid(dpr->dpr_pid);
> +
> +                        if ((Psystem_daemon(dpr->dpr_pid, dtp->dt_useruid,
>  				    dtp->dt_sysslice) > 0) ||
> +			    ((tracer_pid != 0) &&
> +			     (tracer_pid != getpid())) ||
>  			    (dpr->dpr_pid == getpid()))
>  				noninvasive = 2;
>  		}
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index 9bdf2068478c..c9a4847cdb94 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -2775,6 +2775,24 @@ Puid(pid_t pid)
>  	return uid;
>  }
>  
> +/*
> + * Determine if this process is already being ptraced, and if so, the pid of the
> + * ptracer.  The caller should eschew invasive tracing in this case.
> + */
> +pid_t
> +Ptracer_pid(pid_t pid)
> +{
> +	char *traced;
> +	pid_t tracer_pid;
> +
> +	if ((traced = Pget_proc_status(pid, "TracerPid")) == NULL)
> +		return 0;
> +
> +	tracer_pid = strtol(traced, NULL, 10);
> +	free(traced);
> +	return tracer_pid;
> +}
> +
>  /*
>   * Return 1 if this process is a system daemon, 0 if it is not, and -1 on error.
>   *
> diff --git a/libproc/libproc.h b/libproc/libproc.h
> index 9f434fcaab22..761a0a0bdfff 100644
> --- a/libproc/libproc.h
> +++ b/libproc/libproc.h
> @@ -100,6 +100,7 @@ extern	void	Pset_procfs_path(const char *);
>  extern	int	Pdynamically_linked(struct ps_prochandle *);
>  extern	int	Ptraceable(struct ps_prochandle *);
>  extern	int	Pelf64(struct ps_prochandle *);
> +extern	pid_t	Ptracer_pid(pid_t);
>  
>  /*
>   * Calls that do not take a process structure.  These are used to determine
> -- 
> 2.41.0.270.g68fa1d84b5
> 
>