Unverified Commit ccde2ac7 authored by David Howells's avatar David Howells Committed by Christian Brauner
Browse files

netfs: Fix folio->private handling in netfs_perform_write() 

Under some circumstances, netfs_perform_write() doesn't correctly
manipulate folio->private between NULL, NETFS_FOLIO_COPY_TO_CACHE, pointing
to a group and pointing to a netfs_folio struct, leading to potential
multiple attachments of private data with associated folio ref leaks and
also leaks of netfs_folio structs or netfs_group refs.

Fix this by consolidating the place at which a folio is marked uptodate in
one place and having that look at what's attached to folio->private and
decide how to clean it up and then set the new group.  Also, the content
shouldn't be flushed if group is NULL, even if a group is specified in the
netfs_group parameter, as that would be the case for a new folio.  A
filesystem should always specify netfs_group or never specify netfs_group.

The Sashiko auto-review tool noted that it was theoretically possible that
the fpos >= ctx->zero_point section might leak if it modified a streaming
write folio.  This is unlikely, but with a network filesystem, third party
changes can happen.  It also pointed out that __netfs_set_group() would
leak if called multiple times on the same folio from the "whole folio
modify section".

Fixes: 8f52de00 ("netfs: Reduce number of conditional branches in netfs_perform_write()")
Closes: https://sashiko.dev/#/patchset/20260414082004.3756080-1-dhowells%40redhat.com


Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
Link: https://patch.msgid.link/20260512123404.719402-22-dhowells@redhat.com


cc: Paulo Alcantara <pc@manguebit.org>
cc: Matthew Wilcox <willy@infradead.org>
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
Signed-off-by: default avatarChristian Brauner <brauner@kernel.org>
parent 6d91acc7

fs/netfs/buffered_write.c

+81 −53
Original line number Diff line number Diff line
@@ -12,24 +12,6 @@ 
#include <linux/slab.h>

#include "internal.h"



static void __netfs_set_group(struct folio *folio, struct netfs_group *netfs_group)

{

	if (netfs_group)

		folio_attach_private(folio, netfs_get_group(netfs_group));

}



static void netfs_set_group(struct folio *folio, struct netfs_group *netfs_group)

{

	void *priv = folio_get_private(folio);



	if (unlikely(priv != netfs_group)) {

		if (netfs_group && (!priv || priv == NETFS_FOLIO_COPY_TO_CACHE))

			folio_attach_private(folio, netfs_get_group(netfs_group));

		else if (!netfs_group && priv == NETFS_FOLIO_COPY_TO_CACHE)

			folio_detach_private(folio);

	}

}



/*

 * Grab a folio for writing and lock it.  Attempt to allocate as large a folio

 * as possible to hold as much of the remaining length as possible in one go.
@@ -157,6 +139,7 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
		size_t offset;	/* Offset into pagecache folio */

		size_t part;	/* Bytes to write to folio */

		size_t copied;	/* Bytes copied from user */

		void *priv;



		offset = pos & (max_chunk - 1);

		part = min(max_chunk - offset, iov_iter_count(iter));
