Fix races between notify invalidation and creation/handling of inodes

fs/fuse/dir.c

@@ -138,6 +138,26 @@ static void fuse_dir_changed(struct inode *dir)
 	inode_maybe_inc_iversion(dir, false);
 }
 
+/*
+ * Note in the parent directory that one of its entries was invalidated.
+ *
+ * This bumps the same change counter as fuse_dir_changed(), which doubles as a
+ * generation for the parent's entry cache: fuse_dentry_revalidate() samples it
+ * before sending its LOOKUP and refuses to re-arm the entry timeout if it moved
+ * meanwhile.
+ *
+ * Only for invalidations coming from the server - a plain
+ * fuse_invalidate_entry_cache() (a negative lookup, say) must not bump the
+ * counter, or it would needlessly drop the readdir cache as well.
+ */
+void fuse_entry_invalidated(struct dentry *entry)
+{
+	/* d_lock keeps d_parent stable; a live child pins its parent inode */
+	spin_lock(&entry->d_lock);
+	inode_maybe_inc_iversion(d_inode(entry->d_parent), false);
+	spin_unlock(&entry->d_lock);
+}
+
 /*
  * Mark the attributes as stale due to an atime change.  Avoid the invalidate if
  * atime is not used.
@@ -275,7 +295,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 		struct fuse_lookupx_out ext_out;
 		struct fuse_statx sx;
 		struct fuse_forget_link *forget;
-		u64 attr_version;
+		u64 attr_version, dir_version;
 		uint32_t lookupx_flags = FUSE_LOOKUPX_FOR_REVALIDATE;
 
 		/* For negative dentries, always do a fresh lookup */
@@ -298,6 +318,19 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 			lookupx_flags |= FUSE_LOOKUPX_TARGET_WAS_DIR;
 
 		parent = dget_parent(entry);
+		/*
+		 * Unlike ->lookup() and the create paths, revalidate runs
+		 * without the parent's i_rwsem, so it does not exclude
+		 * fuse_reverse_inval_entry().  An invalidation for this name
+		 * can therefore land while the LOOKUP is in flight, and
+		 * re-arming the entry timeout from the reply below would
+		 * silently undo it - permanently so for FUSE_EXPIRE_ONLY and
+		 * for fuse_prune_aliases(), which leave the dentry hashed.
+		 *
+		 * Sample the parent's entry-cache generation here and compare
+		 * it once the reply is in.
+		 */
+		dir_version = inode_query_iversion(d_inode(parent));
 		ret = fuse_do_lookupx(fm, get_node_id(d_inode(parent)),
 					&entry->d_name, &ext_out,
 					lookupx_flags);
@@ -329,7 +362,11 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 		fuse_change_attributes(inode, &ext_out.entry.attr, &sx,
 				       ATTR_TIMEOUT(&ext_out.entry),
 				       attr_version);
-		fuse_change_entry_timeout(entry, &ext_out.entry);
+
+		parent = dget_parent(entry);
+		if (inode_peek_iversion(d_inode(parent)) == dir_version)
+			fuse_change_entry_timeout(entry, &ext_out.entry);
+		dput(parent);
 	} else if (inode) {
 		fi = get_fuse_inode(inode);
 		if (flags & LOOKUP_RCU) {
@@ -696,6 +733,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	struct fuse_entry_out outentry;
 	struct fuse_inode *fi;
 	struct fuse_file *ff;
+	u64 evict_ctr;
 	bool trunc = flags & O_TRUNC;
 
 	/* Userspace expects S_IFREG in create mode */
@@ -743,6 +781,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (err)
 		goto out_free_ff;
 
+	evict_ctr = fuse_get_evict_ctr(fm->fc);
+
 	err = fuse_simple_request(fm, &args);
 	free_ext_value(&args);
 	if (err)
@@ -757,7 +797,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	ff->nodeid = outentry.nodeid;
 	ff->open_flags = outopen.open_flags;
 	inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
-			  &outentry.attr, ATTR_TIMEOUT(&outentry), 0, 0);
+			  &outentry.attr, ATTR_TIMEOUT(&outentry), 0, evict_ctr);
 	if (!inode) {
 		flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
 		fuse_sync_release(NULL, ff, flags);
@@ -854,6 +894,7 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	struct dentry *d;
 	int err;
 	struct fuse_forget_link *forget;
+	u64 evict_ctr;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
@@ -874,6 +915,8 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 			goto out_put_forget_req;
 	}
 
