[PATCH 0/2] netfs: Miscellaneous fixes

[PATCH 0/2] netfs: Miscellaneous fixes

[David Howells]

Hi Christian,

Here are a couple of fixes for netfslib:

 (1) Fix an i_dio_count leak on a DIO read starting beyond EOF.

 (2) Fix a missing zero-length check in an unbuffered write causing 9p to
     indicate EIO.

The patches can also be found here:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=netfs-fixes

Thanks,
David

David Howells (1):
  netfs: Fix missing zero-length check in unbuffered write

Marc Dionne (1):
  netfs: Fix i_dio_count leak on DIO read past i_size

 fs/netfs/buffered_write.c | 3 +++
 fs/netfs/direct_write.c   | 5 ++++-
 fs/netfs/io.c             | 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

[PATCH 1/2] netfs: Fix i_dio_count leak on DIO read past i_size

[David Howells]

From: Marc Dionne <marc.dionne@auristor.com>

If netfs_begin_read gets a NETFS_DIO_READ request that begins
past i_size, it won't perform any i/o and just return 0.  This
will leak an increment to i_dio_count that is done at the top
of the function.

This can cause subsequent buffered read requests to block
indefinitely, waiting for a non existing dio operation to complete.

Add a inode_dio_end() for the NETFS_DIO_READ case, before returning.

Signed-off-by: Marc Dionne <marc.dionne@auristor.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: linux-afs@lists.infradead.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
 fs/netfs/io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/netfs/io.c b/fs/netfs/io.c
index e8ff1e61ce79..4261ad6c55b6 100644
--- a/fs/netfs/io.c
+++ b/fs/netfs/io.c
@@ -748,6 +748,8 @@ int netfs_begin_read(struct netfs_io_request *rreq, bool sync)
 
 	if (!rreq->submitted) {
 		netfs_put_request(rreq, false, netfs_rreq_trace_put_no_submit);
+		if (rreq->origin == NETFS_DIO_READ)
+			inode_dio_end(rreq->inode);
 		ret = 0;
 		goto out;
 	}

[PATCH 2/2] netfs: Fix missing zero-length check in unbuffered write

[David Howells]

Fix netfs_unbuffered_write_iter() to return immediately if
generic_write_checks() returns 0, indicating there's nothing to write.
Note that netfs_file_write_iter() already does this.

Also, whilst we're at it, put in checks for the size being zero before we
even take the locks.  Note that generic_write_checks() can still reduce the
size to zero, so we still need that check.

Without this, a warning similar to the following is logged to dmesg:

	netfs: Zero-sized write [R=1b6da]

and the syscall fails with EIO, e.g.:

	/sbin/ldconfig.real: Writing of cache extension data failed: Input/output error

This can be reproduced on 9p by:

	xfs_io -f -c 'pwrite 0 0' /xfstest.test/foo

Fixes: 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
Reported-by: Eric Van Hensbergen <ericvh@kernel.org>
Link: https://lore.kernel.org/r/ZbQUU6QKmIftKsmo@FV7GG9FTHL/
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Dominique Martinet <asmadeus@codewreck.org>
cc: Jeff Layton <jlayton@kernel.org>
cc: v9fs@lists.linux.dev
cc: linux_oss@crudebyte.com
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
 fs/netfs/buffered_write.c | 3 +++
 fs/netfs/direct_write.c   | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c
index a3059b3168fd..9a0d32e4b422 100644
--- a/fs/netfs/buffered_write.c
+++ b/fs/netfs/buffered_write.c
@@ -477,6 +477,9 @@ ssize_t netfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	_enter("%llx,%zx,%llx", iocb->ki_pos, iov_iter_count(from), i_size_read(inode));
 
+	if (!iov_iter_count(from))
+		return 0;
+
 	if ((iocb->ki_flags & IOCB_DIRECT) ||
 	    test_bit(NETFS_ICTX_UNBUFFERED, &ictx->flags))
 		return netfs_unbuffered_write_iter(iocb, from);
diff --git a/fs/netfs/direct_write.c b/fs/netfs/direct_write.c
index 60a40d293c87..bee047e20f5d 100644
--- a/fs/netfs/direct_write.c
+++ b/fs/netfs/direct_write.c
@@ -139,6 +139,9 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	_enter("%llx,%zx,%llx", iocb->ki_pos, iov_iter_count(from), i_size_read(inode));
 
+	if (!iov_iter_count(from))
+		return 0;
+
 	trace_netfs_write_iter(iocb, from);
 	netfs_stat(&netfs_n_rh_dio_write);
 
@@ -146,7 +149,7 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (ret < 0)
 		return ret;
 	ret = generic_write_checks(iocb, from);
-	if (ret < 0)
+	if (ret <= 0)
 		goto out;
 	ret = file_remove_privs(file);
 	if (ret < 0)

Re: [PATCH 1/2] netfs: Fix i_dio_count leak on DIO read past i_size

