NetBSD Problem Report #28448

From yamt@mwd.biglobe.ne.jp  Sun Nov 28 23:39:55 2004
Return-Path: <yamt@mwd.biglobe.ne.jp>
Received: from yamt.dyndns.org (FLA1Aaf058.kng.mesh.ad.jp [61.193.99.58])
	by narn.netbsd.org (Postfix) with ESMTP id CD47A251EB8
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 28 Nov 2004 23:39:54 +0000 (UTC)
Message-Id: <1101685174.330885.8884.nullmailer@yamt.dyndns.org>
Date: Mon, 29 Nov 2004 08:39:34 +0900
From: yamt@mwd.biglobe.ne.jp
Reply-To: yamt@mwd.biglobe.ne.jp
To: gnats-bugs@netbsd.org
Subject: stackable filesystems locking breakage when looking up DOTDOT
X-Send-Pr-Version: 3.95

>Number:         28448
>Category:       kern
>Synopsis:       stackable filesystems locking breakage when looking up DOTDOT
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    dholland
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 28 23:42:00 +0000 2004
>Closed-Date:    Mon Jun 21 02:58:08 +0000 2021
>Last-Modified:  Mon Jun 21 02:58:08 +0000 2021
>Originator:     YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
>Release:        NetBSD 2.99.10
>Organization:

>Environment:


System: NetBSD kaeru 2.99.10 NetBSD 2.99.10 (build.kaeru) #1898: Sun Nov 28 20:43:05 JST 2004 takashi@kaeru:/usr/home/takashi/work/kernel/build.kaeru i386
Architecture: i386
Machine: i386
>Description:
	layer_lookup calls underlying filesystem's VOP_LOOKUP with
	dvp locked.  while underlying filesystem might perform relock dance
	for it's vnodes, it doesn't know about upper vnodes.
	thus it will deadlock in the case of DOTDOT.
>How-To-Repeat:
	code inspection.
>Fix:

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->feedback
State-Changed-By: pooka@narn.netbsd.org
State-Changed-When: Sat, 19 Jan 2008 18:35:38 +0200
State-Changed-Why:
I don't understand the problem.  There are two cases:

1) the lower layer exports a lock and there is no upper lock
2) the lower layer does not export a lock pointer, in which case the
   relock dance in the lower layer will not use the upper layer's
   locks

Either way I don't see a deadlock between the lower and upper layer.


Responsible-Changed-From-To: kern-bug-people->dholland
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Mon, 27 Oct 2008 00:13:18 +0000
Responsible-Changed-Why:
Lookup issues are mine... but I'm inclined to agree with pooka - I don't see
that there's a problem. (Other than the general problem that lookup is gross.)


From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org, gnats-admin@netbsd.org,
        pooka@NetBSD.org
Subject: Re: kern/28448 (stackable filesystems locking breakage when looking
 up DOTDOT)
Date: Sat,  3 Jan 2009 13:11:59 +0900 (JST)

 hi,

 > Synopsis: stackable filesystems locking breakage when looking up DOTDOT
 > 
 > State-Changed-From-To: open->feedback
 > State-Changed-By: pooka@narn.netbsd.org
 > State-Changed-When: Sat, 19 Jan 2008 18:35:38 +0200
 > State-Changed-Why:
 > I don't understand the problem.  There are two cases:
 > 
 > 1) the lower layer exports a lock and there is no upper lock
 > 2) the lower layer does not export a lock pointer, in which case the
 >    relock dance in the lower layer will not use the upper layer's
 >    locks
 > 
 > Either way I don't see a deadlock between the lower and upper layer.

 this PR was about 2).
 it can deadlock between the parent and child directories
 in the upper layer due to the lack of dotdot relock dances in
 the upper layer.  the relock dance in the lower layer doesn't
 make sense because it's done keeping the upper layer vnode locked.

 YAMAMOTO Takashi

