Re: kern/46997: ufs_extattr_uepm_lock() is broken

[David Holland <dholland-bugs%netbsd.org@localhost>]

The following reply was made to PR kern/46997; it has been noted by GNATS.

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/46997: ufs_extattr_uepm_lock() is broken
Date: Wed, 9 Nov 2016 05:31:53 +0000

 On Sun, Sep 23, 2012 at 03:05:01AM +0000, mrg%eterna.com.au@localhost wrote:
  > static void
  > ufs_extattr_uepm_lock(struct ufsmount *ump)
  > {
  > 
  >         /* XXX Why does this need to be recursive? */
  >         if (mutex_owned(&ump->um_extattr.uepm_lock)) {
  >                 ump->um_extattr.uepm_lockcnt++;
  >                 return;
  >         }
  >         mutex_enter(&ump->um_extattr.uepm_lock);
  > }
  > 
  > mutex_owned() is not usable this way.  it should never be used
  > for anything except asserting a mutex is held.  see mutex(9).
 
 here is an untested candidate patch that makes it into a workable
 recursive lock.
 
 (How does one test this? Do we have atf test coverage for the ffsv1
 extended attributes code?)
 
 diff -r 91e47d073544 sys/ufs/ufs/extattr.h
 --- a/sys/ufs/ufs/extattr.h	Wed Nov 09 00:12:44 2016 -0500
 +++ b/sys/ufs/ufs/extattr.h	Wed Nov 09 00:26:57 2016 -0500
 @@ -94,12 +94,21 @@ struct ufs_extattr_list_entry {
  
  #define	UELE_NEEDSWAP(uele)	((uele)->uele_flags & UELE_F_NEEDSWAP)
  
 +/*
 + * XXX: this should not need to exist. See code in ufs_extattr.c.
 + */
 +struct ufs_extattr_custom_lock {
 +	kmutex_t			uecl_innerlock;
 +	kcondvar_t			uecl_waithook;
 +	struct lwp *			uecl_locker;
 +	unsigned			uecl_lockcount;
 +};
 +
  struct lock;
  struct ufs_extattr_per_mount {
 -	kmutex_t			uepm_lock;
 +	struct ufs_extattr_custom_lock	uepm_lock;
  	struct ufs_extattr_list_head	uepm_list;
  	kauth_cred_t			uepm_ucred;
 -	int				uepm_lockcnt;
  	int				uepm_flags;
  };
  
 diff -r 91e47d073544 sys/ufs/ufs/ufs_extattr.c
 --- a/sys/ufs/ufs/ufs_extattr.c	Wed Nov 09 00:12:44 2016 -0500
 +++ b/sys/ufs/ufs/ufs_extattr.c	Wed Nov 09 00:26:57 2016 -0500
 @@ -160,6 +160,78 @@ internal_extattr_check_cred(vnode_t *vp,
  }
  
  /*
 + * Custom recursive lock because this file's locking model is crap.
 + *
 + * A recursive lock is needed for the following reasons:
 + *   - it is taken in ufs_extattr_vnode_inactive
 + *   - which is called from VOP_INACTIVE
 + *   - which can be triggered by any vrele, vput, or vn_close
 + *   - several of these can happen while it's held
 + *
 + * XXX: this should be removed.
 + */
 +
 +static void
 +uecl_init(struct ufs_extattr_custom_lock *uecl)
 +{
 +	mutex_init(&uecl->uecl_innerlock, MUTEX_DEFAULT, IPL_NONE);
 +	cv_init(&uecl->uecl_waithook, "ufsextattr");
 +	uecl->uecl_locker = NULL;
 +	uecl->uecl_lockcount = 0;
 +}
 +
 +static void
 +uecl_cleanup(struct ufs_extattr_custom_lock *uecl)
 +{
 +	KASSERT(uecl->uecl_locker == NULL);
 +	KASSERT(uecl->uecl_lockcount == 0);
 +
 +	mutex_destroy(&uecl->uecl_innerlock);
 +	cv_destroy(&uecl->uecl_waithook);
 +}
 +
 +static void
 +uecl_enter(struct ufs_extattr_custom_lock *uecl)
 +{
 +	/*int error;*/
 +
 +	mutex_enter(&uecl->uecl_innerlock);
 +	if (uecl->uecl_locker == curlwp) {
 +		uecl->uecl_lockcount++;
 +	} else {
 +		while (uecl->uecl_locker != NULL) {
 +			KASSERT(uecl->uecl_locker != curlwp);
 +			/*error =*/ cv_wait/*_sig*/(&uecl->uecl_waithook,
 +					    &uecl->uecl_innerlock);
 +			/*if (error) {
 +				mutex_exit(&uecl->uecl_innerlock);
 +				return error;
 +			}*/
 +		}
 +		KASSERT(uecl->uecl_lockcount == 0);
 +		uecl->uecl_locker = curlwp;
 +		uecl->uecl_lockcount = 1;
 +	}
 +	mutex_exit(&uecl->uecl_innerlock);
 +}
 +
 +static void
 +uecl_exit(struct ufs_extattr_custom_lock *uecl)
 +{
 +	mutex_enter(&uecl->uecl_innerlock);
 +
 +	KASSERT(uecl->uecl_locker == curlwp);
 +	KASSERT(uecl->uecl_lockcount > 0);
 +
 +	uecl->uecl_lockcount--;
 +	if (uecl->uecl_lockcount == 0) {
 +		uecl->uecl_locker = NULL;
 +		cv_signal(&uecl->uecl_waithook /*, &uecl->uecl_innerlock*/);
 +	}
 +	mutex_exit(&uecl->uecl_innerlock);
 +}
 +
 +/*
   * Per-FS attribute lock protecting attribute operations.
   * XXX Right now there is a lot of lock contention due to having a single
   * lock per-FS; really, this should be far more fine-grained.
 @@ -167,31 +239,13 @@ internal_extattr_check_cred(vnode_t *vp,
  static void
  ufs_extattr_uepm_lock(struct ufsmount *ump)
  {
 -
 -	/*
 -	 * XXX This needs to be recursive for the following reasons:
 -	 *   - it is taken in ufs_extattr_vnode_inactive
 -	 *   - which is called from VOP_INACTIVE
 -	 *   - which can be triggered by any vrele, vput, or vn_close
 -	 *   - several of these can happen while it's held
 -	 */
 -	if (mutex_owned(&ump->um_extattr.uepm_lock)) {
 -		ump->um_extattr.uepm_lockcnt++;
 -		return;
 -	}
 -	mutex_enter(&ump->um_extattr.uepm_lock);
 +	uecl_enter(&ump->um_extattr.uepm_lock);
  }
  
  static void
  ufs_extattr_uepm_unlock(struct ufsmount *ump)
  {
 -
 -	if (ump->um_extattr.uepm_lockcnt != 0) {
 -		KASSERT(mutex_owned(&ump->um_extattr.uepm_lock));
 -		ump->um_extattr.uepm_lockcnt--;
 -		return;
 -	}
 -	mutex_exit(&ump->um_extattr.uepm_lock);
 +	uecl_exit(&ump->um_extattr.uepm_lock);
  }
  
  /*-
 @@ -390,10 +444,9 @@ ufs_extattr_uepm_init(struct ufs_extattr
  {
  
  	uepm->uepm_flags = 0;
 -	uepm->uepm_lockcnt = 0;
  
  	LIST_INIT(&uepm->uepm_list);
 -	mutex_init(&uepm->uepm_lock, MUTEX_DEFAULT, IPL_NONE);
 +	uecl_init(&uepm->uepm_lock);
  	uepm->uepm_flags |= UFS_EXTATTR_UEPM_INITIALIZED;
  }
  
 @@ -418,7 +471,7 @@ ufs_extattr_uepm_destroy(struct ufs_exta
  	 */
  	uepm->uepm_flags &= ~UFS_EXTATTR_UEPM_STARTED;
  	uepm->uepm_flags &= ~UFS_EXTATTR_UEPM_INITIALIZED;
 -	mutex_destroy(&uepm->uepm_lock);
 +	uecl_cleanup(&uepm->uepm_lock);
  }
  
  /*
 
 
 -- 
 David A. Holland
 dholland%netbsd.org@localhost