fuse: fix some subtle deadlocks and lock irregularities

fs/fuse/dir.c

@@ -2040,6 +2040,12 @@ int fuse_flush_times(struct inode *inode, struct fuse_file *ff)
 		inarg.valid |= FATTR_FH;
 		inarg.fh = ff->fh;
 	}
+	/*
+	 * This is ->write_inode() flushing times the kernel owns locally, not
+	 * a userspace utimes(); let the server tell the two apart.
+	 */
+	if (fm->fc->setattr_writeback)
+		inarg.valid |= FATTR_WRITEBACK;
 	fuse_setattr_fill(fm->fc, &args, inode, &inarg, &outarg);
 
 	return fuse_simple_request(fm, &args);

fs/fuse/file.c

@@ -1899,7 +1899,31 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		if (err)
 			return err;
 
-		if (!fc->handle_killpriv_v2 ||
+		/*
+		 * A write that drops suid/sgid stays off the writeback path,
+		 * so it holds i_rwsem exclusive.
+		 *
+		 * With handle_killpriv_v2 that is because the server does the
+		 * killing from the WRITE itself.  Without it, fuse_setattr()
+		 * has to ask the server, and fuse_do_setattr() freezes
+		 * writepages around that SETATTR: fuse_set_nowrite() asserts
+		 * BUG_ON(fi->writectr < 0), which assumes an exclusive
+		 * i_rwsem, and the DLM-relaxed buffered write path below holds
+		 * it only shared.  Two writers can both see the bits set
+		 * before either has cleared them, and the second one would
+		 * then oops inside spin_lock(&fi->lock).
+		 *
+		 * Only the DLM path needs the detour: everywhere else the
+		 * buffered write already holds i_rwsem exclusive, so the two
+		 * writers cannot overlap in the first place.
+		 *
+		 * The bits are read without the inode lock here, so a server
+		 * attribute update can still set them between this test and
+		 * file_remove_privs().  That leaves the same race, but only
+		 * for writers whose mode changed underneath them, rather than
+		 * for every write to a suid file.
+		 */
+		if (!(fc->handle_killpriv_v2 || fc->dlm) ||
 		    !setattr_should_drop_suidgid(idmap, file_inode(file)))
 			writeback = true;
 	}
@@ -1964,6 +1988,30 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			goto out;
 	}
 
+	err = count = generic_write_checks(iocb, from);
+	if (err <= 0)
+		goto out;
+
+	/*
+	 * Kill suid/sgid and stamp the timestamps here, before the gate,
+	 * instead of leaving them next to the write itself.  kiocb_modified()
+	 * -> file_remove_privs() is the one that reaches the server: without
+	 * handle_killpriv[_v2] fuse_setattr() kills the bits by asking it (a
+	 * FUSE_GETATTR to refresh the mode, then a FUSE_SETATTR, which for a
+	 * writeback inode first flushes and freezes writepages), and
+	 * security_inode_killpriv() can drop the capability xattr with another
+	 * round trip.  A server may have to invalidate this inode from inside
+	 * such a handler; its NOTIFY_INVAL_INODE then blocks in
+	 * percpu_down_write() draining a gate reader that is itself waiting for
+	 * the reply.  Nothing held under the gate may wait for the server.
+	 *
+	 * This also runs before the forced-DIO re-route below, so a re-routed
+	 * write repeats it; there is nothing left to do the second time.
+	 */
+	err = kiocb_modified(iocb);
+	if (err)
+		goto out;
+
 	wb_guard = !!wb_sem;
 	if (wb_guard) {
 retry:
@@ -1989,16 +2037,8 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		}
 	}
 
-	err = count = generic_write_checks(iocb, from);
-	if (err <= 0)
-		goto out;
-
 	task_io_account_write(count);
 