From: David Holland <dholland-bugs@netbsd.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org,
	netbsd-bugs@netbsd.org, gnats-admin@netbsd.org, pooka@NetBSD.org
Subject: Re: kern/28448 (stackable filesystems locking breakage when
	looking up DOTDOT)
Date: Sat, 3 Jan 2009 04:37:27 +0000

 On Sat, Jan 03, 2009 at 01:11:59PM +0900, YAMAMOTO Takashi wrote:
  > > 2) the lower layer does not export a lock pointer, in which case the
  > >    relock dance in the lower layer will not use the upper layer's
  > >    locks
  > 
  > this PR was about 2).
  > it can deadlock between the parent and child directories
  > in the upper layer due to the lack of dotdot relock dances in
  > the upper layer.  the relock dance in the lower layer doesn't
  > make sense because it's done keeping the upper layer vnode locked.

 So (to clarify as much as possible) the case is:

    a. locks are not shared between the upper and lower layers
    b. we try to lookup .. on an upper layer directory UD
    c. vfs locks the directory, which locks both UD and the lower-layer
       directory LD corresponding to UD
    d. vfs calls VOP_LOOKUP, which goes to layer_lookup, which goes to
       the lower fs
    e. the lookup op on the lower fs unlocks LD and locks LD's parent,
       then returns LD's parent
    f. layer_lookup gets LD's parent back and digs out the
       corresponding upper vnode with layer_node_create
    g. layer_node_create then locks vnode without first unlocking UD
    h. this violates the locking order, because UD's parent is locked
       after UD.

 Do we have any layered fses where both c. and d. happen? It seems to
 me, based on my perhaps too vague understanding of how the layering
 system works, that if you don't share locks you can't expect to share
 the fs namespace, meaning you can't expect to use the default
 layer_lookup. I suspect this is not the only problem that'll show up
 if someone tries.

 If that's supposed to be legal, then it's a problem, yes. Nice catch :-)

 (although note that I'm planning to abolish the dotdot-dance)

 -- 
 David A. Holland
 dholland@netbsd.org

From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: dholland-bugs@netbsd.org
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
        gnats-admin@netbsd.org, pooka@NetBSD.org
Subject: Re: kern/28448 (stackable filesystems locking breakage when
	looking up DOTDOT)
Date: Sat,  3 Jan 2009 14:32:13 +0900 (JST)

 > On Sat, Jan 03, 2009 at 01:11:59PM +0900, YAMAMOTO Takashi wrote:
 >  > > 2) the lower layer does not export a lock pointer, in which case the
 >  > >    relock dance in the lower layer will not use the upper layer's
 >  > >    locks
 >  > 
 >  > this PR was about 2).
 >  > it can deadlock between the parent and child directories
 >  > in the upper layer due to the lack of dotdot relock dances in
 >  > the upper layer.  the relock dance in the lower layer doesn't
 >  > make sense because it's done keeping the upper layer vnode locked.
 > 
 > So (to clarify as much as possible) the case is:
 > 
 >    a. locks are not shared between the upper and lower layers
 >    b. we try to lookup .. on an upper layer directory UD
 >    c. vfs locks the directory, which locks both UD and the lower-layer
 >       directory LD corresponding to UD
 >    d. vfs calls VOP_LOOKUP, which goes to layer_lookup, which goes to
 >       the lower fs
 >    e. the lookup op on the lower fs unlocks LD and locks LD's parent,
 >       then returns LD's parent
 >    f. layer_lookup gets LD's parent back and digs out the
 >       corresponding upper vnode with layer_node_create
 >    g. layer_node_create then locks vnode without first unlocking UD
 >    h. this violates the locking order, because UD's parent is locked
 >       after UD.
 > 
 > Do we have any layered fses where both c. and d. happen? It seems to
 > me, based on my perhaps too vague understanding of how the layering
 > system works, that if you don't share locks you can't expect to share
 > the fs namespace, meaning you can't expect to use the default
 > layer_lookup. I suspect this is not the only problem that'll show up
 > if someone tries.
 > 
 > If that's supposed to be legal, then it's a problem, yes. Nice catch :-)

 locks can be shared only when the lower vnode exports them
 for the upper layer.  ie. a layered filesystem can't assume that
 it can share locks.

 > 
 > (although note that I'm planning to abolish the dotdot-dance)

 good to hear.  thanks for working on it.
 how will you do so?

 YAMAMOTO Takashi

 > 
 > -- 
 > David A. Holland
 > dholland@netbsd.org

From: David Holland <dholland-bugs@netbsd.org>
To: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
Cc: dholland-bugs@netbsd.org, gnats-bugs@NetBSD.org,
	kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
	gnats-admin@netbsd.org, pooka@NetBSD.org
Subject: Re: kern/28448 (stackable filesystems locking breakage when
	looking up DOTDOT)
Date: Sun, 4 Jan 2009 01:18:26 +0000

 On Sat, Jan 03, 2009 at 02:32:13PM +0900, YAMAMOTO Takashi wrote:
  > locks can be shared only when the lower vnode exports them
  > for the upper layer.  ie. a layered filesystem can't assume that
  > it can share locks.

 That sounds like a problem... I'll keep it in mind.

  > > (although note that I'm planning to abolish the dotdot-dance)
  > 
  > good to hear.  thanks for working on it.
  > how will you do so?

 See http://mail-index.netbsd.org/tech-kern/2008/03/19/msg000653.html
 and following.

 (Which are turning out to be much longer-range plans than I'd
 intended... but that's another story.)

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: feedback->analyzed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 04 Jan 2009 01:26:53 +0000
State-Changed-Why:
Problem has been identified more clearly, and is definitely real.


From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/28448 (stackable filesystems locking breakage when looking
 up DOTDOT)
Date: Wed, 14 Oct 2015 20:17:24 +0000

 On Sun, Jan 04, 2009 at 01:26:54AM +0000, dholland@NetBSD.org wrote:
  > Problem has been identified more clearly, and is definitely real.

 ok, update:
   (1) this is fixed in -current and -7, because VOP_LOOKUP no longer
       locks the result before returning it.
   (2) candidate patch for -6; this needs testing. (it's passed an
       anita run but that isn't by itself real persuasive)

 Index: sys/miscfs/genfs/layer_vnops.c
 ===================================================================
 RCS file: /cvsroot/src/sys/miscfs/genfs/layer_vnops.c,v
 retrieving revision 1.50.8.1
 diff -u -p -r1.50.8.1 layer_vnops.c
 --- sys/miscfs/genfs/layer_vnops.c	18 Nov 2012 18:36:58 -0000	1.50.8.1
 +++ sys/miscfs/genfs/layer_vnops.c	14 Oct 2015 20:14:42 -0000
 @@ -382,7 +382,21 @@ layer_lookup(void *v)
  		vref(dvp);
  		*ap->a_vpp = dvp;
  		vrele(lvp);
 -	} else if (lvp != NULL) {
 +	} else if (lvp == NULL) {
 +		/* nothing */
 +	} else if (cnp->cn_flags & ISDOTDOT) {
 +		/* must unlock dvp temporarily to conform to lock ordering */
 +		struct mount *mp;
 +
 +		/* mp can't disappear because we still hold a ref on dvp */
 +		mp = dvp->v_mount;
 +		VOP_UNLOCK(dvp);
 +		error = layer_node_create(mp, lvp, ap->a_vpp);
 +		vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY);
 +		if (error) {
 +			vrele(lvp);
 +		}
 +	} else {
  		/* Note: dvp, ldvp and lvp are all locked. */
  		error = layer_node_create(dvp->v_mount, lvp, ap->a_vpp);
  		if (error) {

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: analyzed->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 21 Jun 2021 02:58:08 +0000
State-Changed-Why:
fixed in -7, dangling patches for -6 are no longer remotely relevant


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.