Re: [PATCH] cifs: Fix unbuffered read

[Steve French]

Updated to add Paulo's Acked-by and also attached the other fix. Let
me know if any additional feedback/review/testing results

    cifs: Reapply lost fix from commit 30b2b2196d6e

    Reapply the fix from
       30b2b2196d6e ("cifs: do not include page data when checking signature")
    that got lost in the iteratorisation of the cifs driver.

On Tue, Apr 18, 2023 at 5:40 PM David Howells <dhowells@xxxxxxxxxx> wrote:
>
>
> If read() is done in an unbuffered manner, such that, say,
> cifs_strict_readv() goes through cifs_user_readv() and thence
> __cifs_readv(), it doesn't recognise the EOF and keeps indicating to
> userspace that it returning full buffers of data.
>
> This is due to ctx->iter being advanced in cifs_send_async_read() as the
> buffer is split up amongst a number of rdata objects.  The iterator count
> is then used in collect_uncached_read_data() in the non-DIO case to set the
> total length read - and thus the return value of sys_read().  But since the
> iterator normally gets used up completely during splitting, ctx->total_len
> gets overridden to the full amount.
>
> However, prior to that in collect_uncached_read_data(), we've gone through
> the list of rdatas and added up the amount of data we actually received
> (which we then throw away).
>
> Fix this by removing the bit that overrides the amount read in the non-DIO
> case and just going with the total added up in the aforementioned loop.
>
> This was observed by mounting a cifs share with multiple channels, e.g.:
>
>         mount //192.168.6.1/test /test/ -o user=shares,pass=...,max_channels=6
>
> and then reading a 1MiB file on the share:
>
>         strace cat /xfstest.test/1M  >/dev/null
>
> Through strace, the same data can be seen being read again and again.
>
> Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Steve French <smfrench@xxxxxxxxx>
> cc: Paulo Alcantara <pc@xxxxxxxxxxxxx>
> cc: Jérôme Glisse <jglisse@xxxxxxxxxx>
> cc: Long Li <longli@xxxxxxxxxxxxx>
> cc: Enzo Matsumiya <ematsumiya@xxxxxxx>
> cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
> cc: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
> cc: Jeff Layton <jlayton@xxxxxxxxxx>
> cc: linux-cifs@xxxxxxxxxxxxxxx
> ---
>  fs/cifs/file.c |    4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 321f9b7c84c9..f8877dc91cc5 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -4010,7 +4010,6 @@ static void
>  collect_uncached_read_data(struct cifs_aio_ctx *ctx)
>  {
>         struct cifs_readdata *rdata, *tmp;
> -       struct iov_iter *to = &ctx->iter;
>         struct cifs_sb_info *cifs_sb;
>         int rc;
>
> @@ -4076,9 +4075,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
>                 kref_put(&rdata->refcount, cifs_readdata_release);
>         }
>
> -       if (!ctx->direct_io)
> -               ctx->total_len = ctx->len - iov_iter_count(to);
> -
>         /* mask nodata case */
>         if (rc == -ENODATA)
>                 rc = 0;
>


-- 
Thanks,

Steve
From ac13692844f2fb23c503066c0cb231243218a7c8 Mon Sep 17 00:00:00 2001
From: David Howells <dhowells@xxxxxxxxxx>
Date: Tue, 18 Apr 2023 23:40:07 +0100
Subject: [PATCH 2/3] cifs: Fix unbuffered read
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If read() is done in an unbuffered manner, such that, say,
cifs_strict_readv() goes through cifs_user_readv() and thence
__cifs_readv(), it doesn't recognise the EOF and keeps indicating to
userspace that it returning full buffers of data.

This is due to ctx->iter being advanced in cifs_send_async_read() as the
buffer is split up amongst a number of rdata objects.  The iterator count
is then used in collect_uncached_read_data() in the non-DIO case to set the
total length read - and thus the return value of sys_read().  But since the
iterator normally gets used up completely during splitting, ctx->total_len
gets overridden to the full amount.

