linux-kernel - Re: [PATCH] pid: Add a judgment for ns null in pid_nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250902143716.GB23520@redhat.com>
Date: Tue, 2 Sep 2025 16:37:17 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: Christian Brauner <brauner@...nel.org>,
	Xiang Gao <gxxa03070307@...il.com>, joel.granados@...nel.org,
	lorenzo.stoakes@...cle.com, linux-kernel@...r.kernel.org,
	gaoxiang17 <gaoxiang17@...omi.com>, Liam.Howlett@...cle.com,
	viro@...iv.linux.org.uk
Subject: Re: [PATCH] pid: Add a judgment for ns null in pid_nr_ns

On 09/01, Mateusz Guzik wrote:
>
> On Mon, Sep 1, 2025 at 5:44 PM Mateusz Guzik <mjguzik@...il.com> wrote:
> >
> > On Mon, Sep 1, 2025 at 5:32 PM Oleg Nesterov <oleg@...hat.com> wrote:
> > >
> > > ping...
> > >
> > > We need either
> > >
> > >   [1/1] pid: Add a judgment for ns null in pid_nr_ns
> > >   https://git.kernel.org/vfs/vfs/c/006568ab4c5c
> > >
> > > or
> > >
> > >   [1/4] pid: make __task_pid_nr_ns(ns => NULL) safe for zombie callers
> > >   https://git.kernel.org/vfs/vfs/c/abdfd4948e45
> > >
> > > in any case imo the changelog should explain why do we care
> > > to check ns != NUll, "Sometimes null is returned for task_active_pid_ns"
> > > doesn't look like a good explanation...
> > >
> >
> > Since I caught this a stray patchset I'll bite: given the totally
> > arbitrary task struct in an irq handler, why even allow querying it
> > from that level? The task is literally random, and even possibly dead
> > as in this crash report.

I won't really argue. And initially I was going to "ignore" the original
bug report. If nothing else, the code which triggered the crash was buggy.
But then I changed my mind. People will do mistakes, I think it would be
better to make this API a bit safer. See below.

But in any case, at least one of the patches above should be removed from
vfs-6.18.pidfs.

OTOH, the trivial/cosmetic

	[PATCH 2/4] pid: introduce task_ppid_vnr()
	https://lore.kernel.org/all/20250810173610.GA19995@redhat.com/

makes sense imo, and it was missed.

Now. I mostly agree about IRQ, but to me it would be better to avoid the
crash if, say, task_pid_vnr(&init_task) is called from IRQ context.

But lets forget about IRQ. Please look at the changelog in
[PATCH 1/4] pid: make __task_pid_nr_ns(ns => NULL) safe for zombie callers

Even task_ppid_nr_ns(current, NULL) is not safe if it is called by the
exiting task after exit_notify() in the process context, and this is not
immediately clear. This doesn't look good to me.

> Maybe even go a little further and assert that the task at hand is
> fully constructed and not exiting yet.

So what do you suggest? Add the current->exit_state check somewhere?

Oleg.