[DTrace-devel] [PATCH v4] fixup! proc: rip out waitfd() and hit waitpidding thread with a signal instead

[Kris Van Hees]

On Mon, Oct 23, 2023 at 04:25:06PM +0100, Nick Alcock via DTrace-devel wrote:
> Introduce and use a compiler barrier to prevent use of return_early's
> dereferent from being moved around.
> 
> We want this to be right next to the call to waitpid(): if the compiler
> moves it up, it widens a race window, and if it moves it down, it makes it
> useless.
> 
> Introduce a compiler barrier to make sure it does neither of these things.
> ---
>  include/sys/compiler.h | 4 +++-
>  libproc/Pcontrol.c     | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> Spotted over the weekend while compiling DTrace with LTO for the hell of it.
> This fixes an obvious miscompilation in that case (which, admittedly, is a
> case we don't care much about) and seems unlikely to do any harm.
> 
> diff --git a/include/sys/compiler.h b/include/sys/compiler.h
> index c3ffccfc0f9e0..82066e5e48d5f 100644
> --- a/include/sys/compiler.h
> +++ b/include/sys/compiler.h
> @@ -1,6 +1,6 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2011, 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.
>   */
> @@ -38,6 +38,7 @@
>  #define _dt_unused_ __attribute__((__unused__))
>  #define _dt_noreturn_ __attribute__((__noreturn__))
>  #define _dt_unlikely_(x) __builtin_expect((x),0)
> +#define _dt_barrier_(x) __asm__ __volatile__("": :"r"(x):"memory")
>  
>  #elif defined (__SUNPRO_C)
>  
> @@ -45,6 +46,7 @@
>  #define _dt_destructor_(x) _Pragma("fini(" #x ")")
>  #define _dt_noreturn_
>  #define _dt_unlikely_(x) (x)
> +#define _dt_barrier_(x)
>  
>  /*
>   * These are lint comments with no compiler equivalent.
> diff --git a/libproc/Pcontrol.c b/libproc/Pcontrol.c
> index 371b547c5e07b..79bcb5c24d34c 100644
> --- a/libproc/Pcontrol.c
> +++ b/libproc/Pcontrol.c
> @@ -691,11 +691,13 @@ Pwait_internal(struct ps_prochandle *P, boolean_t block, int *return_early)
>  		 * unlock again to minimize the size of the race window in which
>  		 * the signal might hit before waitpid() starts.)
>  		 */
> +		_dt_barrier_(*return_early);
>  		if (return_early && *return_early > 0) {

Um???  So, if return_early is NULL, that barier expression is really going
to pose a problem, right?  Like, SEGV?  Surely you need to check whether
return_early is NULL before doing the barrier here (and below).

>  			if (block && waitpid_lock_hook)
>  				waitpid_lock_hook(P, P->wrap_arg, 0);
>  			return 0;
>  		}
> +		_dt_barrier_(*return_early);
>  
>                  err = waitpid(P->pid, &status, __WALL | (!block ? WNOHANG : 0));
>  	
> 
> base-commit: cb3a9e81efd9d4fbc413b9427f86af079b5faab6
> prerequisite-patch-id: ce4dfaceb9baa012a354643f839cc413de5fd9d9
> prerequisite-patch-id: b525e5cb144cc160e18531916c8e4bba2666a39a
> -- 
> 2.42.0.271.g85384428f1
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel