> Date: Wed, 14 Oct 2020 20:14:18 -0500
> From: Scott Cheloha <[email protected]>
>
> On Wed, Oct 14, 2020 at 08:06:52PM -0500, Scott Cheloha wrote:
> > _exit(2) and execve(2) need to obey the locking protocol described in
> > proc.h when manipulating the per-process interval timer state.
> >
> > While we're here we can also remove the now pointless splclock/splx
> > dance from execve(2).
> >
> > The easiest way to obey the locking protocol is to reuse the interface
> > the syscalls are using: setitimer() in kern_time.c.
> >
> > Given that we only want to cancel the timers I wrote a small helper
> > function, cancelitimer(). I think it's tidier than putting the
> > prototype for setitimer() into sys/time.h and requiring the caller to
> > prepare an itimerval struct before calling.
> >
> > Compare:
> >
> > struct itimerval itv;
> > timerclear(&itv.it_value);
> > timerclear(&itv.it_interval);
> > setitimer(ITIMER_REAL, &itv, NULL);
> >
> > with:
> >
> > cancelitimer(ITIMER_REAL);
> >
> > ... should I shove the for-loop into the helper function too? Maybe
> > call it "cancel_all_itimers()"? I have a vague feeling that showing
> > the reader that there are multiple timers is a good thing here, but
> > then again maybe I'm wrong and nobody cares.
> >
> > Preferences? ok?
>
> Whoops, forgot the kern_time.c part of the diff.
>
> Index: kern/kern_exit.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> retrieving revision 1.188
> diff -u -p -r1.188 kern_exit.c
> --- kern/kern_exit.c 18 Mar 2020 15:48:21 -0000 1.188
> +++ kern/kern_exit.c 15 Oct 2020 01:12:50 -0000
> @@ -194,7 +194,11 @@ exit1(struct proc *p, int xexit, int xsi
> /* close open files and release open-file table */
> fdfree(p);
>
> - timeout_del(&pr->ps_realit_to);
> + /* cancel all interval timers */
> + int i;
Don't put variable definitions in the middle of a function.
> + for (i = 0; i < nitems(pr->ps_timer); i++)
> + cancelitimer(i);
> +
> timeout_del(&pr->ps_rucheck_to);
> #ifdef SYSVSEM
> semexit(pr);
> Index: kern/kern_exec.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exec.c,v
> retrieving revision 1.217
> diff -u -p -r1.217 kern_exec.c
> --- kern/kern_exec.c 11 Jul 2020 22:59:05 -0000 1.217
> +++ kern/kern_exec.c 15 Oct 2020 01:12:50 -0000
> @@ -656,14 +656,9 @@ sys_execve(struct proc *p, void *v, regi
> }
>
> if (pr->ps_flags & PS_SUGIDEXEC) {
> - int i, s = splclock();
> -
> - timeout_del(&pr->ps_realit_to);
> - for (i = 0; i < nitems(pr->ps_timer); i++) {
> - timespecclear(&pr->ps_timer[i].it_interval);
> - timespecclear(&pr->ps_timer[i].it_value);
> - }
> - splx(s);
> + int i;
You should put a blank line here to separate definitions from the code.
> + for (i = 0; i < nitems(pr->ps_timer); i++)
> + cancelitimer(i);
> }
>
> /* reset CPU time usage for the thread, but not the process */
> Index: kern/kern_time.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.146
> diff -u -p -r1.146 kern_time.c
> --- kern/kern_time.c 13 Oct 2020 17:33:39 -0000 1.146
> +++ kern/kern_time.c 15 Oct 2020 01:12:50 -0000
> @@ -572,6 +572,16 @@ setitimer(int which, const struct itimer
> }
> }
>
> +void
> +cancelitimer(int which)
> +{
> + struct itimerval itv;
> +
> + timerclear(&itv.it_value);
> + timerclear(&itv.it_interval);
> + setitimer(which, &itv, NULL);
> +}
> +
> int
> sys_getitimer(struct proc *p, void *v, register_t *retval)
> {
> Index: sys/time.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/time.h,v
> retrieving revision 1.55
> diff -u -p -r1.55 time.h
> --- sys/time.h 6 Jul 2020 13:33:09 -0000 1.55
> +++ sys/time.h 15 Oct 2020 01:12:50 -0000
> @@ -307,6 +307,7 @@ time_t getuptime(void);
> struct proc;
> int clock_gettime(struct proc *, clockid_t, struct timespec *);
>
> +void cancelitimer(int);
> int itimerfix(struct timeval *);
> int itimerdecr(struct itimerspec *, long);
> int settime(const struct timespec *);
>
>
