Re: sysctl_doeproc() race

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Sun, 11 Mar 2018 16:02:57 +0100
    From:        Kamil Rytarowski <n54%gmx.com@localhost>
    Message-ID:  <10b0efe2-35bb-d1e8-ed28-587f643bdfd7%gmx.com@localhost>

  | POSIX people told me that polling of a process entity is reliable only
  | for parent (whether a real one or a tracer in non-POSIX extensions).

I agree with that - what's more, that is all it reasonably can be.

  | We can use pid lookup in this particular case calling proc_find_raw().

Yes, there's an XXX comment in the code already, but I agree:

  | I treat this as just an optimization (something nice to have, but not
  | important now).

An alternative would be to simply break out of the loop after having
found the process in the case of  (op == KERN_PROC_PID)

  | This will not solve the bug for other sysctl_doeproc() cases and a fix
  | is still needed.

Is it really?    Those cases won't have the issue you saw of there
being insufficient buffer space (or the code will handle that already)
as the number of processes that match could be anything, the buffer
will not (should not) have space for just one (unlike the KERN_PROC_PID
case where only 1 should be possible).

It would also (IMO) be OK to simply document that if a process happens
to become a zombie while collecting the data to satisfy the sysctl, then
data for it might be returned twice, once before, and once after.   Then
leave it up to the user code to handle that as it likes.

  | We already use markers so I prefer to stick to this solution and it
  | makes this code reliable.

I don't much like the way you LIST_REMOVE(marker0) - if we add almost
any fix to short circuit the complete list scan after having found the proc
entry for the KERN_PROC_PID case your current method will fail.

Better to simply remove it after the loop ends (no matter why it ends),
rather than when it is used (and then also do it in the cleanup code
always, rather than only when !mmmbrains).   That's simpler, and easier
to understand.

  | For the completeness I'm going to test the approach proposed by Christos
  | whether it is stable, and I plan to commit the patch of mine as a
  | personal preference.

Actually, now I have looked at what the code is doing (refreshed my memory)
I don't think that would solve anything - it helps only to make sure we do not
find a process twice - but as long as it exists (and other than the KERN_PROC_PID
case) I think that is better than not including the process at all.

There are other ways to avoid the "find twice" issue for KERN_PROC_PID
(including your marker0 - though that is now embedding the assumption
that new zombies must always be pushed onto the head of the zombproc
list, which while the way it works (and the easy way) I do not recall being
mandated previously (we could also append them.)

The other issues you mentioned that are other possible problems, I agree
with Martin, simply ignore them - if they cause problems it is because the
user code is making assumptions that the interface is not expected to provide.

kre