On Tue, Jun 05, 2018 at 03:57:48PM -0600, Todd C. Miller wrote:
> On Tue, 05 Jun 2018 16:30:14 -0500, Scott Cheloha wrote:
>
> > Because the system clock can jump around, process start times
> > should be recorded as offsets from boot and only converted to
> > a time of day on request.
> >
> > This keeps ps(1)'s etime accurate and ensures processes are
> > correctly sorted by age in ps(1) and pgrep(1).
>
> Looks good.
>
> > I don't think we're losing much by declining to preserve the
> > time of day when the process forked.
> >
> > FreeBSD has made this change, though NetBSD has not.
> >
> > The attached patch isn't perfect, as there remains a small window
> > after kvm_getprocs is called where the system clock can jump again.
> > To circumvent that we'd need to compute the elapsed real time for
> > the process in the kernel, or pass out ps_start alongside the derived
> > time of day and let userspace compute the elapsed realtime with
> > the CLOCK_BOOTTIME clock. Unsure how people feel about extending
> > kinfo_proc, though.
>
> I really don't this is worth worrying about. In the current state
> of things, if the system clock jumps the process start time as
> reported by ps will be incorrect anyway. This seems like an
> improvement.
>
> > This change doesn't require any userspace recompilation for inspecting
> > a live kernel: on reboot -lkvm binaries behave as expected.
>
> Great. I was worried that consumers of p_ustart_{sec,usec} would
> need to change but I can see that is not the case.
Update: here is a completed patch. This now adds bits to libkvm that
derive the process starting time in the coredump case. It doesn't take
subseconds into account: for that we'd need to expose the timehands
definition to userspace. So I left a comment marking the way forward
for the intrepid.
For the coredump case to work obviously libkvm itself will need to be
recompiled, as will statically linked binaries like ps(1), otherwise
you'll get zeroes for your process starting times and ps(1) will claim
all the processes on your cored system started in ~1970. The interface
to libkvm is entirely unchanged, so I'm pretty sure this doesn't
require a version bump.
ps(1)'s etime column will still be incorrect in the coredump case because
it computes the elapsed time using the active kernel's time. There is a
similar problem with the uptime line in w(1). The attached patch itself
doesn't break these outputs, though, so I'll fix them in a separate patch.
The only port using p_ustart_{sec,usec} in a way that might be broken by
this change is sudo. I talked to todd and he said that this change wouldn't
cause any problems, though.
ok?
--
Scott Cheloha
Index: lib/libkvm/kvm_proc2.c
===
RCS file: /cvs/src/lib/libkvm/kvm_proc2.c,v
retrieving revision 1.27
diff -u -p -r1.27 kvm_proc2.c
--- lib/libkvm/kvm_proc2.c 7 Nov 2016 00:26:33 - 1.27
+++ lib/libkvm/kvm_proc2.c 23 Jun 2018 00:12:14 -
@@ -119,13 +119,35 @@ kvm_proclist(kvm_t *kd, int op, int arg,
struct sigacts sa, *sap;
struct vmspace vm, *vmp;
struct plimit limits, *limp;
+ struct nlist nl[3];
pid_t parent_pid, leader_pid;
- int cnt = 0;
- int dothreads = 0;
+ time_t time_second, time_uptime;
+ int cnt, dothreads, i;
+ cnt = 0;
dothreads = op & KERN_PROC_SHOW_THREADS;
op &= ~KERN_PROC_SHOW_THREADS;
+ /* Determine the victim's age and time of death. */
+ memset(&nl, 0, sizeof(nl));
+ nl[0].n_name = "time_second";
+ nl[1].n_name = "time_uptime";
+ nl[2].n_name = NULL;
+ if (kvm_nlist(kd, nl) != 0) {
+ for (i = 0; nl[i].n_type != 0; i++)
+ continue;
+ _kvm_err(kd, kd->program, "%s: no such symbol", nl[i].n_name);
+ return (-1);
+ }
+ if (KREAD(kd, nl[0].n_value, &time_second)) {
+ _kvm_err(kd, kd->program, "can't read %s", nl[0].n_name);
+ return (-1);
+ }
+ if (KREAD(kd, nl[1].n_value, &time_uptime)) {
+ _kvm_err(kd, kd->program, "can't read %s", nl[1].n_name);
+ return (-1);
+ }
+
/*
* Modelled on sysctl_doproc() in sys/kern/kern_sysctl.c
*/
@@ -288,6 +310,19 @@ kvm_proclist(kvm_t *kd, int op, int arg,
kp.p_tpgid = -1;
kp.p_tdev = NODEV;
}
+
+ /*
+* Derive the starting time for the process.
+*
+* XXX This ignores subseconds. One solution would be
+* to expose the timehands definition to userspace and
+* grab th_offset/th_nanotime from the active timehands.
+* Pending that, just pretend every process forked at
+* the beginning of a nearby second.
+*/
+ kp.p_ustart
