[systemd-devel] [RFC PATCH] proc, pidns: Add highpid

[Andy Lutomirski]

On Nov 30, 2014 9:45 AM, "David Herrmann" <dh.herrmann at gmail.com> wrote:
>
> Hi Andy
>
> On Sat, Nov 29, 2014 at 12:05 AM, Andy Lutomirski <luto at amacapital.net> wrote:
> > Pid reuse is common, which means that it's difficult or impossible
> > to read information about a pid from /proc without races.
> >
> > This introduces a second number associated with each (task, pidns)
> > pair called highpid.  Highpid is a 64-bit number, and, barring
> > extremely unlikely circumstances or outright error, a (highpid, pid)
> > will never be reused.
> >
> > With just this change, a program can open /proc/PID/status, read the
> > "Highpid" field, and confirm that it has the expected value.  If the
> > pid has been reused, then highpid will be different.
> >
> > The initial implementation is straightforward: highpid is simply a
> > 64-bit counter. If a high-end system can fork every 3 ns (which
> > would be amazing, given that just allocating a pid requires at
> > atomic operation), it would take well over 1000 years for highpid to
> > wrap.
> >
> > For CRIU's benefit, the next highpid can be set by a privileged
> > user.
> >
> > NB: The sysctl stuff only works on 64-bit systems.  If the approach
> > looks good, I'll fix that somehow.
> >
> > Signed-off-by: Andy Lutomirski <luto at amacapital.net>
> > ---
> >
> > If this goes in, there's plenty of room to add new interfaces to
> > make this more useful.  For example, we could add a fancier tgkill
> > that adds and validates hightgid and highpid, and we might want to
> > add a syscall to read one's own hightgid and highpid.  These would
> > be quite useful for pidfiles.
> >
> > David, would this be useful for kdbus?
>
> Much appreciated! This would serve well as replacement for
> 'starttime'. I'd prefer if pid_t was 64bit, but I guess that ship
> sailed long ago. Though, your patch might in the end just introduce a
> new pid64, which replaces the old pid and lives in parallel.
>
> Anyway, considering that we actually want the same pid-reuse
> protection for tid, tgid, ppid and so on, we'd have to add a
> 'starttime' for all of them. Sounds ugly.. so we might just end up
> dropping 'starttime' and introduce KDBUS_ITEM_PIDS2 whenever a 'fix'
> is merged upstream.

I don't understand "we want".  Doesn't kdbus currently only think
about tid and tgid?

In any event, I just looked at the kdbus code
(https://github.com/gregkh/kdbus/blob/master/metadata.c), and I see:

meta->pid = get_pid(task_pid(current));
meta->tgid = get_pid(task_tgid(current));
meta->starttime = current->start_time,

I don't understand how that concept of starttime is particularly
useful.  Isn't that the start time associated with tid?  How can that
be useful when looking up tgid?

Regardless, my patch here associates a highpid with each struct pid,
and both tid and tgid are just struct pids, so you get both.  I agree
that adding Highppid to /proc might be useful, but that seems like a
future extension that has nothing to do with kdbus.

--Andy