+	evict_ctr = fuse_get_evict_ctr(fm->fc);
+
 	err = fuse_simple_request(fm, args);
 	free_ext_value(args);
 	if (err)
@@ -887,7 +930,7 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 		goto out_put_forget_req;
 
 	inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
-			  &outarg.attr, ATTR_TIMEOUT(&outarg), 0, 0);
+			  &outarg.attr, ATTR_TIMEOUT(&outarg), 0, evict_ctr);
 	if (!inode) {
 		fuse_queue_forget(fm->fc, forget, outarg.nodeid, 1);
 		return -ENOMEM;

fs/fuse/fuse_i.h

@@ -909,9 +909,6 @@ struct fuse_conn {
 	/** Version counter for attribute changes */
 	atomic64_t attr_version;
 
-	/** Waitqueue for attr_version initialization */
-	wait_queue_head_t attr_version_waitq;
-
 	/** Version counter for evict inode */
 	atomic64_t evict_ctr;
 
@@ -1267,6 +1264,8 @@ void fuse_invalidate_attr_mask(struct inode *inode, u32 mask);
 
 void fuse_invalidate_entry_cache(struct dentry *entry);
 
+void fuse_entry_invalidated(struct dentry *entry);
+
 void fuse_invalidate_atime(struct inode *inode);
 
 u64 fuse_time_to_jiffies(u64 sec, u32 nsec);

fs/fuse/inode.c

@@ -216,6 +216,26 @@ static ino_t fuse_squash_ino(u64 ino64)
 	return ino;
 }
 
+/*
+ * May attributes returned by a request be marked valid, i.e. cleared
+ * from inval_mask?
+ *
+ * Not if they come from a fuse_iget() call whose request raced with an
+ * evict or a reverse invalidation - either would have invalidated the
+ * result if the inode's attr_version would've been preserved.
+ *
+ * !evict_ctr -> this is not fuse_iget()
+ * fi->attr_version != 0 -> this is not a new inode
+ * evict_ctr == fuse_get_evict_ctr() -> no evicts or missed
+ * invalidations during the request
+ */
+static bool fuse_may_validate_attrs(struct fuse_conn *fc,
+				    struct fuse_inode *fi, u64 evict_ctr)
+{
+	return !evict_ctr || fi->attr_version ||
+	       evict_ctr == fuse_get_evict_ctr(fc);
+}
+
 /*
  * Handle statx-specific attribute updates with partial attribute support.
  */
@@ -231,18 +251,8 @@ static void fuse_change_attributes_common_sx(struct inode *inode,
 
 	lockdep_assert_held(&fi->lock);
 
-	/*
-	 * Clear returned basic stats from invalid mask.
-	 *
-	 * Don't do this if this is coming from a fuse_iget() call and there
-	 * might have been a racing evict which would've invalidated the result
-	 * if the attr_version would've been preserved.
-	 *
-	 * !evict_ctr -> this is create
-	 * fi->attr_version != 0 -> this is not a new inode
-	 * evict_ctr == fuse_get_evict_ctr() -> no evicts while during request
-	 */
-	if (!evict_ctr || fi->attr_version || evict_ctr == fuse_get_evict_ctr(fc))
+	/* Clear returned basic stats from invalid mask */
+	if (fuse_may_validate_attrs(fc, fi, evict_ctr))
 		set_mask_bits(&fi->inval_mask, returned_attrs, 0);
 
 	fi->attr_version = atomic64_inc_return(&fc->attr_version);
@@ -369,22 +379,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 							evict_ctr);
 	}
 
-	/*
-	 * Clear basic stats from invalid mask.
-	 *
-	 * Don't do this if this is coming from a fuse_iget() call and there
-	 * might have been a racing evict which would've invalidated the result
-	 * if the attr_version would've been preserved.
-	 *
-	 * !evict_ctr -> this is create
-	 * fi->attr_version != 0 -> this is not a new inode
-	 * evict_ctr == fuse_get_evict_ctr() -> no evicts while during request
-	 */
-	if (!evict_ctr || fi->attr_version || evict_ctr == fuse_get_evict_ctr(fc))
+	/* Clear basic stats from invalid mask */
+	if (fuse_may_validate_attrs(fc, fi, evict_ctr))
 		set_mask_bits(&fi->inval_mask, STATX_BASIC_STATS, 0);
 
 	fi->attr_version = atomic64_inc_return(&fc->attr_version);
-	wake_up_all(&fc->attr_version_waitq);
 	fi->i_time = attr_valid;
 	fi->i_perm_time = attr_valid;
 
