Re: [PATCH] Make udf(4) MPSAFE and use shared lookups

On Fri, Nov 21, 2008 at 02:52:02PM -0500, John Baldwin wrote:
> On Thursday 20 November 2008 06:32:25 pm Paul B. Mahol wrote:
> > On 11/20/08, John Baldwin <jhb_at_freebsd.org> wrote:
> > > So this patch is fairly minimal since udf(4) is currently read-only.  The
> > > changes include:
> > >
> > > * Set VV_ROOT in udf_vget() if we ever return a vnode instead of doing it
> > > only
> > >   in udf_root().  This matches the behavior of other operating systems and
> > >   correctly tags the root vnode with VV_ROOT in the unlikely case that we
> > >   create the vnode during a call to ufs_vget() that does not come from
> > >   ufs_root().
> > > * If the hash lookup in ufs_vget() fails, ensure an exclusive vnode lock 
> is
> > >   used while creating the new vnode (same as UFS).
> > > * Allow lock recursion (XXX: not really sure this is needed actually).
> > > * Allow shared vnode locks on non-fifos.
> > > * Honor the requested locking flags (shared vs exclusive) instead of 
> always
> > >   using exclusive vnode locks during a lookup operation.
> > > * Handle "." lookups the same way other filesystems do by just bumping the
> > >   reference on 'dvp' rather than calling udf_vget().
> > >
> > > http://www.FreeBSD.org/~jhb/patches/udf_mpsafe.patch
> > >
> > > I have only verified that this compiles and would appreciate it if some
> > > folks
> > > could test this, preferable with INVARIANTS and DEBUG_VFS_LOCKS enabled.
> > 
> > I lightly tested it with md(4) on 47M size iso created with k3b
> > (it contains two files)
> > 
> > I only got this lor related to udf:
> > 
> > lock order reversal:
> >  1st 0xc4449270 udf (udf) _at_ /usr/src/sys/kern/vfs_lookup.c:442
> >  2nd 0xd7d10850 bufwait (bufwait) _at_ /usr/src/sys/kern/vfs_bio.c:2443
> >  3rd 0xc4399488 udf (udf) _at_
> > /usr/src/sys/modules/udf/../../fs/udf/udf_vfsops.c:625
> 
> I've updated the patch at the same URL above to fix this LOR.

udf_vget() does insmntque() before vnode is fully initialized, allowing
other threads to find the vnode on the mount list. This is typical for
!MPSAFE fs, and it seems corresponding call was not marked XXX for udf.

udf_lookup for ISDOTDOT case unlocks dvp before vget'ing "..", allowing
the same race on forced unmount as ufs (I will finally commit ufs patch
today). The race happens for !MPSAFE code too, but it is easier to
execute without Giant.