Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 19 Oct 2020 19:20:24 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r366848 - head/sys/kern
Message-ID:  <202010191920.09JJKOva029898@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kib
Date: Mon Oct 19 19:20:23 2020
New Revision: 366848
URL: https://svnweb.freebsd.org/changeset/base/366848

Log:
  vgonel(): avoid recursing into VOP_INACTIVE().
  
  It is a common pattern for filesystems' VOP_INACTIVE() implementation
  to forcibly reclaim the vnode when its state is final.  For instance,
  UFS vnode with zero link count is removed, and since it is
  inactivated, the last open reference on it is dropped.
  
  On the other hand, vnode might get spurious usecount reference for
  many reasons.  If the spurious reference exists while vgonel() checks
  for active state of the vnode, it would recurse into VOP_INACTIVE().
  
  Fix it by checking and not doing inactivation when vgone() was called
  from inactive VOP.
  
  Reported and tested by:	pho
  Discussed with:	mjg
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week

Modified:
  head/sys/kern/vfs_subr.c

Modified: head/sys/kern/vfs_subr.c
==============================================================================
--- head/sys/kern/vfs_subr.c	Mon Oct 19 18:54:44 2020	(r366847)
+++ head/sys/kern/vfs_subr.c	Mon Oct 19 19:20:23 2020	(r366848)
@@ -1794,6 +1794,8 @@ freevnode(struct vnode *vp)
 	VNASSERT(vp->v_cache_dd == NULL, vp, ("vp has namecache for .."));
 	VNASSERT(TAILQ_EMPTY(&vp->v_rl.rl_waiters), vp,
 	    ("Dangling rangelock waiters"));
+	VNASSERT((vp->v_iflag & (VI_DOINGINACT | VI_OWEINACT)) == 0, vp,
+	    ("Leaked inactivation"));
 	VI_UNLOCK(vp);
 #ifdef MAC
 	mac_vnode_destroy(vp);
@@ -3803,7 +3805,7 @@ vgonel(struct vnode *vp)
 	struct thread *td;
 	struct mount *mp;
 	vm_object_t object;
-	bool active, oweinact;
+	bool active, doinginact, oweinact;
 
 	ASSERT_VOP_ELOCKED(vp, "vgonel");
 	ASSERT_VI_LOCKED(vp, "vgonel");
@@ -3825,11 +3827,17 @@ vgonel(struct vnode *vp)
 	vp->v_irflag |= VIRF_DOOMED;
 
 	/*
-	 * Check to see if the vnode is in use.  If so, we have to call
-	 * VOP_CLOSE() and VOP_INACTIVE().
+	 * Check to see if the vnode is in use.  If so, we have to
+	 * call VOP_CLOSE() and VOP_INACTIVE().
+	 *
+	 * It could be that VOP_INACTIVE() requested reclamation, in
+	 * which case we should avoid recursion, so check
+	 * VI_DOINGINACT.  This is not precise but good enough.
 	 */
 	active = vp->v_usecount > 0;
 	oweinact = (vp->v_iflag & VI_OWEINACT) != 0;
+	doinginact = (vp->v_iflag & VI_DOINGINACT) != 0;
+
 	/*
 	 * If we need to do inactive VI_OWEINACT will be set.
 	 */
@@ -3850,7 +3858,7 @@ vgonel(struct vnode *vp)
 	 */
 	if (active)
 		VOP_CLOSE(vp, FNONBLOCK, NOCRED, td);
-	if (oweinact || active) {
+	if ((oweinact || active) && !doinginact) {
 		VI_LOCK(vp);
 		vinactivef(vp);
 		VI_UNLOCK(vp);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202010191920.09JJKOva029898>