@@ -202,6 +185,25 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			goto error_folio_unlock;

		}



		finfo = netfs_folio_info(folio);

		group = netfs_folio_group(folio);



		/* If the requested group differs from the group set on the

		 * page, then we need to flush out the folio if it has a group

		 * set (ie. is non-NULL).  Note that COPY_TO_CACHE is a special

		 * case, being a netfs annotation rather than an actual group.

		 *

		 * The filesystem isn't permitted to mix writes with groups and

		 * writes without groups as the NULL group is used to indicate

		 * that no group is set.

		 */

		if (unlikely(group != netfs_group) &&

		    group != NETFS_FOLIO_COPY_TO_CACHE &&

		    group) {

			WARN_ON_ONCE(!netfs_group);

			goto flush_content;

		}



		/* Decide how we should modify a folio.  We might be attempting

		 * to do write-streaming, as we don't want to a local RMW cycle

		 * if we can avoid it.  If we're doing local caching or content
@@ -209,22 +211,14 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
		 * file is open readably, then we let ->read_folio() fill in

		 * the gaps.

		 */

		finfo = netfs_folio_info(folio);

		group = netfs_folio_group(folio);



		if (unlikely(group != netfs_group) &&

		    group != NETFS_FOLIO_COPY_TO_CACHE)

			goto flush_content;



		if (folio_test_uptodate(folio)) {

			if (mapping_writably_mapped(mapping))

				flush_dcache_folio(folio);

			copied = copy_folio_from_iter_atomic(folio, offset, part, iter);

			if (unlikely(copied == 0))

				goto copy_failed;

			netfs_set_group(folio, netfs_group);

			trace = netfs_folio_is_uptodate;

			goto copied;

			goto copied_uptodate;

		}



		/* If the page is above the zero-point then we assume that the
@@ -237,24 +231,22 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			if (unlikely(copied == 0))

				goto copy_failed;

			folio_zero_segment(folio, offset + copied, flen);

			__netfs_set_group(folio, netfs_group);

			folio_mark_uptodate(folio);

			if (finfo)

				trace = netfs_modify_and_clear_rm_finfo;

			else

				trace = netfs_modify_and_clear;

			goto copied;

			goto mark_uptodate;

		}



		/* See if we can write a whole folio in one go. */

		if (!maybe_trouble && offset == 0 && part >= flen) {

			copied = copy_folio_from_iter_atomic(folio, offset, part, iter);

			if (likely(copied == part)) {

				if (finfo) {

				if (finfo)

					trace = netfs_whole_folio_modify_filled;

					goto folio_now_filled;

				}

				__netfs_set_group(folio, netfs_group);

				folio_mark_uptodate(folio);

				else

					trace = netfs_whole_folio_modify;

				goto copied;

				goto mark_uptodate;

			}

			if (copied == 0)

				goto copy_failed;
@@ -272,7 +264,7 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			finfo->dirty_len += finfo->dirty_offset;

			if (finfo->dirty_len == flen) {

				trace = netfs_whole_folio_modify_filled_efault;

				goto folio_now_filled;

				goto mark_uptodate;

			}

			if (copied > finfo->dirty_len)

				finfo->dirty_len = copied;
@@ -300,11 +292,11 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			copied = copy_folio_from_iter_atomic(folio, offset, part, iter);

			if (unlikely(copied == 0))

				goto copy_failed;

			netfs_set_group(folio, netfs_group);

			trace = netfs_just_prefetch;

			goto copied;

			goto copied_uptodate;

		}



		/* Do a streaming write on a folio that has nothing in it yet. */

		if (!finfo) {

			ret = -EIO;

			if (WARN_ON(folio_get_private(folio)))
@@ -313,10 +305,8 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			if (unlikely(copied == 0))

				goto copy_failed;

			if (offset == 0 && copied == flen) {

				__netfs_set_group(folio, netfs_group);

				folio_mark_uptodate(folio);

				trace = netfs_streaming_filled_page;

				goto copied;

				goto mark_uptodate;

			}



			finfo = kzalloc_obj(*finfo);
@@ -345,7 +335,7 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			finfo->dirty_len += copied;

			if (finfo->dirty_offset == 0 && finfo->dirty_len == flen) {

				trace = netfs_streaming_cont_filled_page;

				goto folio_now_filled;

				goto mark_uptodate;

			}

			trace = netfs_streaming_write_cont;

			goto copied;
@@ -361,13 +351,36 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, 
			goto out;

		continue;



	folio_now_filled:

		if (finfo->netfs_group)

			folio_change_private(folio, finfo->netfs_group);

		/* Mark a folio as being up to data when we've filled it

		 * completely.  If the folio has a group attached, then it must

		 * be the same group, otherwise we should have flushed it out

		 * above.  We have to get rid of the netfs_folio struct if

		 * there was one.

		 */

	mark_uptodate:

		folio_mark_uptodate(folio);



	copied_uptodate:

		priv = folio_get_private(folio);

		if (likely(priv == netfs_group)) {

			/* Already set correctly; no change required. */

		} else if (priv == NETFS_FOLIO_COPY_TO_CACHE) {

			if (!netfs_group)

				folio_detach_private(folio);

			else

				folio_change_private(folio, netfs_get_group(netfs_group));

		} else if (!priv) {

			folio_attach_private(folio, netfs_get_group(netfs_group));

		} else {

			WARN_ON_ONCE(!finfo);

			if (netfs_group)

				/* finfo->netfs_group has a ref */

				folio_change_private(folio, netfs_group);

			else

				folio_detach_private(folio);

		folio_mark_uptodate(folio);

			kfree(finfo);

		}



	copied:

		trace_netfs_folio(folio, trace);

		flush_dcache_folio(folio);
@@ -530,6 +543,7 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr 
	struct inode *inode = file_inode(file);

	struct netfs_inode *ictx = netfs_inode(inode);

	vm_fault_t ret = VM_FAULT_NOPAGE;

	void *priv;

	int err;



	_enter("%lx", folio->index);
@@ -550,7 +564,9 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr 
	}



	group = netfs_folio_group(folio);

	if (group != netfs_group && group != NETFS_FOLIO_COPY_TO_CACHE) {

	if (group &&

	    group != netfs_group &&

	    group != NETFS_FOLIO_COPY_TO_CACHE) {

		folio_unlock(folio);

		err = filemap_fdatawrite_range(mapping,

					       folio_pos(folio),
@@ -572,7 +588,19 @@ vm_fault_t netfs_page_mkwrite(struct vm_fault *vmf, struct netfs_group *netfs_gr 
		trace_netfs_folio(folio, netfs_folio_trace_mkwrite_plus);

	else

		trace_netfs_folio(folio, netfs_folio_trace_mkwrite);

	netfs_set_group(folio, netfs_group);



	priv = folio_get_private(folio);

	if (priv != netfs_group) {

		if (!netfs_group && priv == NETFS_FOLIO_COPY_TO_CACHE)

			folio_detach_private(folio);

		else if (netfs_group && priv == NETFS_FOLIO_COPY_TO_CACHE)

			folio_change_private(folio, netfs_get_group(netfs_group));

		else if (netfs_group && !priv)

			folio_attach_private(folio, netfs_get_group(netfs_group));

		else

			WARN_ON_ONCE(1);

	}



	file_update_time(file);

	set_bit(NETFS_ICTX_MODIFIED_ATTR, &ictx->flags);

	if (ictx->ops->post_modify)

include/trace/events/netfs.h

+1 −0
Original line number Diff line number Diff line
@@ -181,6 +181,7 @@ 
	EM(netfs_whole_folio_modify_filled,	"mod-whole-f+")	\

	EM(netfs_whole_folio_modify_filled_efault, "mod-whole-f+!") \

	EM(netfs_modify_and_clear,		"mod-n-clear")	\

	EM(netfs_modify_and_clear_rm_finfo,	"mod-n-clear+")	\

	EM(netfs_streaming_write,		"mod-streamw")	\

	EM(netfs_streaming_write_cont,		"mod-streamw+")	\

	EM(netfs_flush_content,			"flush")	\