[Jeff Layton]

On Mon, 2024-01-29 at 09:49 +0000, David Howells wrote:
> From: Marc Dionne <marc.dionne@auristor.com>
> 
> If netfs_begin_read gets a NETFS_DIO_READ request that begins
> past i_size, it won't perform any i/o and just return 0.  This
> will leak an increment to i_dio_count that is done at the top
> of the function.
> 
> This can cause subsequent buffered read requests to block
> indefinitely, waiting for a non existing dio operation to complete.
> 
> Add a inode_dio_end() for the NETFS_DIO_READ case, before returning.
> 
> Signed-off-by: Marc Dionne <marc.dionne@auristor.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: linux-afs@lists.infradead.org
> cc: netfs@lists.linux.dev
> cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/netfs/io.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> index e8ff1e61ce79..4261ad6c55b6 100644
> --- a/fs/netfs/io.c
> +++ b/fs/netfs/io.c
> @@ -748,6 +748,8 @@ int netfs_begin_read(struct netfs_io_request *rreq, bool sync)
>  
>  	if (!rreq->submitted) {
>  		netfs_put_request(rreq, false, netfs_rreq_trace_put_no_submit);
> +		if (rreq->origin == NETFS_DIO_READ)
> +			inode_dio_end(rreq->inode);
>  		ret = 0;
>  		goto out;
>  	}
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/2] netfs: Fix missing zero-length check in unbuffered write

[Jeff Layton]

On Mon, 2024-01-29 at 09:49 +0000, David Howells wrote:
> Fix netfs_unbuffered_write_iter() to return immediately if
> generic_write_checks() returns 0, indicating there's nothing to write.
> Note that netfs_file_write_iter() already does this.
> 
> Also, whilst we're at it, put in checks for the size being zero before we
> even take the locks.  Note that generic_write_checks() can still reduce the
> size to zero, so we still need that check.
> 
> Without this, a warning similar to the following is logged to dmesg:
> 
> 	netfs: Zero-sized write [R=1b6da]
> 
> and the syscall fails with EIO, e.g.:
> 
> 	/sbin/ldconfig.real: Writing of cache extension data failed: Input/output error
> 
> This can be reproduced on 9p by:
> 
> 	xfs_io -f -c 'pwrite 0 0' /xfstest.test/foo
> 
> Fixes: 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
> Reported-by: Eric Van Hensbergen <ericvh@kernel.org>
> Link: https://lore.kernel.org/r/ZbQUU6QKmIftKsmo@FV7GG9FTHL/
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Dominique Martinet <asmadeus@codewreck.org>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: v9fs@lists.linux.dev
> cc: linux_oss@crudebyte.com
> cc: netfs@lists.linux.dev
> cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/netfs/buffered_write.c | 3 +++
>  fs/netfs/direct_write.c   | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c
> index a3059b3168fd..9a0d32e4b422 100644
> --- a/fs/netfs/buffered_write.c
> +++ b/fs/netfs/buffered_write.c
> @@ -477,6 +477,9 @@ ssize_t netfs_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  
>  	_enter("%llx,%zx,%llx", iocb->ki_pos, iov_iter_count(from), i_size_read(inode));
>  
> +	if (!iov_iter_count(from))
> +		return 0;
> +
>  	if ((iocb->ki_flags & IOCB_DIRECT) ||
>  	    test_bit(NETFS_ICTX_UNBUFFERED, &ictx->flags))
>  		return netfs_unbuffered_write_iter(iocb, from);
> diff --git a/fs/netfs/direct_write.c b/fs/netfs/direct_write.c
> index 60a40d293c87..bee047e20f5d 100644
> --- a/fs/netfs/direct_write.c
> +++ b/fs/netfs/direct_write.c
> @@ -139,6 +139,9 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  
>  	_enter("%llx,%zx,%llx", iocb->ki_pos, iov_iter_count(from), i_size_read(inode));
>  
> +	if (!iov_iter_count(from))
> +		return 0;
> +
>  	trace_netfs_write_iter(iocb, from);
>  	netfs_stat(&netfs_n_rh_dio_write);
>  
> @@ -146,7 +149,7 @@ ssize_t netfs_unbuffered_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	if (ret < 0)
>  		return ret;
>  	ret = generic_write_checks(iocb, from);
> -	if (ret < 0)
> +	if (ret <= 0)
>  		goto out;
>  	ret = file_remove_privs(file);
>  	if (ret < 0)
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/2] netfs: Miscellaneous fixes

[Christian Brauner]

On Mon, 29 Jan 2024 09:49:17 +0000, David Howells wrote:
> Here are a couple of fixes for netfslib:
> 
>  (1) Fix an i_dio_count leak on a DIO read starting beyond EOF.
> 
>  (2) Fix a missing zero-length check in an unbuffered write causing 9p to
>      indicate EIO.
> 
> [...]

