msdosfs not MPSAFE

[Brian Chu]

Bruce,

I've already planned to look at this.  I had not been aware that calls
to the Giant Lock were being ignored in the current kernel.

Brian

On 7/10/07, Bruce Evans <brde at optusnet.com.au> wrote:
> msdsofs has been broken since Giant locking for file systems (or syscalls)
> was removed.  It allows multiple threads to race accessing the shared
> static buffer `nambuf' and related variables.  This causes remarkably
> few problems.  One case that breaks reliably is concurrent tars or finds,
> especially with a small cluster size, even on a UP system.  Then
> getdirentries() returns garbage entries and/or lookup() of correct entries
> can't find things.  On my UP system, I think the race occurs mainly when
> getdirentries() blocks on i/o and a directory entry spans the block
> boundary.  Then another getdirentries() or lookup() can run, and if it
> does then it will almost certainly corrupt the state for the blocked
> getdirentries().  On SMP systems, I think the race occurs much more often
> and suspect that it is more harmful.
>
> Quick fix for an old version of FreeBSD. The part in msdosfs_lookup.c
> has not been tested at runtime.  The part in msdosfs_vnops.c may also
> fix bugs involving cookie handling (but not a memory leak like I first
> thought?).  msdosfs_lookup.c is harder to fix because it has no common
> cleanup code.
>
> %%%
> Index: msdosfs_lookup.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_lookup.c,v
> retrieving revision 1.40
> diff -u -2 -r1.40 msdosfs_lookup.c
> --- msdosfs_lookup.c    26 Dec 2003 17:24:37 -0000      1.40
> +++ msdosfs_lookup.c    10 Jul 2007 13:33:37 -0000
> @@ -78,11 +78,11 @@
>    * memory denode's will be in synch.
>    */
> -int
> -msdosfs_lookup(ap)
> +static int
> +msdosfs_lookup_locked(
>         struct vop_cachedlookup_args /* {
>                 struct vnode *a_dvp;
>                 struct vnode **a_vpp;
>                 struct componentname *a_cnp;
> -       } */ *ap;
> +       } */ *ap)
>   {
>         struct vnode *vdp = ap->a_dvp;
> @@ -560,4 +561,20 @@
>
>   /*
> + * XXX msdosfs_lookup() is split up because unlocking Giant before all the
> + * returns in the original function would be too churning.
> + */
> +int
> +msdosfs_lookup(ap)
> +       struct vop_cachedlookup_args *ap;
> +{
> +       int error;
> +
> +       mtx_lock(&Giant);       /* XXX for exclusive access to nambuf. */
> +       error = msdosfs_lookup_locked(ap);
> +       mtx_unlock(&Giant);
> +       return (error);
> +}
> +
> +/*
>    * dep  - directory entry to copy into the directory
>    * ddep - directory to add to
> Index: msdosfs_vnops.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_vnops.c,v
> retrieving revision 1.147
> diff -u -2 -r1.147 msdosfs_vnops.c
> --- msdosfs_vnops.c     4 Feb 2004 21:52:53 -0000       1.147
> +++ msdosfs_vnops.c     10 Jul 2007 05:08:17 -0000
> @@ -1559,4 +1592,5 @@
>         }
>
> +       mtx_lock(&Giant);       /* XXX for exclusive access to nambuf. */
>         mbnambuf_init();
>         off = offset;
> @@ -1575,10 +1609,13 @@
>                 if (error) {
>                         brelse(bp);
> -                       return (error);
> +                       Debugger("used to leak cookies 1");
> +                       break;
>                 }
>                 n = min(n, blsize - bp->b_resid);
>                 if (n == 0) {
>                         brelse(bp);
> -                       return (EIO);
> +                       error = EIO;
> +                       Debugger("used to leak cookies 2");
> +                       break;
>                 }
>
> @@ -1687,4 +1725,6 @@
>         }
>   out:
> +       mtx_unlock(&Giant);
> +
>         /* Subtract unused cookies */
>         if (ap->a_ncookies)
> %%%
>
> Please fix this better.  mbnambuf_init() could return a non-static buffer
> that doesn't require locking.  Deallocation would be painful.  Note that
> even the details for Giant locking can't be hidden in msdosfs_conv.c like
> the current interfaces intend, since mbnambuf_init() is used a lot to
> reinitialize an in-use buffer, and there is no interface to drop usage.
>
> Bruce
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
>