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

[Nick Alcock]

On 25 Oct 2023, Kris Van Hees verbalised:

> On Mon, Oct 23, 2023 at 04:25:06PM +0100, Nick Alcock via DTrace-devel wrote:
>>  /*
>>   * 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).

Uh, right. I pushed the wrong commit, sorry!

Obvious fix:

>From 6dcbe39e636ed1bd8132e2861b26eeeea1638546 Mon Sep 17 00:00:00 2001
From: Nick Alcock <nick.alcock at oracle.com>
Date: Mon, 23 Oct 2023 13:49:36 +0100
Subject: [PATCH v4] fixup! proc: rip out waitfd() and hit waitpidding thread
 with a signal instead

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

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..97735c848cf84 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) {
 			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