Date: Mon, 6 Jul 2015 12:09:58 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20150706090958.GM2080@kib.kiev.ua> In-Reply-To: <20150706064601.GB28898@dft-labs.eu> References: <20140623131653.GC27040@dft-labs.eu> <20140623163523.GK93733@kib.kiev.ua> <20140711024351.GA18214@dft-labs.eu> <20140711095551.GA93733@kib.kiev.ua> <20140711111925.GB18214@dft-labs.eu> <20140713132652.GZ93733@kib.kiev.ua> <20140713213623.GA13241@dft-labs.eu> <20140717005638.GF93733@kib.kiev.ua> <20150706055135.GA28898@dft-labs.eu> <20150706064601.GB28898@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 06, 2015 at 08:46:02AM +0200, Mateusz Guzik wrote:
> On Mon, Jul 06, 2015 at 07:51:35AM +0200, Mateusz Guzik wrote:
> > On Thu, Jul 17, 2014 at 03:56:38AM +0300, Konstantin Belousov wrote:
> > > On Sun, Jul 13, 2014 at 11:36:24PM +0200, Mateusz Guzik wrote:
> > > > There is an additional problem with slocked case: witness report a lor
> > > > with devfs vnodes.
> > > >
> > > > lock order reversal:
> > > > 1st 0xfffff80006fe6ab0 process imagelock (process imagelock) @ /usr/src/sys/kern/kern_proc.c:287
> > > > 2nd 0xfffff80018c88240 devfs (devfs) @ /usr/src/sys/kern/vfs_cache.c:1241
> > > > KDB: stack backtrace:
> > > > db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe012324f120
> > > > kdb_backtrace() at kdb_backtrace+0x39/frame 0xfffffe012324f1d0
> > > > witness_checkorder() at witness_checkorder+0xdc2/frame 0xfffffe012324f260
> > > > __lockmgr_args() at __lockmgr_args+0x588/frame 0xfffffe012324f3a0
> > > > vop_stdlock() at vop_stdlock+0x3c/frame 0xfffffe012324f3c0
> > > > VOP_LOCK1_APV() at VOP_LOCK1_APV+0xfc/frame 0xfffffe012324f3f0
> > > > _vn_lock() at _vn_lock+0xaa/frame 0xfffffe012324f460
> > > > vn_vptocnp_locked() at vn_vptocnp_locked+0xe8/frame 0xfffffe012324f4d0
> > > > vn_fullpath1() at vn_fullpath1+0xb0/frame 0xfffffe012324f530
> > > > vn_fullpath() at vn_fullpath+0xc1/frame 0xfffffe012324f580
> > > > export_fd_to_sb() at export_fd_to_sb+0x489/frame 0xfffffe012324f7b0
> > > > kern_proc_filedesc_out() at kern_proc_filedesc_out+0x1c6/frame 0xfffffe012324f840
> > > > sysctl_kern_proc_filedesc() at sysctl_kern_proc_filedesc+0x84/frame 0xfffffe012324f900
> > > > sysctl_root_handler_locked() at sysctl_root_handler_locked+0x68/frame 0xfffffe012324f940
> > > > sysctl_root() at sysctl_root+0x18e/frame 0xfffffe012324f990
> > > > userland_sysctl() at userland_sysctl+0x192/frame 0xfffffe012324fa30
> > > >
> > > > witness detected the following lock orders:
> > > > devfs -> proctree
> > > Where is the dependency catched comes from ? I suspect it might be tty.
> > >
> > > I consider this case as an advantage of using sx over the hand-rolled lock,
> > > since the issue is/must be present with the counter as well, or the LOR
> > > is false positive, possibly due to sx taken in shared mode. But debugging
> > > features of sx give the warning, while counter is silent.
> > >
> > > That said, if the issue above is analyzed, I do not have any preference
> > > and will not object strongly against you decision.
> > >
> >
> > [snip]
> >
> > This patch is about providing a way to block execs and exits of
> > processes so that it is safe to inspect their state. For instance it
> > deals with a case where the target process executes a setuid binary,
> > where the process inspecting it could have its permission checked prior
> > to exec but is reading data after setuid completed.
> >
> > LOR mentioned above indeed comes form tty handling and is handled by
> > relocking the vnode.
> >
>
> Oops, the attached patch was missing one place (devfs_lookupx). Updated patch:
>
>
> diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
> index fe8e9ef..c7f579a 100644
> --- a/sys/fs/devfs/devfs_vnops.c
> +++ b/sys/fs/devfs/devfs_vnops.c
> @@ -574,6 +574,8 @@ devfs_close(struct vop_close_args *ap)
> if (vp->v_data == NULL)
> return (0);
>
> + vp_locked = VOP_ISLOCKED(vp);
> +
> /*
> * Hack: a tty device that is a controlling terminal
> * has a reference from the session structure.
> @@ -589,6 +591,7 @@ devfs_close(struct vop_close_args *ap)
> if (vp == p->p_session->s_ttyvp) {
> PROC_UNLOCK(p);
> oldvp = NULL;
> + VOP_UNLOCK(vp, 0);
This opens a window where vp can be reclaimed. Then vp->v_rdev
is invalid and dev_refthread() below accesses random memory.
Might be, the easiest fix is to call dev_refthread() before the td !=
NULL block, but leave dsw == NULL check where it is now.
> sx_xlock(&proctree_lock);
> if (vp == p->p_session->s_ttyvp) {
> SESS_LOCK(p->p_session);
> @@ -603,6 +606,7 @@ devfs_close(struct vop_close_args *ap)
> SESS_UNLOCK(p->p_session);
> }
> sx_xunlock(&proctree_lock);
> + vn_lock(vp, vp_locked | LK_RETRY);
> if (oldvp != NULL)
> vrele(oldvp);
> } else
> @@ -632,7 +636,6 @@ devfs_close(struct vop_close_args *ap)
> }
> vholdl(vp);
> VI_UNLOCK(vp);
> - vp_locked = VOP_ISLOCKED(vp);
> VOP_UNLOCK(vp, 0);
> KASSERT(dev->si_refcount > 0,
> ("devfs_close() on un-referenced struct cdev *(%s)", devtoname(dev)));
> @@ -964,10 +967,13 @@ devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock)
> cdev = NULL;
> DEVFS_DMP_HOLD(dmp);
> sx_xunlock(&dmp->dm_lock);
> + dvplocked = VOP_ISLOCKED(dvp);
> + VOP_UNLOCK(dvp, 0);
> sx_slock(&clone_drain_lock);
> EVENTHANDLER_INVOKE(dev_clone,
> td->td_ucred, pname, strlen(pname), &cdev);
> sx_sunlock(&clone_drain_lock);
> + vn_lock(dvp, dvplocked | LK_RETRY);
>
> if (cdev == NULL)
> sx_xlock(&dmp->dm_lock);
Again, dvp might be reclaimed while unlocked, then devfs_populate_vp()
dereferences dvp->v_mount == NULL.
> diff --git a/sys/fs/nfsclient/nfs_clport.c b/sys/fs/nfsclient/nfs_clport.c
> index d3bac30..118a7d9 100644
> --- a/sys/fs/nfsclient/nfs_clport.c
> +++ b/sys/fs/nfsclient/nfs_clport.c
> @@ -1188,7 +1188,7 @@ nfscl_procdoesntexist(u_int8_t *own)
> tl.cval[2] = *own++;
> tl.cval[3] = *own++;
> pid = tl.lval;
> - p = pfind_locked(pid);
> + p = pfind_locked(pid, 0);
> if (p == NULL)
> return (1);
> if (p->p_stats == NULL) {
> diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
> index 0675128..2ad09aa 100644
> --- a/sys/kern/imgact_elf.c
> +++ b/sys/kern/imgact_elf.c
> @@ -1887,6 +1887,7 @@ note_procstat_files(void *arg, struct sbuf *sb, size_t *sizep)
> sb = sbuf_new(NULL, NULL, 128, SBUF_FIXEDLEN);
> sbuf_set_drain(sb, sbuf_drain_count, &size);
> sbuf_bcat(sb, &structsize, sizeof(structsize));
> + sx_slock(&p->p_imagelock);
> PROC_LOCK(p);
> kern_proc_filedesc_out(p, sb, -1);
> sbuf_finish(sb);
> @@ -1895,6 +1896,7 @@ note_procstat_files(void *arg, struct sbuf *sb, size_t *sizep)
> } else {
> structsize = sizeof(struct kinfo_file);
> sbuf_bcat(sb, &structsize, sizeof(structsize));
> + sx_slock(&p->p_imagelock);
> PROC_LOCK(p);
> kern_proc_filedesc_out(p, sb, -1);
> }
> diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
> index 37539c4..9364dcb 100644
> --- a/sys/kern/init_main.c
> +++ b/sys/kern/init_main.c
> @@ -480,6 +480,7 @@ proc0_init(void *dummy __unused)
> p->p_flag2 = 0;
> p->p_state = PRS_NORMAL;
> knlist_init_mtx(&p->p_klist, &p->p_mtx);
> + sx_init_flags(&p->p_imagelock, "process imagelock", SX_NEW);
> STAILQ_INIT(&p->p_ktr);
> p->p_nice = NZERO;
> /* pid_max cannot be greater than PID_MAX */
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> index 85cda01..bbdcbf5 100644
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -3271,6 +3271,7 @@ kern_proc_filedesc_out(struct proc *p, struct sbuf *sb, ssize_t maxlen)
> FILEDESC_SUNLOCK(fdp);
> fddrop(fdp);
> fail:
> + sx_sunlock(&p->p_imagelock);
IMO it would be less confusing to unlock the p_imagelock at the caller'
location. At the kern_proc_filedesc_out(), the p_imagelock slocked state
must be asserted.
Same for ofiledesc().
> free(efbuf, M_TEMP);
> return (error);
> }
> @@ -3292,7 +3293,7 @@ sysctl_kern_proc_filedesc(SYSCTL_HANDLER_ARGS)
>
> sbuf_new_for_sysctl(&sb, NULL, FILEDESC_SBUF_SIZE, req);
> sbuf_clear_flags(&sb, SBUF_INCLUDENUL);
> - error = pget((pid_t)name[0], PGET_CANDEBUG | PGET_NOTWEXIT, &p);
> + error = pget((pid_t)name[0], PGET_LOCK, &p);
> if (error != 0) {
> sbuf_delete(&sb);
> return (error);
> @@ -3359,7 +3360,7 @@ sysctl_kern_proc_ofiledesc(SYSCTL_HANDLER_ARGS)
> struct proc *p;
>
> name = (int *)arg1;
> - error = pget((pid_t)name[0], PGET_CANDEBUG | PGET_NOTWEXIT, &p);
> + error = pget((pid_t)name[0], PGET_LOCK, &p);
> if (error != 0)
> return (error);
> fdp = fdhold(p);
> @@ -3391,6 +3392,7 @@ sysctl_kern_proc_ofiledesc(SYSCTL_HANDLER_ARGS)
> }
> FILEDESC_SUNLOCK(fdp);
> fddrop(fdp);
> + sx_sunlock(&p->p_imagelock);
> free(kif, M_TEMP);
> free(okif, M_TEMP);
> return (0);
> diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
> index 859b2e3..0d9aac1 100644
> --- a/sys/kern/kern_exec.c
> +++ b/sys/kern/kern_exec.c
> @@ -381,6 +381,7 @@ do_execve(td, args, mac_p)
> * that might allow a local user to illicitly obtain elevated
> * privileges.
> */
> + sx_xlock(&p->p_imagelock);
> PROC_LOCK(p);
> KASSERT((p->p_flag & P_INEXEC) == 0,
> ("%s(): process already has P_INEXEC flag", __func__));
> @@ -907,6 +908,7 @@ exec_fail:
> SDT_PROBE(proc, kernel, , exec__failure, error, 0, 0, 0, 0);
>
> done2:
> + sx_xunlock(&p->p_imagelock);
> #ifdef MAC
> mac_execve_exit(imgp);
> mac_execve_interpreter_exit(interpvplabel);
> diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
> index 9ce6d34..27fd764 100644
> --- a/sys/kern/kern_exit.c
> +++ b/sys/kern/kern_exit.c
> @@ -213,6 +213,7 @@ exit1(struct thread *td, int rv)
> /*
> * MUST abort all other threads before proceeding past here.
> */
> + sx_xlock(&p->p_imagelock);
> PROC_LOCK(p);
> /*
> * First check if some other thread or external request got
> @@ -279,6 +280,7 @@ exit1(struct thread *td, int rv)
> * decided to wait again after we told them we are exiting.
> */
> p->p_flag |= P_WEXIT;
> + sx_xunlock(&p->p_imagelock);
I do not understand why p_imagelock is released so early in the exit1().
> wakeup(&p->p_stype);
>
> /*
> diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
> index 27c6f40..ac6b441 100644
> --- a/sys/kern/kern_proc.c
> +++ b/sys/kern/kern_proc.c
> @@ -230,6 +230,7 @@ proc_init(void *mem, int size, int flags)
> mtx_init(&p->p_statmtx, "pstatl", NULL, MTX_SPIN | MTX_NEW);
> mtx_init(&p->p_itimmtx, "pitiml", NULL, MTX_SPIN | MTX_NEW);
> mtx_init(&p->p_profmtx, "pprofl", NULL, MTX_SPIN | MTX_NEW);
> + sx_init_flags(&p->p_imagelock, "process imagelock", SX_NEW);
> cv_init(&p->p_pwait, "ppwait");
> cv_init(&p->p_dbgwait, "dbgwait");
> TAILQ_INIT(&p->p_threads); /* all threads in proc */
> @@ -278,16 +279,20 @@ inferior(struct proc *p)
> }
>
> struct proc *
> -pfind_locked(pid_t pid)
> +pfind_locked(pid_t pid, int lockimage)
> {
> struct proc *p;
>
> sx_assert(&allproc_lock, SX_LOCKED);
> LIST_FOREACH(p, PIDHASH(pid), p_hash) {
> if (p->p_pid == pid) {
> + if (lockimage)
> + sx_slock(&p->p_imagelock);
Probably makes sense to explicitely add imagelock name into the
witness' order_lists[] array.
> PROC_LOCK(p);
> if (p->p_state == PRS_NEW) {
> PROC_UNLOCK(p);
> + if (lockimage)
> + sx_sunlock(&p->p_imagelock);
> p = NULL;
> }
> break;
> @@ -308,7 +313,7 @@ pfind(pid_t pid)
> struct proc *p;
>
> sx_slock(&allproc_lock);
> - p = pfind_locked(pid);
> + p = pfind_locked(pid, 0);
> sx_sunlock(&allproc_lock);
> return (p);
> }
> @@ -364,11 +369,17 @@ int
> pget(pid_t pid, int flags, struct proc **pp)
> {
> struct proc *p;
> - int error;
> + int error, lockimage;
> +
> + lockimage = ((flags & PGET_IMAGELOCK) != 0);
> + if (lockimage) {
> + MPASS((flags & PGET_NOTWEXIT) != 0);
> + MPASS((flags & PGET_HOLD) == 0);
> + }
>
> sx_slock(&allproc_lock);
> if (pid <= PID_MAX) {
> - p = pfind_locked(pid);
> + p = pfind_locked(pid, lockimage);
> if (p == NULL && (flags & PGET_NOTWEXIT) == 0)
> p = zpfind_locked(pid);
> } else if ((flags & PGET_NOTID) == 0) {
> @@ -413,6 +424,8 @@ pget(pid_t pid, int flags, struct proc **pp)
> return (0);
> errout:
> PROC_UNLOCK(p);
> + if (lockimage)
> + sx_sunlock(&p->p_imagelock);
> return (error);
> }
>
> diff --git a/sys/sys/proc.h b/sys/sys/proc.h
> index e6c83b4..5df817a 100644
> --- a/sys/sys/proc.h
> +++ b/sys/sys/proc.h
> @@ -522,6 +522,7 @@ struct proc {
> (if I am reaper). */
> LIST_ENTRY(proc) p_reapsibling; /* (e) List of siblings - descendants of
> the same reaper. */
> + struct sx p_imagelock; /* Lock blocking exec/exit. */
> struct mtx p_mtx; /* (n) Lock for this struct. */
> struct mtx p_statmtx; /* Lock for the stats */
> struct mtx p_itimmtx; /* Lock for the virt/prof timers */
> @@ -886,7 +887,7 @@ extern struct proc *initproc, *pageproc; /* Process slots for init, pager. */
> extern struct uma_zone *proc_zone;
>
> struct proc *pfind(pid_t); /* Find process by id. */
> -struct proc *pfind_locked(pid_t pid);
> +struct proc *pfind_locked(pid_t pid, int lockimage);
> struct pgrp *pgfind(pid_t); /* Find process group by id. */
> struct proc *zpfind(pid_t); /* Find zombie process by id. */
>
> @@ -900,8 +901,10 @@ struct proc *zpfind(pid_t); /* Find zombie process by id. */
> #define PGET_NOTWEXIT 0x00010 /* Check that the process is not in P_WEXIT. */
> #define PGET_NOTINEXEC 0x00020 /* Check that the process is not in P_INEXEC. */
> #define PGET_NOTID 0x00040 /* Do not assume tid if pid > PID_MAX. */
> +#define PGET_IMAGELOCK 0x00080 /* Prevent execs and exits. */
>
> #define PGET_WANTREAD (PGET_HOLD | PGET_CANDEBUG | PGET_NOTWEXIT)
> +#define PGET_LOCK (PGET_CANDEBUG | PGET_NOTWEXIT | PGET_IMAGELOCK)
>
> int pget(pid_t pid, int flags, struct proc **pp);
>
> --
> Mateusz Guzik
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150706090958.GM2080>