Applied to the vfs.netfs branch of the vfs/vfs.git tree.
Patches in the vfs.netfs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.netfs

[1/2] netfs: Fix i_dio_count leak on DIO read past i_size
      https://git.kernel.org/vfs/vfs/c/a4bb694db189
[2/2] netfs: Fix missing zero-length check in unbuffered write
      https://git.kernel.org/vfs/vfs/c/2d6e065e2431

Re: [PATCH 2/2] netfs: Fix missing zero-length check in unbuffered write

[Dominique Martinet]

David Howells wrote on Mon, Jan 29, 2024 at 09:49:19AM +0000:
> Fix netfs_unbuffered_write_iter() to return immediately if
> generic_write_checks() returns 0, indicating there's nothing to write.
> Note that netfs_file_write_iter() already does this.
> 
> Also, whilst we're at it, put in checks for the size being zero before we
> even take the locks.  Note that generic_write_checks() can still reduce the
> size to zero, so we still need that check.
> 
> Without this, a warning similar to the following is logged to dmesg:
> 
> 	netfs: Zero-sized write [R=1b6da]
> 
> and the syscall fails with EIO, e.g.:
> 
> 	/sbin/ldconfig.real: Writing of cache extension data failed: Input/output error
> 
> This can be reproduced on 9p by:
> 
> 	xfs_io -f -c 'pwrite 0 0' /xfstest.test/foo
> 
> Fixes: 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
> Reported-by: Eric Van Hensbergen <ericvh@kernel.org>
> Link: https://lore.kernel.org/r/ZbQUU6QKmIftKsmo@FV7GG9FTHL/
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Dominique Martinet <asmadeus@codewreck.org>

Thanks!

Tested-by: Dominique Martinet <asmadeus@codewreck.org>

-- 
Dominique Martinet | Asmadeus

Re: [PATCH 2/2] netfs: Fix missing zero-length check in unbuffered write

[Linux regression tracking (Thorsten Leemhuis)]

On 29.01.24 10:49, David Howells wrote:
> Fix netfs_unbuffered_write_iter() to return immediately if
> generic_write_checks() returns 0, indicating there's nothing to write.
> Note that netfs_file_write_iter() already does this.
> 
> Also, whilst we're at it, put in checks for the size being zero before we
> even take the locks.  Note that generic_write_checks() can still reduce the
> size to zero, so we still need that check.
> 
> Without this, a warning similar to the following is logged to dmesg:
> 
> 	netfs: Zero-sized write [R=1b6da]
> 
> and the syscall fails with EIO, e.g.:
> 
> 	/sbin/ldconfig.real: Writing of cache extension data failed: Input/output error
> 
> This can be reproduced on 9p by:
> 
> 	xfs_io -f -c 'pwrite 0 0' /xfstest.test/foo
> 
> Fixes: 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
> Reported-by: Eric Van Hensbergen <ericvh@kernel.org>
> Link: https://lore.kernel.org/r/ZbQUU6QKmIftKsmo@FV7GG9FTHL/

David, thx for fixing Eric's regression, which I'm tracking.

Christian, just wondering: that patch afaics is sitting in vfs.netfs for
about three weeks now -- is that intentional or did it maybe fell
through the cracks somehow?

> [...]

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

Re: [PATCH 2/2] netfs: Fix missing zero-length check in unbuffered write

[Christian Brauner]

On Mon, Feb 19, 2024 at 09:38:33AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 29.01.24 10:49, David Howells wrote:
> > Fix netfs_unbuffered_write_iter() to return immediately if
> > generic_write_checks() returns 0, indicating there's nothing to write.
> > Note that netfs_file_write_iter() already does this.
> > 
> > Also, whilst we're at it, put in checks for the size being zero before we
> > even take the locks.  Note that generic_write_checks() can still reduce the
> > size to zero, so we still need that check.
> > 
> > Without this, a warning similar to the following is logged to dmesg:
> > 
> > 	netfs: Zero-sized write [R=1b6da]
> > 
> > and the syscall fails with EIO, e.g.:
> > 
> > 	/sbin/ldconfig.real: Writing of cache extension data failed: Input/output error
> > 
> > This can be reproduced on 9p by:
> > 
> > 	xfs_io -f -c 'pwrite 0 0' /xfstest.test/foo
> > 
> > Fixes: 153a9961b551 ("netfs: Implement unbuffered/DIO write support")
> > Reported-by: Eric Van Hensbergen <ericvh@kernel.org>
> > Link: https://lore.kernel.org/r/ZbQUU6QKmIftKsmo@FV7GG9FTHL/
> 
> David, thx for fixing Eric's regression, which I'm tracking.
> 
> Christian, just wondering: that patch afaics is sitting in vfs.netfs for
> about three weeks now -- is that intentional or did it maybe fell
> through the cracks somehow?

I've moved it to vfs.fixes now and will send later this week. Thanks for
the reminder!