[Devel] [PATCH VZ10 v2] fs/fuse: align fuse_create_open()'s open path with fuse_open()

[Denis V. Lunev]

======================================================================
NOTE: the crash-dump root-cause analysis and the VFS/refcount reasoning
in this review were produced with the help of an AI assistant. I have
gone over the conclusions against the source myself and stand behind
them, but the detailed mechanism description below is AI-assisted --
please read it with that in mind and double-check before relying on it.
======================================================================

On Tue, Jun 23, 2026 at 02:43:18PM +0800, Liu Kui wrote:
> fuse_create_open()'s close_wait branch was not correct -- in particular
> the fput() in its error path was wrong; fuse_open()'s close_wait branch
> should apply instead. Refactor the close_wait branch into a new
> function used in both places. fuse_sync_release() applies to the
> close_wait error path in both places.

The diagnosis of the original bug is right, and so is the !err guard.
But the chosen cure -- routing both callers through one helper that
calls fuse_sync_release() on the error path -- is not correct for
fuse_create_open(). The two call sites are not symmetric, and this
patch releases the fuse_file twice on the atomic-open path.

NAK in this form. Details below.

> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> @@ -722,22 +722,8 @@ static int fuse_create_open(...)
>  			invalidate_inode_pages2(inode->i_mapping);
>  	}
>
> -	if (fm->fc->close_wait) {
> -		...
> -		if (need_open && fm->fc->kio.op && fm->fc->kio.op->file_open) {
> -			err = fm->fc->kio.op->file_open(file, inode);
> -			if (err)
> -				fput(file);
> -		}
> -		inode_unlock(inode);
> -	}
> +	if (!err && fm->fc->close_wait)
> +		err = fuse_open_close_wait(inode, file);
>
>  	return err;

Dropping the bogus fput() and gating on !err is correct -- agreed.

The problem is what fuse_open_close_wait() now does on error in this
caller.

> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> +int fuse_open_close_wait(struct inode *inode, struct file *file)
> +{
> +	struct fuse_conn *fc = get_fuse_conn(inode);
> +	struct fuse_inode *fi = get_fuse_inode(inode);
> +	struct fuse_file *ff = file->private_data;
> +	int err = 0;
> +
> +	inode_lock(inode);
> +	spin_lock(&fi->lock);
> +
> +	if (++fi->num_openers == 1 || fi->i_size_unstable) {
> +		...
> +		if (!err && fc->kio.op && fc->kio.op->file_open)
> +			err = fc->kio.op->file_open(file, inode);
> +		...
> +		if (err)
> +			fi->num_openers--;
> +	}
> +	...
> +	if (err)
> +		fuse_sync_release(fi, ff, file->f_flags);
> +
> +	return err;
> +}

fuse_sync_release() is safe in fuse_open(), but not when this helper is
called from fuse_create_open(), because the two callers hand us the
file in different states:

  - fuse_open() is the ->open callback. do_dentry_open() sets
    FMODE_OPENED only *after* ->open returns 0 (fs/open.c). On an error
    return the file is NOT FMODE_OPENED, so __fput() will not call
    ->release (fs/file_table.c: __fput() bails out early when
    !FMODE_OPENED). Hence fuse_open() must release ff itself, and
    fuse_sync_release()/fuse_release_common() is the only release. Fine.

  - fuse_create_open() reaches the close_wait branch only after
    finish_open() has succeeded, i.e. the file IS FMODE_OPENED. On an
    error return from ->atomic_open() the VFS still owns that reference:
    path_openat() does an unconditional fput(file) on the error path,
    and because the file is FMODE_OPENED that fput() runs fuse_release()
    -> fuse_release_common() -> fuse_file_put(ff).

So on the atomic path this patch releases ff twice:

  1. fuse_open_close_wait():  fi->num_openers-- ; fuse_sync_release()
     -> fuse_file_put(ff): ff->count 1 -> 0, ff is kfree()d. Note
     file->private_data is left pointing at the freed ff and the file
     stays FMODE_OPENED.

  2. path_openat() fput(file) -> __fput() -> fuse_release():
     reads the now-freed ff (use-after-free) and fuse_release_common()
     -> fuse_file_put(ff) again (double free). On writeback_cache
     mounts -- which is the vstorage case here -- fuse_release() also
     runs fi->num_openers-- a second time (the decrement at the top of
     fuse_release() is gated on writeback_cache, while the open-side
     increment is gated on close_wait), so the open count underflows on
     top of the double free.

That is exactly the openat(O_CREAT|...) path that crashed in
VSTOR-136027. The original code's double put was on the struct file and
was at least caught loudly by the file_ref imbalance WARN; this turns it
into a silent fuse_file use-after-free plus a num_openers underflow on
the same path. A KASAN build opening a file with O_CREAT|O_TRUNC on a
close_wait+kio mount while the backend fails file_open() should show it.

The asymmetry cannot be hidden inside a single helper that "releases ff
on error in both cases", because in one caller the VFS will release it
for you and in the other it will not. The shared helper should only do
the num_openers/attr/file_open work (and the num_openers-- on error),
and leave the reference cleanup to each caller:

  - fuse_open(): on error, fuse_release_common()/fuse_sync_release() as
    today (file is not FMODE_OPENED).
  - fuse_create_open(): on error, do nothing -- return err and let
    path_openat()'s fput() run fuse_release(), which releases ff (and
    drops the matching num_openers) exactly once.

i.e. for fuse_create_open() the correct error handling is simply to drop
the fput() and gate on !err, with no extra release at all:

	-	if (fm->fc->close_wait) {
	+	if (!err && fm->fc->close_wait) {
			struct fuse_inode *fi = get_fuse_inode(inode);
			bool need_open;

			inode_lock(inode);
			spin_lock(&fi->lock);
			need_open = (++fi->num_openers == 1);
			spin_unlock(&fi->lock);

	-		if (need_open && fm->fc->kio.op && fm->fc->kio.op->file_open) {
	-			err = fm->fc->kio.op->file_open(file, inode);
	-			if (err)
	-				fput(file);
	-		}
	+		if (need_open && fm->fc->kio.op && fm->fc->kio.op->file_open)
	+			err = fm->fc->kio.op->file_open(file, inode);
			inode_unlock(inode);
		}

I have this as a standalone commit (by code inspection only -- not yet
built or run); happy to post it as the immediate crash fix if you would
rather respin the dedup/align refactor on top of it.

Two more notes, independent of the bug above:

  - Factoring the duplicated block into one helper is a good cleanup,
    and pulling the atomic path's attribute handling (i_size_unstable /
    fuse_update_attributes / FMODE_NOWAIT) in line with fuse_open() is a
    reasonable thing to want. But that is an additional behavioural
    change to fuse_create_open() (it did not call fuse_update_attributes
    before) and should be called out as such in the changelog, not
    folded silently into a bugfix.

Thanks,
Den