[Devel] [PATCH hci-8.0] fs/fuse: fix writectr imbalance when failing writeback of invalidated file

[Konstantin Khorenko]

> The shoot-on-sight path in fuse_writepages_send() for files being
> invalidated violates the writectr protocol: the increment is not
> gated on writectr >=0, and the matching decrement lives in a separate
> fi->lock critical section. A concurrent fuse_set_nowrite() and
> fuse_release_nowrite() pair lands in that window observes writectr ==
> FUSE_NOWRITE + 1 and triggers BUG_ON(fi->writectr != FUSE_NOWRITE) in
> __fuse_release_nowrite():
> 
>   kernel BUG at fs/fuse/dir.c:1842!
>   RIP: fuse_release_nowrite+0x45/0x50 [fuse]
>    fuse_release+0xd0/0x300 [fuse]
>    __fput+0xe1/0x2a0
>    __x64_sys_close+0x3c/0x80
> 
> Fix it by always routing writeback through fi->queued_writes so the
> writectr gating applies, and failing requests on invalidated files
> in fuse_send_writepage() instead, where the existing out_free path
> keeps the writectr increment and decrement balanced within a single
> fi->lock critical section. Note that if a NOWRITE section is active,
> queue the request instead of failing it immediately is harmless for
> the file invalidation operation.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-145614

The ungated fi->writectr++ under FUSE_S_FAIL_IMMEDIATELY was added to
fuse_writepages_send() by 2d23aac3630e, so:

Fixes: 2d23aac3630e ("fuse: fix writeback/release race")

?
> 
> Signed-off-by: Liu Kui <kui.liu at virtuozzo.com>
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index c96a631b95292..180f60fc539a5 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2237,6 +2237,11 @@ __acquires(fi->lock)
>  	int err;
>  
>  	fi->writectr++;
> +	if (fuse_file_fail_immediately(wpa->ia.ff)) {
> +		/* File is under invalidation: fail request without sending */
> +		mapping_set_error(wpa->inode->i_mapping, -EIO);
> +		goto out_free;
> +	}

[Severity: Medium]
Does the out_free path drop the fuse_file references held by the auxiliary
requests on a close_wait mount?

Every wpa created in fuse_writepages_fill() takes its own reference:

fs/fuse/file.c:fuse_writepages_fill() {
    ...
		wpa = fuse_writepage_args_setup(folio, data->ff);
		...
		fuse_file_get(wpa->ia.ff);
    ...
}

and when fuse_writepage_add() finds the page already under writeback the
new wpa is chained onto old_wpa->next and keeps that reference. The two
release points are split by fc->close_wait:

fs/fuse/file.c:fuse_writepage_finish() {
    ...
	if (wpa->ia.ff && fc->close_wait)
		__fuse_file_put(wpa->ia.ff);
    ...
}

fs/fuse/file.c:fuse_writepage_free() {
    ...
	if (wpa->ia.ff && !fc->close_wait)
		fuse_file_put(wpa->ia.ff, false);
    ...
}

The out_free path this change now jumps to calls fuse_writepage_finish()
for the primary only. The auxiliary requests get fuse_writepage_finish_stat()
and fuse_writepage_free():

fs/fuse/file.c:fuse_send_writepage() {
    ...
 out_free:
	fi->writectr--;
	rb_erase(&wpa->writepages_entry, &fi->writepages);
	fuse_writepage_finish(wpa);
	spin_unlock(&fi->lock);

	/* After rb_erase() aux request list is private */
	for (aux = wpa->next; aux; aux = next) {
		next = aux->next;
		aux->next = NULL;
		fuse_writepage_finish_stat(aux->inode, aux->ia.ap.pages[0]);
		fuse_writepage_free(aux);
	}
    ...
}

With fc->close_wait set, neither call puts aux->ia.ff, so each auxiliary
request leaves one reference on the fuse_file behind.

Before this change a request for a FUSE_S_FAIL_IMMEDIATELY file never sat in
fi->queued_writes, so an auxiliary request could only attach to a primary
that was already in flight, and that primary was completed through
fuse_writepage_end(), which resends each aux with fuse_send_writepage() and
so ends up in fuse_writepage_finish() for it.

Now the request waits in fi->queued_writes while fuse_set_nowrite() is
active (truncate, fsync, or open with truncate on another file). If the page
is redirtied in that window, which fuse_writepage_need_send() documents as
possible for a direct read into an mmapped page, the next writeback pass
chains an aux onto the queued primary. When __fuse_release_nowrite() finally
sends it, this new check takes out_free and the aux references are lost.

On a close_wait mount that reference is what fuse_release() waits for:

fs/fuse/file.c:fuse_release() {
    ...
		if (fc->close_wait)
			wait_event(fi->page_waitq, refcount_read(&ff->count) == 1);
    ...
}

There is no timeout and nothing else drops the count, so close() on that
file would never return.

The same gap already exists for the "Got truncated off completely" and
broken-connection exits of out_free, so this is not new code, but this
change makes out_free the regular completion path for every invalidated
file. Should the aux loop in out_free call fuse_writepage_finish() (or
otherwise do the close_wait __fuse_file_put()) for each aux, either in this
patch or in a fix preceding it?

>  	if (inarg->offset + data_size <= size) {
>  		inarg->size = data_size;
>  	} else if (inarg->offset < size) {

[ ... ]

-- 
Konstantin Khorenko <khorenko at virtuozzo.com>