Re: [PATCH v2 1/2] pid_namespace: allow opening pid_for_children before init was created

[Oleg Nesterov <oleg@redhat.com>]

On 02/24, Pavel Tikhomirov wrote:
>
> On 2/24/26 13:09, Oleg Nesterov wrote:
> > 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...
>
> AFAICS this should be fine for memory safety of accesses to ->child_reaper.
> I would love if someone more experienced in this area would confirm.

__READ_ONCE() uses volatile cast, DEFINE_TSAN_VOLATILE_READ_WRITE()
will pass KCSAN_ACCESS_ATOMIC to check_access(), so it seems that
READ_ONCE() should be enough...

But I am not sure, I don't really understand this code.

> > I won't insist, but I think it would be better to do this in a separate
> > change for documenation purposes and for discussion.
>
> Ok, will do. It will be a bit ugly as I will first add READ_ONCE to the
> pidns_for_children_get() and then remove the hunk with it in the next patch.

Agreed, this is ugly. I almost regret I mentioned _ONCE() in the previous
discussions, I only tried to "nack" another read_lock(tasklist).

So lets avoid a separate change and WRITE_ONCE()'s in copy_process/find_child_reaper,
we can add them later if KCSAN complains, they are not needed for correctness.

But up to you, I am fine either way.

> > Perhaps something like the preparational patch below makes sense ? Not
> > sure this is actually better...
>
> This looks more universal at least, as instead of two checks we have one in one
> place. My only concern of putting the check where I put it was to avoid extra
> idr_alloc_cyclic() + idr_remove(), if we already know we don't need it. But it's
> only in last pid_namespace we can have ->child_reaper unset so we do alloc/remove
> for all other namespaces anyway in error case, should not be a big deal.

Yes...

> I will add the preparation patches: for below patch and related to _ONCE.

Again, up to you. But either way it would be nice to have a comment or at
least a note in the changelog to explain that this is also needed to avoid
the race between alloc_pid() + fail and another alloc_pid(). This is subtle.

Oleg.