svn commit: r267760 - head/sys/kern

[Mateusz Guzik]

On Sun, Jul 13, 2014 at 04:26:52PM +0300, Konstantin Belousov wrote:
> On Fri, Jul 11, 2014 at 01:19:25PM +0200, Mateusz Guzik wrote:
> > On Fri, Jul 11, 2014 at 12:55:51PM +0300, Konstantin Belousov wrote:
> > > The nolock version requires two atomics on both entry and leave from the
> > > protected region, while sx-locked variant requires only one atomic for
> > > entry and leave.
> > > 
> > > I am not sure why you decided to acquire p->p_keeplock in after the
> > > proc lock in pget(), which indeed causes the complications of dropping
> > > the proc_lock and rechecking to avoid LOR.  Did you tried to add a flag
> > > to pfind*() functions to indicate that p_keeplock should be acquired,
> > > instead ?
> > 
> > Lock is taken later to avoid waiting for finished exec/exit of processes
> > we cannot return, so that e.g. procstat -fa does not trip over that
> > much.
> > 
> > Right now only PROC_LOCK guarantees stability of p->p_ucred across pget
> > operation. Without that the code would have to crget() and various
> > functions modified to accept cred instead of proc, or 'imagelock'
> > mechanism would have to be extended to also protect against cred
> > changes.
> No, you could get both locks, imagelock first, proc_lock next.
> 

Ignoring allproc_lock:

sx lock case:
filedesc out: slock + proc lock + proc unlock + sunlock
exit/exec: xlock + xunlock

counter case:
filedesc out: proc lock + proc unlock + proc lock + proc unlock
exit/exec: just wait for imagelock to be 0

counter can result in temporary errors due to catching the process
in exec, on the other hand slock before proc lock forces the caller to
wait even for processes it cannot read

I find the counter case better.

sx:
http://people.freebsd.org/~mjg/patches/sx-imagelock.patch

counter: 
http://people.freebsd.org/~mjg/patches/counter-imagelock.patch

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
proctree -> allproc
allproc -> imagelock
imagelock -> devfs

-- 
Mateusz Guzik <mjguzik gmail.com>