Re: [PATCH v2] pid: annotate data-races around pid_ns->pid_allocated

[Oleg Nesterov <oleg@redhat.com>]

On 04/25, Jiayuan Chen wrote:
>
> @@ -2584,7 +2584,7 @@ __latent_entropy struct task_struct *copy_process(
>  	rseq_fork(p, clone_flags);
>
>  	/* Don't start children in a dying pid namespace */
> -	if (unlikely(!(ns_of_pid(pid)->pid_allocated & PIDNS_ADDING))) {
> +	if (unlikely(!(data_race(ns_of_pid(pid)->pid_allocated & PIDNS_ADDING)))) {

Well. data_race() just hides the potential problem. READ_ONCE() makes more
sense imo, even if I think there are no real problems with the current code.

Either way,

> @@ -271,13 +271,13 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
>  	upid = pid->numbers + ns->level;
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(&pidmap_lock);
> -	if (!(ns->pid_allocated & PIDNS_ADDING))
> +	if (!(data_race(ns->pid_allocated & PIDNS_ADDING)))

again, you do not need data_race() or READ_ONCE() if you read the
data protected by pidmap_lock. But you still need WRITE_ONCE() when
->pid_allocated is modified.

Oleg.