Re: [PATCH] pid: Add a judgment for ns null in pid_nr_ns

[Oleg Nesterov <oleg@redhat.com>]

On 09/01, Mateusz Guzik wrote:
>
> On Mon, Sep 1, 2025 at 5:44 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > On Mon, Sep 1, 2025 at 5:32 PM Oleg Nesterov <oleg@redhat.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.