Should EXIT_DEAD be visible to userspace ?

Should EXIT_DEAD be visible to userspace ?

[Dave Jones]

One of our users reported a crash in some userspace tooling this
morning, which scrapes /proc/pid to gather stack traces, process states
etc of everything running at the time.

The crash occurred because it fell over an unexpected task state,
which was 'X'.   According to the procps man-pages, this state should
never be seen, but here it clearly was.

The kernel running at the time was kinda old (5.2) but I don't see much
change in the EXIT_DEAD space that would explain something that got
fixed subsequently.   It's also probably going to be difficult to
reproduce unfortunately.

So my question is, is procps wrong and code should expect to see X state
processes in proc ?  The code in question is being hardened to handle
unexpected inputs, but I'm curious if the kernel is leaking some state
that it shouldn't.

	Dave

Re: Should EXIT_DEAD be visible to userspace ?

[Linus Torvalds]

On Mon, Oct 11, 2021 at 12:40 PM Dave Jones <davej@codemonkey.org.uk> wrote:
>
> One of our users reported a crash in some userspace tooling this
> morning, which scrapes /proc/pid to gather stack traces, process states
> etc of everything running at the time.
>
> The crash occurred because it fell over an unexpected task state,
> which was 'X'.   According to the procps man-pages, this state should
> never be seen, but here it clearly was.

Heh.

> The kernel running at the time was kinda old (5.2) but I don't see much
> change in the EXIT_DEAD space that would explain something that got
> fixed subsequently.   It's also probably going to be difficult to
> reproduce unfortunately.
>
> So my question is, is procps wrong and code should expect to see X state
> processes in proc ?  The code in question is being hardened to handle
> unexpected inputs, but I'm curious if the kernel is leaking some state
> that it shouldn't.

My gut feel is that the man-pages have clearly been wrong - or at
least misleading - for at least the last couple of years (and possibly
longer), and this is the first report we've ever had of it actually
causing problems.

The docs *do* mention 'X'. Even if they say 'should never be seen',
it's not like it's not right there.

So we could either ask to just have the man-pages fixed to be a little
less strongly worded ("never" -> "seldom" or whatever). And apparently
procps is already getting fixed.

Or we could hide the 'X' state in newer kernels, and just call them
zombies to user space. We could literally just change the string from
"X (dead)" to "Z (dead)" and the "dead" part would still be there (and
different from "Z (zombie)").

And either way, it's likely not going to be something that people will
notice ever again. You update your system, and you wouldn't see the
problem, because whether the kernel was changed or not, procps was
updated.

And if the argument is that people didn't update procps, but *did*
update the kernel, then sure, that could avoid somebody hitting this
again, but that's where the "at least a couple of years and nobody has
noticed before" comes in.

So I can certainly take a patch that hides 'X', and we can even mark
it for stable.

But it feels like realistically nobody will actually care, and the
real fix is the one to procps, and that fix will make any kernel
change irrelevant (and possibly even a slight negative, since now
procps might report interesting cases?).

End result: if somebody cares enough and sends me a tested patch, I'll
apply it. But I personally wouldn't care much, and wouldn't push for
it unless we get somebody who does.

And I really think that "should never be seen" in the docs is just wrong.

