NetBSD Problem Report #46997

From mrg@eterna.com.au  Sun Sep 23 03:00:55 2012
Return-Path: <mrg@eterna.com.au>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 62C4363E033
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 23 Sep 2012 03:00:55 +0000 (UTC)
Message-Id: <20120923030052.9F8C682200@splode.eterna.com.au>
Date: Sun, 23 Sep 2012 13:00:52 +1000 (EST)
From: mrg@eterna.com.au
Reply-To: mrg@eterna.com.au
To: gnats-bugs@gnats.NetBSD.org
Subject: ufs_extattr_uepm_lock() is broken
X-Send-Pr-Version: 3.95

>Number:         46997
>Category:       kern
>Synopsis:       ufs_extattr_uepm_lock() is broken
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Sep 23 03:05:00 +0000 2012
>Last-Modified:  Wed Nov 09 05:45:00 +0000 2016
>Originator:     matthew green
>Release:        NetBSD -current 2012-09-21
>Organization:
people's front against (bozotic) www (softwar foundation)
>Environment:
>Description:

/*
 * 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.
 */
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).

>How-To-Repeat:

	code-inspection.

>Fix:

	this code should be rewritten to use mutex_tryenter().

>Audit-Trail:
From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46997 CVS commit: src/sys/ufs/ufs
Date: Wed, 9 Nov 2016 05:08:35 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Wed Nov  9 05:08:35 UTC 2016

 Modified Files:
 	src/sys/ufs/ufs: ufs_extattr.c

 Log Message:
 Explain why the lock in here needs to be recursive. Related to PR 46997.


 To generate a diff of this commit:
 cvs rdiff -u -r1.47 -r1.48 src/sys/ufs/ufs/ufs_extattr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
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 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

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46997 CVS commit: src/sys/ufs/lfs
Date: Wed, 9 Nov 2016 05:44:42 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Wed Nov  9 05:44:42 UTC 2016

 Modified Files:
 	src/sys/ufs/lfs: ulfs_extattr.c

 Log Message:
 Apply ufs_extattr.c 1.48:
 Explain why the lock in here needs to be recursive. Related to PR 46997.

 ufs_extattr 1.47 was also committed directly here, so this file is still
 fully synced with it.


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.14 src/sys/ufs/lfs/ulfs_extattr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.