msdosfs not MPSAFE

[Kostik Belousov]

On Wed, Jul 11, 2007 at 12:08:19AM +1000, Bruce Evans 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

It seems that msdosfs_lookup() can sleep, thus Giant protection would be
lost.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-bugs/attachments/20070712/21bf65cd/attachment.pgp