[PATCH 4/4] utrace_release_task: check REAP before utrace_reap()

[Oleg Nesterov]

On 09/07, Roland McGrath wrote:
>
> --- a/include/linux/tracehook.h
> +++ b/include/linux/tracehook.h
> @@ -362,7 +362,12 @@ static inline void tracehook_report_vfor
>   */
>  static inline void tracehook_prepare_release_task(struct task_struct *task)
>  {
> -	utrace_release_task(task);
> +	/*
> +	 * See kernel/utrace.c:utrace_attach_task() about this barrier.
> +	 */
> +	smp_mb();
> +	if (task_utrace_flags(task))
> +		utrace_release_task(task);

I think we can do this optimization, but see below.

> +	 * If it already could be getting reaped, we cannot attach now.
> +	 * The barrier ensures this check precedes utrace_add_engine()
> +	 * setting ->utrace_flags.  tracehook_prepare_release_task() has
> +	 * the corresponding barrier, after setting EXIT_DEAD and before
> +	 * checking ->utrace_flags to decide to call utrace_release_task().
> +	 */
> +	ret = target->exit_state;
> +	smp_mb();
> +
> +	if (unlikely(ret == EXIT_DEAD))
> +		return ERR_PTR((flags & UTRACE_ATTACH_CREATE) ?

No, I think this can't work.

The problem is, release and attach pathes set/check exit_state and
utrace_flags in the same order, mb() can't help.

release:

	->exit_state = EXIT_DEAD;
	mb();
	if (->utrace_flags)
		utrace_release_task();

utrace_attach_task:

	if (->exit_state)
		return -ERR;
	mb();
	->utrace_flags |= REAP;

Even simpler, imagine that attach checks ->exit_state == 0, then
the task exits and utrace_release_task() is called.



I need to re-check, but afaics we can do something like the patch
below. utrace_add_engine() sets REAP and _then_ checks exit_state.
This way, if we race with release path, either tracehook_prepare_release_task()
must notice REAP, or utrace_add_engine() must see EXIT_DEAD.


But. In any case, I always dreamed to kill the current EXIT_DEAD
check in utrace_attach_task. Like the previous EXIT_DEAD check which
we already killed, it buys nothing imho but complicates the code.
This case is very unlikely and the check is not needed for correctness.
IOW, this check just slowns down the likely case.

Oleg.

--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -139,7 +139,7 @@ static int utrace_add_engine(struct task
 			     const struct utrace_engine_ops *ops,
 			     void *data)
 {
-	int ret;
+	int ret = 0;
 
 	spin_lock(&utrace->lock);
 
@@ -168,10 +168,19 @@ static int utrace_add_engine(struct task
 		 * In case we had no engines before, make sure that
 		 * utrace_flags is not zero.
 		 */
-		target->utrace_flags |= UTRACE_EVENT(REAP);
-		list_add_tail(&engine->entry, &utrace->attaching);
-		utrace->pending_attach = 1;
-		ret = 0;
+		if (!target->utrace_flags) {
+			target->utrace_flags = UTRACE_EVENT(REAP);
+			smp_mb();
+			if (unlikely(target->exit_state == EXIT_DEAD)) {
+				target->utrace_flags = 0;
+				ret = -ESRCH;
+			}
+		}
+
+		if (!ret) {
+			list_add_tail(&engine->entry, &utrace->attaching);
+			utrace->pending_attach = 1;
+		}
 	}
 
 	spin_unlock(&utrace->lock);