Re: [RFC] Get siginfo from unreaped task

From: "Robert Święcki" <robert@swiecki.net>
To: Kees Cook <keescook@chromium.org>
Cc: Christian Brauner <brauner@kernel.org>,
	Andy Lutomirski <luto@amacapital.net>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Jann Horn <jannh@google.com>, Will Drewry <wad@chromium.org>,
	linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [RFC] Get siginfo from unreaped task
Date: Mon, 14 Feb 2022 23:08:53 +0100	[thread overview]
Message-ID: <CAP145piExdhYLaKcHaCOoXVageMqmB6eDV-iyzeNc+RgVc=B-Q@mail.gmail.com> (raw)
In-Reply-To: <202202141152.6296CD7F@keescook>

pon., 14 lut 2022 o 21:07 Kees Cook <keescook@chromium.org> napisał(a):

> > > I had a kind of opposite thought, which is that it would be very nice
> > > to be able to get all the waitid() data without reaping a process or
> > > even necessarily being its parent.  Maybe these can be combined?  A
> > > new waitid() option like you’re suggesting could add siginfo (and
> > > might need permissions).  And we could have a different waitid() flag
> > > that says “maybe not my child, don’t reap” (and also needs
> > > permissions).
> > >
> > > Although the “don’t reap” thing is fundamentally racy. What a sane
> > > process manager actually wants is an interface to read all this info
> > > from a pidfd, which means it all needs to get stuck in struct pid. And
> >
> > /me briefly pops out from vacation
> >
> > Agreed and not just siginfo I would expect(?). We already came to that
> > conclusion when we first introduced them.
> >
> > > task_struct needs a completion or wait queue so you can actually wait
> > > for a pidfd to exit (unless someone already did this — I had patches a
> > > while back).  And this would be awesome.
> >
> > Currently, you can wait for a pidfd to exit via polling and you can use
> > a pidfd to pass it to waitid(P_PIDFD, pidfd, ...).
> >
> > /me pops back into vacation
>
> Right, so waitid already has all the infrastructure for this, so I think
> adding it there makes a lot of sense. Here's what I've got:
>
> One usability question I have is:
>
> - if the process just exited normally, should it return an empty
>   siginfo, or should it ignore __WCHILDSIGINFO? (I have it ignoring it
>   above.)

Maybe ENODATA as return code, in order to make it obvious to the
caller that the siginfo is missing? In the end if somebody requests
it, they probably should have already checked that the child process
was killed by a signal, by using WNOWAIT with waitid() or wait4().

Additionally, there might be a problem with input params. The waitid()
can take P_ALL or P_PGID as arguments. If waitid() returns with 0, the
pid of the child process generating the event can be read from
si->si_pid. But siginfo_t is a union of multiple different structs,
many of them missing si_pid. So, such a flag (__WCHILDSIGINFO) would
make sense with P_PID and P_PIDFD only, maybe this should be
explicitly checked for?