-	err = kiocb_modified(iocb);
-	if (err)
-		goto out;
-
 	if (iocb->ki_flags & IOCB_DIRECT) {
 		written = generic_file_direct_write(iocb, from);
 		if (written < 0 || !iov_iter_count(from))

fs/fuse/fuse_dlm_cache.c

@@ -562,6 +562,37 @@ bool fuse_dlm_range_is_locked(struct fuse_inode *inode, uint64_t start,
 	return true;
 }
 
+/**
+ * fuse_dlm_write_grant_exists - does the inode hold an exclusive grant anywhere
+ * @fi: the fuse inode
+ *
+ * Unlike fuse_dlm_range_is_locked(), which asks whether one range is fully
+ * covered, this asks whether any part of the file is held exclusively.  A
+ * client that holds a write grant may be sitting on dirty page cache the
+ * server has not seen, so its mtime and ctime run ahead of anything the
+ * server can report.
+ *
+ * Return: true if at least one recorded range is held for write
+ */
+bool fuse_dlm_write_grant_exists(struct fuse_inode *fi)
+{
+	struct fuse_dlm_cache *cache = &fi->dlm_locked_areas;
+	struct fuse_dlm_range *range;
+	bool held = false;
+
+	down_read(&cache->lock);
+	for (range = fuse_dlm_find_overlapping(cache, 0, U64_MAX); range;
+	     range = fuse_page_it_iter_next(range, 0, U64_MAX)) {
+		if (range->mode == FUSE_PCACHE_LK_WRITE) {
+			held = true;
+			break;
+		}
+	}
+	up_read(&cache->lock);
+
+	return held;
+}
+
 /**
  * fuse_dlm_lock_is_held - check that a byte range is covered by a granted lock
  * @fi:     the fuse inode

fs/fuse/fuse_dlm_cache.h

@@ -71,6 +71,9 @@ bool fuse_dlm_range_is_locked(struct fuse_inode *inode, uint64_t start,
 bool fuse_dlm_lock_is_held(struct fuse_inode *inode, loff_t offset,
 			   size_t length, enum fuse_page_lock_mode mode);
 
+/* Is any part of the file held for write? */
+bool fuse_dlm_write_grant_exists(struct fuse_inode *inode);
+
 /* This is the interface to the filesystem */
 int fuse_get_dlm_lock(struct file *file, loff_t offset,
 		      size_t length, enum fuse_page_lock_mode mode);

fs/fuse/fuse_i.h

@@ -874,6 +874,9 @@ struct fuse_conn {
 	/*  expire inode entries when doing inode invalidation */
 	unsigned expire_inode_entries:1;
 
+	/*  mark writeback-initiated SETATTR requests with FATTR_WRITEBACK */
+	unsigned setattr_writeback:1;
+
 	/*
 	 * The following bitfields are only for optimization purposes
 	 * and hence races in setting them will not cause malfunction

fs/fuse/inode.c

@@ -512,6 +512,68 @@ u32 fuse_get_cache_mask(struct inode *inode)
 	return STATX_MTIME | STATX_CTIME | STATX_SIZE;
 }
 
+/*
+ * Which cached attributes survive a server reply.
+ *
+ * Without DLM this is fuse_get_cache_mask(): with the writeback cache on,
+ * writes update mtime and ctime and may extend i_size locally, the server
+ * knows about none of it, so the cached values win.
+ *
+ * With DLM the server is the authority (fuse_get_cache_mask() returns 0),
+ * because another node may have changed the file behind us and only the
+ * server can say so.  That holds for the parts of the file we do not own.  A
+ * write grant means no other node can touch the range until we are revoked,
+ * so anything the server reports about it is at best as new as what we have,
+ * and older if we still have unwritten data there.  Keep the cached values
+ * for exactly what the grant covers:
+ *
+ *  - size, when the server reports less than i_size and the tail it does not
+ *    know about, [srv_size, i_size), is entirely under a write grant.  Taking
+ *    the server's answer would shrink i_size and have truncate_pagecache()
+ *    throw the unwritten tail away.
+ *  - mtime and ctime, while a write grant covers unwritten data: our writes
+ *    have stamped them locally and the server's stamps predate them.  Only
+ *    while the cache is actually dirty, not for as long as the grant lives:
+ *    a grant is held until it is revoked or the inode is evicted, and past
+ *    the writeback the server's stamps are the newer ones.  Keeping ours
+ *    beyond that would hide a remote chown or chmod indefinitely.
+ *
+ * A remote truncate cannot slip through.  It has to revoke the grant first,
+ * and the revoke launders the tail and drops the grant, so by the time the
+ * smaller size is reported neither check holds and the server's answer is
+ * applied as usual.  A grant the server made but that could not be recorded
+ * (FUSE_DLM_GRANT_UNRECORDED) is invisible to the lock tree and falls back to
+ * trusting the server, as before.
+ *
+ * Must be called without fi->lock: the lock tree query sleeps.
+ */
+static u32 fuse_attr_cache_mask(struct inode *inode, struct fuse_attr *attr,
+				bool have_size)
+{
+	struct fuse_conn *fc = get_fuse_conn(inode);
+	struct fuse_inode *fi = get_fuse_inode(inode);
+	u32 cache_mask = fuse_get_cache_mask(inode);
+	loff_t size = i_size_read(inode);
+
+	if (cache_mask || !fc->dlm || !fc->writeback_cache ||
+	    !S_ISREG(inode->i_mode))
+		return cache_mask;
+
+	if (!fuse_dlm_write_grant_exists(fi))
+		return cache_mask;
+
+	if (mapping_tagged(inode->i_mapping, PAGECACHE_TAG_DIRTY) ||
+	    mapping_tagged(inode->i_mapping, PAGECACHE_TAG_WRITEBACK))
+		cache_mask |= STATX_MTIME | STATX_CTIME;
+
+	if (have_size && size > (loff_t) attr->size &&
+	    fuse_dlm_lock_is_held(fi, attr->size, size - attr->size,
+				  FUSE_PAGE_LOCK_WRITE))
+		cache_mask |= STATX_SIZE;
+
+	return cache_mask;
+}
+
 static void fuse_change_attributes_i(struct inode *inode, struct fuse_attr *attr,
 				     struct fuse_statx *sx, u64 attr_valid,
 				     u64 attr_version, u64 evict_ctr)
@@ -524,15 +586,11 @@ static void fuse_change_attributes_i(struct inode *inode, struct fuse_attr *attr
 	bool have_size = !sx || (sx->mask & STATX_SIZE);
 	u64 srv_size;
 
+	cache_mask = fuse_attr_cache_mask(inode, attr, have_size);
+
 	spin_lock(&fi->lock);
 	srv_size = attr->size;
 
-	/*
-	 * In case of writeback_cache enabled, writes update mtime, ctime and
-	 * may update i_size.  In these cases trust the cached value in the
-	 * inode.
-	 */
-	cache_mask = fuse_get_cache_mask(inode);
 	if (cache_mask & STATX_SIZE)
 		attr->size = i_size_read(inode);
 
@@ -580,11 +638,16 @@ static void fuse_change_attributes_i(struct inode *inode, struct fuse_attr *attr
 	spin_unlock(&fi->lock);
 
 	/*
-	 * Only do page cache invalidation when cache_mask is not set
-	 * (writeback_cache disabled) AND the relevant attributes (SIZE/MTIME)
-	 * were actually returned by the server.
+	 * Only do page cache invalidation when the size was not served from
+	 * the cache (writeback_cache disabled, or no grant covering the tail)
+	 * AND the relevant attributes (SIZE/MTIME) were actually returned by
+	 * the server.  This has to key off STATX_SIZE alone: i_size_write()
+	 * above took the server's size for any mask without that bit, and the
+	 * cache has to be truncated to match it.  The mtime branch neutralises
+	 * itself when STATX_MTIME is set, since attr->mtime then holds the
+	 * value old_mtime was read from.
 	 */
-	if (!cache_mask && S_ISREG(inode->i_mode)) {
+	if (!(cache_mask & STATX_SIZE) && S_ISREG(inode->i_mode)) {
 		bool inval = false;
 		bool have_mtime = !sx || (sx->mask & STATX_MTIME);
 
@@ -851,6 +914,40 @@ static void fuse_dlm_revoke_inval_range(struct fuse_inode *fi, loff_t offset,
 	fuse_dlm_unlock_range(fi, start, end);
 }
 
+/*
+ * Drop a page-cache range on behalf of a NOTIFY invalidate.
+ *
+ * invalidate_inode_pages2_range() waits out folios under writeback and
+ * launders dirty ones, both of which need a FUSE_WRITE reply.  While
+ * writepages are frozen (fuse_set_nowrite(): truncate, O_TRUNC open, fsync,
+ * pre-SETATTR flush) no reply can arrive, because fuse_flush_writepages()
+ * parks the request on fi->queued_writes until fuse_release_nowrite().  A
+ * server that revokes from inside the handler it is revoking for then
+ * deadlocks against its own reply.  fuse_do_setattr() states the same rule
+ * for its own invalidate.
+ *
+ * So while frozen use invalidate_mapping_pages(), which skips dirty and
+ * under-writeback folios and never blocks.  The stale clean folios still
+ * go, and the freezes that span a request drop the cache themselves once
+ * they complete: fuse_do_setattr() invalidates the mapping after releasing
+ * the freeze, the O_TRUNC open path calls truncate_pagecache().
+ */
+static void fuse_notify_invalidate_range(struct inode *inode, pgoff_t start,
+					 pgoff_t end)
+{
+	struct fuse_inode *fi = get_fuse_inode(inode);
+	bool frozen;
+
+	spin_lock(&fi->lock);
+	frozen = fi->writectr < 0;
+	spin_unlock(&fi->lock);
+
+	if (frozen)
+		invalidate_mapping_pages(inode->i_mapping, start, end);
+	else
+		invalidate_inode_pages2_range(inode->i_mapping, start, end);
+}
+
 int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid,
 			     loff_t offset, loff_t len)
 {
@@ -984,10 +1081,10 @@ int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid,
 			 * notified range.
 			 */
 			if (fuse_inode_force_dio(inode))
-				invalidate_inode_pages2(inode->i_mapping);
+				fuse_notify_invalidate_range(inode, 0, -1);
 			else
-				invalidate_inode_pages2_range(inode->i_mapping,
-							      pg_start, pg_end);
+				fuse_notify_invalidate_range(inode, pg_start,
+							     pg_end);
 
 			percpu_up_write(wb_sem);
 
@@ -1000,8 +1097,7 @@ int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid,
 			 * range unserialized (best-effort), as before. */
 			if (fc->dlm && fc->writeback_cache)
 				fuse_dlm_revoke_inval_range(fi, offset, len);
-			invalidate_inode_pages2_range(inode->i_mapping,
-						      pg_start, pg_end);
+			fuse_notify_invalidate_range(inode, pg_start, pg_end);
 		}
 	}
 	iput(inode);
@@ -1912,6 +2008,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->inval_inode_entries = 1;
 			if (flags & FUSE_EXPIRE_INODE_ENTRY)
 				fc->expire_inode_entries = 1;
+			if (flags & FUSE_SETATTR_WRITEBACK)
+				fc->setattr_writeback = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1965,7 +2063,8 @@ static struct fuse_init_args *fuse_new_init(struct fuse_mount *fm)
 		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
 		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_ALLOW_IDMAP |
 		FUSE_REQUEST_TIMEOUT | FUSE_INVAL_INODE_ENTRY |
-		FUSE_EXPIRE_INODE_ENTRY | FUSE_URING_REDUCED_Q;
+		FUSE_EXPIRE_INODE_ENTRY | FUSE_URING_REDUCED_Q |
+		FUSE_SETATTR_WRITEBACK;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;

include/uapi/linux/fuse.h

@@ -366,6 +366,17 @@ struct fuse_file_lock {
 #define FATTR_LOCKOWNER	(1 << 9)
 #define FATTR_CTIME	(1 << 10)
 #define FATTR_KILL_SUIDGID	(1 << 11)
+/*
+ * Not an attribute selector: marks the request as a kernel-initiated
+ * writeback of locally owned attributes rather than a userspace-initiated
+ * change.  Only sent if the server negotiated FUSE_SETATTR_WRITEBACK.
+ *
+ * The bit is deliberately far above the sequentially allocated FATTR_*
+ * range: libfuse mirrors these bits into its own FUSE_SET_ATTR_* space,
+ * which has its own allocations from bit 12 upwards, and only a value that
+ * is free on both sides can be passed through without translation.
+ */
+#define FATTR_WRITEBACK		(1 << 30)
 
 /**
  * Flags returned by the OPEN request
@@ -449,6 +460,8 @@ struct fuse_file_lock {
  *			optimal io-size alignment
  * FUSE_URING_REDUCED_Q: Client (kernel) supports less queues - Server is free
  *			 to register between 1 and nr-core io-uring queues
+ * FUSE_SETATTR_WRITEBACK: kernel marks writeback-initiated SETATTR requests
+ *			   with FATTR_WRITEBACK
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -498,6 +511,7 @@ struct fuse_file_lock {
 #define FUSE_REQUEST_TIMEOUT	(1ULL << 42)
 
 #define FUSE_ALIGN_PG_ORDER	(1ULL << 50)
+#define FUSE_SETATTR_WRITEBACK	(1ULL << 58)
 #define FUSE_URING_REDUCED_Q	(1ULL << 59)
 #define FUSE_INVAL_INODE_ENTRY  (1ULL << 60)
 #define FUSE_EXPIRE_INODE_ENTRY (1ULL << 61)