On 02/23, Pavel Tikhomirov wrote:
>
> To avoid possible problems related to cpu/compiler optimizations around
> ->child_reaper, let's use WRITE_ONCE (additional to task_list lock)
> everywhere we write it and use READ_ONCE everywhere we read it without
> explicit lock.
Yes, this is what I meant... but I can never recall if READ_ONCE() alone
is enough to make KCSAN happy...
I won't insist, but I think it would be better to do this in a separate
change for documenation purposes and for discussion.
> @@ -247,8 +247,9 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t
> *arg_set_tid,
> * alreay in use. Return EEXIST in that case.
> */
> if (nr == -ENOSPC)
> -
> nr = -EEXIST;
> + } else if (!READ_ONCE(tmp->child_reaper) &&
> idr_get_cursor(&tmp->idr) != 0) {
> + nr = -EINVAL;
> } else {
Oh, this doesn't look clear/clean... This even looks racy even if it is not.
Can you move this check into the "else" branch which does another get_cursor
and unify this check with the RESERVED_PIDS check?
Either way, I don't like the fact we check ->child_reaper != NULL twice.
Perhaps something like the preparational patch below makes sense ? Not
sure this is actually better...
Oleg.
--- x/kernel/pid.c
+++ x/kernel/pid.c
@@ -215,12 +215,6 @@ struct pid *alloc_pid(struct pid_namespa
retval = -EINVAL;
if (tid < 1 || tid >= pid_max[ns->level - i])
goto out_abort;
- /*
- * Also fail if a PID != 1 is requested and
- * no PID 1 exists.
- */
- if (tid != 1 && !tmp->child_reaper)
- goto out_abort;
retval = -EPERM;
if (!checkpoint_restore_ns_capable(tmp->user_ns))
goto out_abort;
@@ -299,6 +293,11 @@ struct pid *alloc_pid(struct pid_namespa
tmp = tmp->parent;
i--;
retried_preload = false;
+
+ if (!READ_ONCE(tmp->child_reaper) && nr != 1) {
+ retval = -EINVAL;
+ goto out_free;
+ }
}
/*
