Fix fid double free crash

Virtfs_vget_common() should add virtfs_node to virtfs_session only when
the vnode is verified otherwise it makes the error handler has a chance
to free fid which should be avoided since callers of virtfs_vget_common
will also free fid if callee returned errors. Also,
virtfs_reload_stats_dotl(no vput), insmntque(doing vput) and
vfs_hash_insert(some cases doing vput) have different ways to release
vnode. They need to be handled separately in the error handler.
In addition, the error handler should dispose virtfs_node first then
do vput vnode to avoid free id here. Besides, if *vpp != NULL, it means
vp2 from the hash table is retrieved and the latest vp isn't inserted
to the hash table. It's not what we want. It should go to the error
handler. The last, reorder the flag VIRTFS_NODE_IN_SESSION check in the
beginning of virtfs_cleanup to make sure each virtfs_node in the session
only clean up fid , vp , and etc once.

Change-Id: I48b772498fee6785895aaccd990baf057aee3c70

sys/dev/virtio/9pfs/virtfs_vfops.c

@@ -211,6 +211,9 @@ virtfs_node_cmp(struct vnode *vp, void *arg)
 	np = vp->v_data;
 	qid = (struct p9_qid *)arg;
 
+	if(np == NULL)
+		return 1;
+
 	if (np->vqid.qid_path == qid->path) {
 		if (vp->v_vflag & VV_ROOT)
 			return 0;
@@ -251,6 +254,7 @@ virtfs_vget_common(struct mount *mp, struct virtfs_node *np, int flags,
 	uint32_t hash;
 	int error;
 	struct virtfs_inode *inode;
+	int error_reload = 0;
 
 	td = curthread;
 	vmp = VFSTOP9(mp);
@@ -333,19 +337,6 @@ virtfs_vget_common(struct mount *mp, struct virtfs_node *np, int flags,
 	inode->i_qid_path = fid->qid.path;
 	VIRTFS_SET_LINKS(inode);
 
-	/*
-	 * Add the VirtFS node to the list for cleanup later.
-	 * Cleanup of this VirtFS node from the list of session
-	 * VirtFS nodes happen in vput() :
-	 * 	- In vfs_hash_insert() after inserting this node
-	 *	  to the VFS hash table.
-	 *	- In error handling below.
-	 */
-	VIRTFS_LOCK(vses);
-	STAILQ_INSERT_TAIL(&vses->virt_node_list, np, virtfs_node_next);
-	np->flags |= VIRTFS_NODE_IN_SESSION;
-	VIRTFS_UNLOCK(vses);
-
 	lockmgr(vp->v_vnlock, LK_EXCLUSIVE, NULL);
 	error = insmntque(vp, mp);
 	if (error != 0) {
@@ -359,19 +350,27 @@ virtfs_vget_common(struct mount *mp, struct virtfs_node *np, int flags,
 	/* Init the vnode with the disk info*/
 	error = virtfs_reload_stats_dotl(vp);
 	if (error != 0) {
-		vput(vp);
+		error_reload = 1;
 		goto out;
 	}
 
 	error = vfs_hash_insert(vp, hash, flags, td, vpp,
 	    virtfs_node_cmp, &fid->qid);
-	if (error != 0) {
+	if (error != 0 || *vpp != NULL) {
+		/*
+		 *  vp is vput already: either v2 from free list vnode
+		 *  found with the hash and is assigned to vpp or v2
+		 *  retrieval fails.
+		 */
 		goto out;
 	}
-	if (*vpp == NULL) {
-		*vpp = vp;
-	}
 
+	VIRTFS_LOCK(vses);
+	STAILQ_INSERT_TAIL(&vses->virt_node_list, np, virtfs_node_next);
+	np->flags |= VIRTFS_NODE_IN_SESSION;
+	VIRTFS_UNLOCK(vses);
+
+	*vpp = vp;
 	return (0);
 out:
 	if (!IS_ROOT(np)) {
@@ -382,19 +381,12 @@ virtfs_vget_common(struct mount *mp, struct virtfs_node *np, int flags,
 
 	/* Something went wrong, dispose the node */
 
-	/*
-	 * Remove the virtfs_node from the list before we cleanup.
-	 * This should ideally have been removed in vput() above.
-	 * We try again here, incase it is missed from vput(), as
-	 * we added this vnode explicitly to virt_node_list above.
-	 */
-	VIRTFS_LOCK(vses);
-	if ((np->flags & VIRTFS_NODE_IN_SESSION) != 0) {
-		np->flags &= ~VIRTFS_NODE_IN_SESSION;
-		STAILQ_REMOVE(&vses->virt_node_list, np, virtfs_node, virtfs_node_next);
-	}
-	VIRTFS_UNLOCK(vses);
 	virtfs_dispose_node(&np);
+
+	if (error_reload) {
+		vput(vp);
+	}
+
 	*vpp = NULLVP;
 	return (error);
 }

sys/dev/virtio/9pfs/virtfs_vnops.c

@@ -99,9 +99,22 @@ virtfs_cleanup(struct virtfs_node *np)
 	struct vnode *vp;
 	struct virtfs_session *vses;
 
+	if (np == NULL)
+		return;
+
 	vp = VIRTFS_NTOV(np);
 	vses = np->virtfs_ses;
 
+	VIRTFS_LOCK(vses);
+	if ((np->flags & VIRTFS_NODE_IN_SESSION) != 0) {
+		np->flags &= ~VIRTFS_NODE_IN_SESSION;
+		STAILQ_REMOVE(&vses->virt_node_list, np, virtfs_node, virtfs_node_next);
+	} else {
+		VIRTFS_UNLOCK(vses);
+		return;
+	}
+	VIRTFS_UNLOCK(vses);
+
 	/* Invalidate all entries to a particular vnode. */
 	cache_purge(vp);
 	/* Destroy the vm object and flush associated pages. */
@@ -118,14 +131,6 @@ virtfs_cleanup(struct virtfs_node *np)
 	VIRTFS_VFID_LOCK_DESTROY(np);
 	VIRTFS_VOFID_LOCK_DESTROY(np);
 
-	/* Remove the virtfs_node from the list before we cleanup.*/
-	VIRTFS_LOCK(vses);
-	if ((np->flags & VIRTFS_NODE_IN_SESSION) != 0) {
-		np->flags &= ~VIRTFS_NODE_IN_SESSION;
-		STAILQ_REMOVE(&vses->virt_node_list, np, virtfs_node, virtfs_node_next);
-	}
-	VIRTFS_UNLOCK(vses);
-
 	/* Dispose all node knowledge.*/
 	virtfs_dispose_node(&np);
 }