Yes, we hold the task lock in task_state() when you look at
/proc/<pid>/status. So maybe it will never be seen there, because
maybe (I didn't check) we move from X->Z while holding the lock.

But other parts of /proc don't actually do that lock_task(), I think.
IOW, looking at /proc/<pid>/stat (which shows just the first letter of
the state), doesn't do that, I think. So it's not actually serialized
with the process going through its dying moments.

So I _think_ the "never" was always just "almost never".

In fact, take a look at commit ad86622b478e ("wait: swap EXIT_ZOMBIE
and EXIT_DEAD to hide EXIT_TRACE from user-space"). That 'X' has been
seen before. It's not great when it happens, but I think this is an
example of "the 'never' has never been true, and goes back to at least
2014".

.. and that 2014 was just when we happened on it before. The actual
issue of 'X' showing up looks like it probably predates it (and likely
goes back to before git history).

So I suspect that stray 'X' is not actually a regression. Just rare
enough to be _almost_ "never".

                     Linus

Re: Should EXIT_DEAD be visible to userspace ?

[Christian Brauner]

On Mon, Oct 11, 2021 at 01:33:21PM -0700, Linus Torvalds wrote:
> On Mon, Oct 11, 2021 at 12:40 PM Dave Jones <davej@codemonkey.org.uk> wrote:
> >
> > One of our users reported a crash in some userspace tooling this
> > morning, which scrapes /proc/pid to gather stack traces, process states
> > etc of everything running at the time.
> >
> > The crash occurred because it fell over an unexpected task state,
> > which was 'X'.   According to the procps man-pages, this state should
> > never be seen, but here it clearly was.
> 
> Heh.
> 
> > The kernel running at the time was kinda old (5.2) but I don't see much
> > change in the EXIT_DEAD space that would explain something that got
> > fixed subsequently.   It's also probably going to be difficult to
> > reproduce unfortunately.
> >
> > So my question is, is procps wrong and code should expect to see X state
> > processes in proc ?  The code in question is being hardened to handle
> > unexpected inputs, but I'm curious if the kernel is leaking some state
> > that it shouldn't.
> 
> My gut feel is that the man-pages have clearly been wrong - or at
> least misleading - for at least the last couple of years (and possibly
> longer), and this is the first report we've ever had of it actually
> causing problems.
> 
> The docs *do* mention 'X'. Even if they say 'should never be seen',
> it's not like it's not right there.
> 
> So we could either ask to just have the man-pages fixed to be a little
> less strongly worded ("never" -> "seldom" or whatever). And apparently
> procps is already getting fixed.
> 
> Or we could hide the 'X' state in newer kernels, and just call them
> zombies to user space. We could literally just change the string from
> "X (dead)" to "Z (dead)" and the "dead" part would still be there (and
> different from "Z (zombie)").
> 
> And either way, it's likely not going to be something that people will
> notice ever again. You update your system, and you wouldn't see the
> problem, because whether the kernel was changed or not, procps was
> updated.
> 
> And if the argument is that people didn't update procps, but *did*
> update the kernel, then sure, that could avoid somebody hitting this
> again, but that's where the "at least a couple of years and nobody has
> noticed before" comes in.
> 
> So I can certainly take a patch that hides 'X', and we can even mark
> it for stable.
> 
> But it feels like realistically nobody will actually care, and the
> real fix is the one to procps, and that fix will make any kernel
> change irrelevant (and possibly even a slight negative, since now
> procps might report interesting cases?).
> 
> End result: if somebody cares enough and sends me a tested patch, I'll
> apply it. But I personally wouldn't care much, and wouldn't push for
> it unless we get somebody who does.
> 
> And I really think that "should never be seen" in the docs is just wrong.
> 
> Yes, we hold the task lock in task_state() when you look at
> /proc/<pid>/status. So maybe it will never be seen there, because
> maybe (I didn't check) we move from X->Z while holding the lock.
> 
> But other parts of /proc don't actually do that lock_task(), I think.
> IOW, looking at /proc/<pid>/stat (which shows just the first letter of
> the state), doesn't do that, I think. So it's not actually serialized
> with the process going through its dying moments.
> 
> So I _think_ the "never" was always just "almost never".
> 
> In fact, take a look at commit ad86622b478e ("wait: swap EXIT_ZOMBIE
> and EXIT_DEAD to hide EXIT_TRACE from user-space"). That 'X' has been
> seen before. It's not great when it happens, but I think this is an
> example of "the 'never' has never been true, and goes back to at least
> 2014".
> 
> .. and that 2014 was just when we happened on it before. The actual
> issue of 'X' showing up looks like it probably predates it (and likely
> goes back to before git history).
> 
> So I suspect that stray 'X' is not actually a regression. Just rare
> enough to be _almost_ "never".

Imho, let's
1. update the manpage
2. update procps
and call it done.

Christian