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

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

[Jiayuan Chen]

Suppress syzbot reports by annotating these accesses using data_race().

Reported-by: syzbot+adcaa842b762a1762e7d@syzkaller.appspotmail.com
Reported-by: syzbot+fab52e3459fa2f95df57@syzkaller.appspotmail.com
Reported-by: syzbot+0718f65353d72efaac1e@syzkaller.appspotmail.com
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 kernel/fork.c          | 2 +-
 kernel/pid.c           | 8 ++++----
 kernel/pid_namespace.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index c4b26cd8998b..5aa050418fda 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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)))) {
 		retval = -ENOMEM;
 		goto bad_fork_core_free;
 	}
diff --git a/kernel/pid.c b/kernel/pid.c
index 4ac2ce46817f..df59228dd27e 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -122,7 +122,7 @@ void free_pid(struct pid *pid)
 	for (i = 0; i <= pid->level; i++) {
 		struct upid *upid = pid->numbers + i;
 		struct pid_namespace *ns = upid->ns;
-		switch (--ns->pid_allocated) {
+		switch (data_race(--ns->pid_allocated)) {
 		case 2:
 		case 1:
 			/* When all that is left in the pid namespace
@@ -134,7 +134,7 @@ void free_pid(struct pid *pid)
 		case PIDNS_ADDING:
 			/* Handle a fork failure of the first process */
 			WARN_ON(ns->child_reaper);
-			ns->pid_allocated = 0;
+			data_race(ns->pid_allocated = 0);
 			break;
 		}
 
@@ -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)))
 		goto out_unlock;
 	pidfs_add_pid(pid);
 	for ( ; upid >= pid->numbers; --upid) {
 		/* Make the PID visible to find_pid_ns. */
 		idr_replace(&upid->ns->idr, pid, upid->nr);
-		upid->ns->pid_allocated++;
+		data_race(upid->ns->pid_allocated++);
 	}
 	spin_unlock(&pidmap_lock);
 	idr_preload_end();
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 7098ed44e717..6e2a32641904 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -268,7 +268,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	 */
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
-		if (pid_ns->pid_allocated == init_pids)
+		if (data_race(pid_ns->pid_allocated == init_pids))
 			break;
 		schedule();
 	}
-- 
2.47.1

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Fri, Apr 25, 2025 at 01:58:14PM +0800, Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> Suppress syzbot reports by annotating these accesses using data_race().

Thanks for trying this approach.
scripts/checkpatch.pl has quite some remarks about the current form :-)
I mean, the data_race annotation should document why the race is
harmless. I only glanced over this so I can't tell myself whether it's a
bug or OK.

> Reported-by: syzbot+adcaa842b762a1762e7d@syzkaller.appspotmail.com
> Reported-by: syzbot+fab52e3459fa2f95df57@syzkaller.appspotmail.com
> Reported-by: syzbot+0718f65353d72efaac1e@syzkaller.appspotmail.com

How can I get to see full syzbot reports? (Stacktrace would be helpful,
I cannot resolve those as message-ids on (LK)ML.)

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jiayuan Chen]

April 25, 2025 at 17:25, "Michal Koutný" <mkoutny@suse.com> wrote:

> 
> On Fri, Apr 25, 2025 at 01:58:14PM +0800, Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
> 
> > 
> > Suppress syzbot reports by annotating these accesses using data_race().
> > 
> 
> Thanks for trying this approach.
> 
> scripts/checkpatch.pl has quite some remarks about the current form :-)
> 
> I mean, the data_race annotation should document why the race is
> 
> harmless. I only glanced over this so I can't tell myself whether it's a
> 
> bug or OK.
> 
> > 
> > Reported-by: syzbot+adcaa842b762a1762e7d@syzkaller.appspotmail.com
> > 
> >  Reported-by: syzbot+fab52e3459fa2f95df57@syzkaller.appspotmail.com
> > 
> >  Reported-by: syzbot+0718f65353d72efaac1e@syzkaller.appspotmail.com
> > 
> 
> How can I get to see full syzbot reports? (Stacktrace would be helpful,
> 
> I cannot resolve those as message-ids on (LK)ML.)
> 
> Thanks,
> 
> Michal
>

Hi Michal,

We can find such reports at https://syzkaller.appspot.com/upstream/s/kernel

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

[Oleg Nesterov]

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.