@@ -619,6 +618,7 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 	struct inode *inode;
 	struct fuse_inode *fi;
 	struct fuse_conn *fc = get_fuse_conn_super(sb);
+	bool is_new_inode = false;
 
 	/*
 	 * Auto mount points get their node id from the submount root, which is
@@ -654,13 +654,13 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 	if (!inode)
 		return NULL;
 
-	if ((inode->i_state & I_NEW)) {
+	is_new_inode = inode->i_state & I_NEW;
+	if (is_new_inode) {
 		inode->i_flags |= S_NOATIME;
 		if (!fc->writeback_cache || !S_ISREG(attr->mode))
 			inode->i_flags |= S_NOCMTIME;
 		inode->i_generation = generation;
 		fuse_init_inode(inode, attr, fc);
-		unlock_new_inode(inode);
 	} else if (fuse_stale_inode(inode, generation, attr)) {
 		/* nodeid was reused, any I/O on the old inode should fail */
 		fuse_make_bad(inode);
@@ -677,6 +677,8 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
 done:
 	fuse_change_attributes_i(inode, attr, NULL, attr_valid, attr_version,
 				 evict_ctr);
+	if (is_new_inode)
+		unlock_new_inode(inode);
 	return inode;
 }
 
@@ -708,6 +710,7 @@ static void fuse_prune_aliases(struct inode *inode)
 
 	spin_lock(&inode->i_lock);
 	hlist_for_each_entry(dentry, &inode->i_dentry, d_u.d_alias) {
+		fuse_entry_invalidated(dentry);
 		fuse_invalidate_entry_cache(dentry);
 	}
 	spin_unlock(&inode->i_lock);
@@ -724,6 +727,7 @@ static void fuse_invalidate_inode_entry(struct inode *inode)
 		dentry = d_find_alias(inode);
 		if (dentry) {
 			d_invalidate(dentry);
+			fuse_entry_invalidated(dentry);
 			fuse_invalidate_entry_cache(dentry);
 			dput(dentry);
 		}
@@ -735,6 +739,7 @@ static void fuse_invalidate_inode_entry(struct inode *inode)
 			if (!d_unhashed(dentry))
 				__d_drop(dentry);
 			spin_unlock(&dentry->d_lock);
+			fuse_entry_invalidated(dentry);
 			fuse_invalidate_entry_cache(dentry);
 		}
 		spin_unlock(&inode->i_lock);
@@ -750,21 +755,29 @@ int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid,
 	pgoff_t pg_end;
 
 	inode = fuse_ilookup(fc, nodeid, NULL);
-	if (!inode)
-		return -ENOENT;
+	if (!inode) {
+		/*
+		 * Not in the icache, but a LOOKUP/READDIRPLUS reply carrying
+		 * pre-invalidation attributes may be about to create it.  Bump
+		 * evict_ctr so that such a reply leaves the new inode's
+		 * attributes invalid (see fuse_change_attributes_common()),
+		 * then look up again in case the inode was hashed while we
+		 * were bumping.  ilookup5() and iget5_locked() serialize on
+		 * the inode hash lock, so if the second lookup still misses,
+		 * the creator inserted after it and is guaranteed to observe
+		 * the bumped counter.
+		 */
+		atomic64_inc(&fc->evict_ctr);
+		inode = fuse_ilookup(fc, nodeid, NULL);
+		if (!inode)
+			return -ENOENT;
+	}
 
 	fi = get_fuse_inode(inode);
-
 	spin_lock(&fi->lock);
-	while (fi->attr_version == 0) {
-		spin_unlock(&fi->lock);
-		wait_event(fc->attr_version_waitq, READ_ONCE(fi->attr_version) != 0);
-		spin_lock(&fi->lock);
-	}
-
 	fi->attr_version = atomic64_inc_return(&fc->attr_version);
 	spin_unlock(&fi->lock);
-	
+
 	if (fc->inval_inode_entries)
 		fuse_invalidate_inode_entry(inode);
 	else if (fc->expire_inode_entries)
@@ -1182,7 +1195,6 @@ void fuse_conn_init(struct fuse_conn *fc, struct fuse_mount *fm,
 	refcount_set(&fc->count, 1);
 	atomic_set(&fc->dev_count, 1);
 	init_waitqueue_head(&fc->blocked_waitq);
-	init_waitqueue_head(&fc->attr_version_waitq);
 	fuse_iqueue_init(&fc->iq, fiq_ops, fiq_priv);
 	INIT_LIST_HEAD(&fc->bg_queue);
 	INIT_LIST_HEAD(&fc->entry);