However, prior to that in collect_uncached_read_data(), we've gone through
the list of rdatas and added up the amount of data we actually received
(which we then throw away).

Fix this by removing the bit that overrides the amount read in the non-DIO
case and just going with the total added up in the aforementioned loop.

This was observed by mounting a cifs share with multiple channels, e.g.:

	mount //192.168.6.1/test /test/ -o user=shares,pass=...,max_channels=6

and then reading a 1MiB file on the share:

	strace cat /xfstest.test/1M  >/dev/null

Through strace, the same data can be seen being read again and again.

Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
Acked-by: Paulo Alcantara (SUSE) <pc@xxxxxxxxxxxxx>
cc: Jérôme Glisse <jglisse@xxxxxxxxxx>
cc: Long Li <longli@xxxxxxxxxxxxx>
cc: Enzo Matsumiya <ematsumiya@xxxxxxx>
cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
cc: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
cc: Jeff Layton <jlayton@xxxxxxxxxx>
cc: linux-cifs@xxxxxxxxxxxxxxx
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
 fs/cifs/file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6831a9949c43..b33d2e7b0f98 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4010,7 +4010,6 @@ static void
 collect_uncached_read_data(struct cifs_aio_ctx *ctx)
 {
 	struct cifs_readdata *rdata, *tmp;
-	struct iov_iter *to = &ctx->iter;
 	struct cifs_sb_info *cifs_sb;
 	int rc;
 
@@ -4076,9 +4075,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
 		kref_put(&rdata->refcount, cifs_readdata_release);
 	}
 
-	if (!ctx->direct_io)
-		ctx->total_len = ctx->len - iov_iter_count(to);
-
 	/* mask nodata case */
 	if (rc == -ENODATA)
 		rc = 0;
-- 
2.34.1

From 023fc150a39ffe656da3e459ad801eb1c7fdfad9 Mon Sep 17 00:00:00 2001
From: David Howells <dhowells@xxxxxxxxxx>
Date: Tue, 18 Apr 2023 23:49:12 +0100
Subject: [PATCH 3/3] cifs: Reapply lost fix from commit 30b2b2196d6e

Reapply the fix from:

   30b2b2196d6e ("cifs: do not include page data when checking signature")

that got lost in the iteratorisation of the cifs driver.

Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
Acked-by: Paulo Alcantara (SUSE) <pc@xxxxxxxxxxxxx>
Reported-by: Paulo Alcantara <pc@xxxxxxxxxxxxx>
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
cc: Paulo Alcantara <pc@xxxxxx>
cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
cc: Bharath S M <bharathsm@xxxxxxxxxxxxx>
cc: Enzo Matsumiya <ematsumiya@xxxxxxx>
cc: linux-cifs@xxxxxxxxxxxxxxx
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
 fs/cifs/smb2pdu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 4245249dbba8..366f0c3b799b 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -4180,10 +4180,12 @@ smb2_readv_callback(struct mid_q_entry *mid)
 	struct smb2_hdr *shdr =
 				(struct smb2_hdr *)rdata->iov[0].iov_base;
 	struct cifs_credits credits = { .value = 0, .instance = 0 };
-	struct smb_rqst rqst = { .rq_iov = &rdata->iov[1],
-				 .rq_nvec = 1,
-				 .rq_iter = rdata->iter,
-				 .rq_iter_size = iov_iter_count(&rdata->iter), };
+	struct smb_rqst rqst = { .rq_iov = &rdata->iov[1], .rq_nvec = 1 };
+
+	if (rdata->got_bytes) {
+		rqst.rq_iter	  = rdata->iter;
+		rqst.rq_iter_size = iov_iter_count(&rdata->iter);
+	};
 
 	WARN_ONCE(rdata->server != mid->server,
 		  "rdata server %p != mid server %p",
-- 
2.34.1