* [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
@ 2025-06-23 13:04 David Howells
2025-06-24 12:25 ` Stefan Metzmacher
0 siblings, 1 reply; 11+ messages in thread
From: David Howells @ 2025-06-23 13:04 UTC (permalink / raw)
To: Tom Talpey
Cc: dhowells, Steve French, Steve French, Stefan Metzmacher,
Paulo Alcantara, Matthew Wilcox, CIFS
Collapse smbd_recv_buf() and smbd_recv_page() into smbd_recv() and just use
copy_to_iter() instead of memcpy().
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <stfrench@microsoft.com>
cc: Tom Talpey <tom@talpey.com>
cc: Stefan Metzmacher <metze@samba.org>
cc: Paulo Alcantara (Red Hat) <pc@manguebit.com>
cc: Matthew Wilcox <willy@infradead.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
fs/smb/client/smbdirect.c | 116 +++++++---------------------------------------
1 file changed, 20 insertions(+), 96 deletions(-)
diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index 5ae847919da5..dc64c337aae0 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -1747,35 +1747,39 @@ struct smbd_connection *smbd_get_connection(
}
/*
- * Receive data from receive reassembly queue
+ * Receive data from the transport's receive reassembly queue
* All the incoming data packets are placed in reassembly queue
- * buf: the buffer to read data into
+ * iter: the buffer to read data into
* size: the length of data to read
* return value: actual data read
- * Note: this implementation copies the data from reassebmly queue to receive
+ *
+ * Note: this implementation copies the data from reassembly queue to receive
* buffers used by upper layer. This is not the optimal code path. A better way
* to do it is to not have upper layer allocate its receive buffers but rather
* borrow the buffer from reassembly queue, and return it after data is
* consumed. But this will require more changes to upper layer code, and also
* need to consider packet boundaries while they still being reassembled.
*/
-static int smbd_recv_buf(struct smbd_connection *info, char *buf,
- unsigned int size)
+int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
{
struct smbdirect_socket *sc = &info->socket;
struct smbd_response *response;
struct smbdirect_data_transfer *data_transfer;
+ size_t size = msg->msg_iter.count;
int to_copy, to_read, data_read, offset;
u32 data_length, remaining_data_length, data_offset;
int rc;
+ if (WARN_ON_ONCE(iov_iter_rw(&msg->msg_iter) == WRITE))
+ return -EINVAL; /* It's a bug in upper layer to get there */
+
again:
/*
* No need to hold the reassembly queue lock all the time as we are
* the only one reading from the front of the queue. The transport
* may add more entries to the back of the queue at the same time
*/
- log_read(INFO, "size=%d info->reassembly_data_length=%d\n", size,
+ log_read(INFO, "size=%zd info->reassembly_data_length=%d\n", size,
info->reassembly_data_length);
if (info->reassembly_data_length >= size) {
int queue_length;
@@ -1811,9 +1815,12 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
* transport layer is added
*/
if (response->first_segment && size == 4) {
- unsigned int rfc1002_len =
+ unsigned int len =
data_length + remaining_data_length;
- *((__be32 *)buf) = cpu_to_be32(rfc1002_len);
+ __be32 rfc1002_len = cpu_to_be32(len);
+ if (copy_to_iter(&rfc1002_len, sizeof(rfc1002_len),
+ &msg->msg_iter) != sizeof(rfc1002_len))
+ return -EFAULT;
data_read = 4;
response->first_segment = false;
log_read(INFO, "returning rfc1002 length %d\n",
@@ -1822,10 +1829,9 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
}
to_copy = min_t(int, data_length - offset, to_read);
- memcpy(
- buf + data_read,
- (char *)data_transfer + data_offset + offset,
- to_copy);
+ if (copy_to_iter((char *)data_transfer + data_offset + offset,
+ to_copy, &msg->msg_iter) != to_copy)
+ return -EFAULT;
/* move on to the next buffer? */
if (to_copy == data_length - offset) {
@@ -1870,6 +1876,8 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
data_read, info->reassembly_data_length,
info->first_entry_offset);
read_rfc1002_done:
+ /* SMBDirect will read it all or nothing */
+ msg->msg_iter.count = 0;
return data_read;
}
@@ -1890,90 +1898,6 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
goto again;
}
-/*
- * Receive a page from receive reassembly queue
- * page: the page to read data into
- * to_read: the length of data to read
- * return value: actual data read
- */
-static int smbd_recv_page(struct smbd_connection *info,
- struct page *page, unsigned int page_offset,
- unsigned int to_read)
-{
- struct smbdirect_socket *sc = &info->socket;
- int ret;
- char *to_address;
- void *page_address;
-
- /* make sure we have the page ready for read */
- ret = wait_event_interruptible(
- info->wait_reassembly_queue,
- info->reassembly_data_length >= to_read ||
- sc->status != SMBDIRECT_SOCKET_CONNECTED);
- if (ret)
- return ret;
-
- /* now we can read from reassembly queue and not sleep */
- page_address = kmap_atomic(page);
- to_address = (char *) page_address + page_offset;
-
- log_read(INFO, "reading from page=%p address=%p to_read=%d\n",
- page, to_address, to_read);
-
- ret = smbd_recv_buf(info, to_address, to_read);
- kunmap_atomic(page_address);
-
- return ret;
-}
-
-/*
- * Receive data from transport
- * msg: a msghdr point to the buffer, can be ITER_KVEC or ITER_BVEC
- * return: total bytes read, or 0. SMB Direct will not do partial read.
- */
-int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
-{
- char *buf;
- struct page *page;
- unsigned int to_read, page_offset;
- int rc;
-
- if (iov_iter_rw(&msg->msg_iter) == WRITE) {
- /* It's a bug in upper layer to get there */
- cifs_dbg(VFS, "Invalid msg iter dir %u\n",
- iov_iter_rw(&msg->msg_iter));
- rc = -EINVAL;
- goto out;
- }
-
- switch (iov_iter_type(&msg->msg_iter)) {
- case ITER_KVEC:
- buf = msg->msg_iter.kvec->iov_base;
- to_read = msg->msg_iter.kvec->iov_len;
- rc = smbd_recv_buf(info, buf, to_read);
- break;
-
- case ITER_BVEC:
- page = msg->msg_iter.bvec->bv_page;
- page_offset = msg->msg_iter.bvec->bv_offset;
- to_read = msg->msg_iter.bvec->bv_len;
- rc = smbd_recv_page(info, page, page_offset, to_read);
- break;
-
- default:
- /* It's a bug in upper layer to get there */
- cifs_dbg(VFS, "Invalid msg type %d\n",
- iov_iter_type(&msg->msg_iter));
- rc = -EINVAL;
- }
-
-out:
- /* SMBDirect will read it all or nothing */
- if (rc > 0)
- msg->msg_iter.count = 0;
- return rc;
-}
-
/*
* Send data to transport
* Each rqst is transported as a SMBDirect payload
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-23 13:04 [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter() David Howells
@ 2025-06-24 12:25 ` Stefan Metzmacher
2025-06-24 14:22 ` David Howells
2025-06-25 14:18 ` Stefan Metzmacher
0 siblings, 2 replies; 11+ messages in thread
From: Stefan Metzmacher @ 2025-06-24 12:25 UTC (permalink / raw)
To: David Howells
Cc: linux-cifs@vger.kernel.org, netfs, linux-fsdevel, Steve French
Hi David,
this looks very useful! Just a few comments below...
> Collapse smbd_recv_buf() and smbd_recv_page() into smbd_recv() and just use
> copy_to_iter() instead of memcpy().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <stfrench@microsoft.com>
> cc: Tom Talpey <tom@talpey.com>
> cc: Stefan Metzmacher <metze@samba.org>
> cc: Paulo Alcantara (Red Hat) <pc@manguebit.com>
> cc: Matthew Wilcox <willy@infradead.org>
> cc: linux-cifs@vger.kernel.org
> cc: netfs@lists.linux.dev
> cc: linux-fsdevel@vger.kernel.org
> ---
> fs/smb/client/smbdirect.c | 116 +++++++---------------------------------------
> 1 file changed, 20 insertions(+), 96 deletions(-)
>
> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
> index 5ae847919da5..dc64c337aae0 100644
> --- a/fs/smb/client/smbdirect.c
> +++ b/fs/smb/client/smbdirect.c
> @@ -1747,35 +1747,39 @@ struct smbd_connection *smbd_get_connection(
> }
>
> /*
> - * Receive data from receive reassembly queue
> + * Receive data from the transport's receive reassembly queue
> * All the incoming data packets are placed in reassembly queue
> - * buf: the buffer to read data into
> + * iter: the buffer to read data into
> * size: the length of data to read
> * return value: actual data read
> - * Note: this implementation copies the data from reassebmly queue to receive
> + *
> + * Note: this implementation copies the data from reassembly queue to receive
> * buffers used by upper layer. This is not the optimal code path. A better way
> * to do it is to not have upper layer allocate its receive buffers but rather
> * borrow the buffer from reassembly queue, and return it after data is
> * consumed. But this will require more changes to upper layer code, and also
> * need to consider packet boundaries while they still being reassembled.
> */
> -static int smbd_recv_buf(struct smbd_connection *info, char *buf,
> - unsigned int size)
> +int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
> {
> struct smbdirect_socket *sc = &info->socket;
> struct smbd_response *response;
> struct smbdirect_data_transfer *data_transfer;
> + size_t size = msg->msg_iter.count;
I think this should be iov_iter_count()?
> int to_copy, to_read, data_read, offset;
> u32 data_length, remaining_data_length, data_offset;
> int rc;
>
> + if (WARN_ON_ONCE(iov_iter_rw(&msg->msg_iter) == WRITE))
> + return -EINVAL; /* It's a bug in upper layer to get there */
> +
> again:
> /*
> * No need to hold the reassembly queue lock all the time as we are
> * the only one reading from the front of the queue. The transport
> * may add more entries to the back of the queue at the same time
> */
> - log_read(INFO, "size=%d info->reassembly_data_length=%d\n", size,
> + log_read(INFO, "size=%zd info->reassembly_data_length=%d\n", size,
> info->reassembly_data_length);
> if (info->reassembly_data_length >= size) {
> int queue_length;
> @@ -1811,9 +1815,12 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
> * transport layer is added
> */
> if (response->first_segment && size == 4) {
> - unsigned int rfc1002_len =
> + unsigned int len =
> data_length + remaining_data_length;
> - *((__be32 *)buf) = cpu_to_be32(rfc1002_len);
> + __be32 rfc1002_len = cpu_to_be32(len);
> + if (copy_to_iter(&rfc1002_len, sizeof(rfc1002_len),
> + &msg->msg_iter) != sizeof(rfc1002_len))
> + return -EFAULT;
> data_read = 4;
> response->first_segment = false;
> log_read(INFO, "returning rfc1002 length %d\n",
> @@ -1822,10 +1829,9 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
> }
>
> to_copy = min_t(int, data_length - offset, to_read);
> - memcpy(
> - buf + data_read,
> - (char *)data_transfer + data_offset + offset,
> - to_copy);
> + if (copy_to_iter((char *)data_transfer + data_offset + offset,
> + to_copy, &msg->msg_iter) != to_copy)
> + return -EFAULT;
>
> /* move on to the next buffer? */
> if (to_copy == data_length - offset) {
> @@ -1870,6 +1876,8 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
> data_read, info->reassembly_data_length,
> info->first_entry_offset);
> read_rfc1002_done:
> + /* SMBDirect will read it all or nothing */
> + msg->msg_iter.count = 0;
And this iov_iter_truncate(0);
While I'm wondering why we had this at all.
It seems all callers of cifs_read_iter_from_socket()
don't care and the code path via sock_recvmsg() doesn't
truncate it just calls copy_to_iter() via this chain:
->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
->simple_copy_to_iter->copy_to_iter()
I think the old code should have called
iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
But the new code doesn't need it as copy_to_iter()
calls iterate_and_advance().
> return data_read;
> }
>
> @@ -1890,90 +1898,6 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
> goto again;
> }
>
> -/*
> - * Receive a page from receive reassembly queue
> - * page: the page to read data into
> - * to_read: the length of data to read
> - * return value: actual data read
> - */
> -static int smbd_recv_page(struct smbd_connection *info,
> - struct page *page, unsigned int page_offset,
> - unsigned int to_read)
> -{
> - struct smbdirect_socket *sc = &info->socket;
> - int ret;
> - char *to_address;
> - void *page_address;
> -
> - /* make sure we have the page ready for read */
> - ret = wait_event_interruptible(
> - info->wait_reassembly_queue,
> - info->reassembly_data_length >= to_read ||
> - sc->status != SMBDIRECT_SOCKET_CONNECTED);
> - if (ret)
> - return ret;
> -
> - /* now we can read from reassembly queue and not sleep */
> - page_address = kmap_atomic(page);
> - to_address = (char *) page_address + page_offset;
> -
> - log_read(INFO, "reading from page=%p address=%p to_read=%d\n",
> - page, to_address, to_read);
> -
> - ret = smbd_recv_buf(info, to_address, to_read);
> - kunmap_atomic(page_address);
> -
> - return ret;
> -}
> -
> -/*
> - * Receive data from transport
> - * msg: a msghdr point to the buffer, can be ITER_KVEC or ITER_BVEC
> - * return: total bytes read, or 0. SMB Direct will not do partial read.
> - */
> -int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
> -{
> - char *buf;
> - struct page *page;
> - unsigned int to_read, page_offset;
> - int rc;
> -
> - if (iov_iter_rw(&msg->msg_iter) == WRITE) {
> - /* It's a bug in upper layer to get there */
> - cifs_dbg(VFS, "Invalid msg iter dir %u\n",
> - iov_iter_rw(&msg->msg_iter));
> - rc = -EINVAL;
> - goto out;
> - }
> -
> - switch (iov_iter_type(&msg->msg_iter)) {
> - case ITER_KVEC:
> - buf = msg->msg_iter.kvec->iov_base;
> - to_read = msg->msg_iter.kvec->iov_len;
> - rc = smbd_recv_buf(info, buf, to_read);
> - break;
> -
> - case ITER_BVEC:
> - page = msg->msg_iter.bvec->bv_page;
> - page_offset = msg->msg_iter.bvec->bv_offset;
> - to_read = msg->msg_iter.bvec->bv_len;
> - rc = smbd_recv_page(info, page, page_offset, to_read);
> - break;
> -
> - default:
> - /* It's a bug in upper layer to get there */
> - cifs_dbg(VFS, "Invalid msg type %d\n",
> - iov_iter_type(&msg->msg_iter));
> - rc = -EINVAL;
> - }
I guess this is actually a real fix as I just saw
CIFS: VFS: Invalid msg type 4
in logs while running the cifs/001 test.
And 4 is ITER_FOLIOQ.
So there might be something broken when ITER_FOLIOQ was
introduced, but I wasn't able to find a specific commit.
Maybe it was also already broken when using
smb3 encryption over smbdirect, when ITER_XARRAY was still used.
metze
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-24 12:25 ` Stefan Metzmacher
@ 2025-06-24 14:22 ` David Howells
2025-06-24 16:05 ` Stefan Metzmacher
` (2 more replies)
2025-06-25 14:18 ` Stefan Metzmacher
1 sibling, 3 replies; 11+ messages in thread
From: David Howells @ 2025-06-24 14:22 UTC (permalink / raw)
To: Stefan Metzmacher
Cc: dhowells, linux-cifs@vger.kernel.org, netfs, linux-fsdevel,
Steve French
Stefan Metzmacher <metze@samba.org> wrote:
> > read_rfc1002_done:
> > + /* SMBDirect will read it all or nothing */
> > + msg->msg_iter.count = 0;
>
> And this iov_iter_truncate(0);
Actually, it should probably have been iov_iter_advance().
> While I'm wondering why we had this at all.
>
> It seems all callers of cifs_read_iter_from_socket()
> don't care and the code path via sock_recvmsg() doesn't
> truncate it just calls copy_to_iter() via this chain:
> ->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
> ->simple_copy_to_iter->copy_to_iter()
>
> I think the old code should have called
> iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
>
> But the new code doesn't need it as copy_to_iter()
> calls iterate_and_advance().
Yeah, it should. I seem to remember that there were situations in which it
didn't, but it's possible I managed to get rid of them.
> > - default:
> > - /* It's a bug in upper layer to get there */
> > - cifs_dbg(VFS, "Invalid msg type %d\n",
> > - iov_iter_type(&msg->msg_iter));
> > - rc = -EINVAL;
> > - }
>
> I guess this is actually a real fix as I just saw
> CIFS: VFS: Invalid msg type 4
> in logs while running the cifs/001 test.
> And 4 is ITER_FOLIOQ.
Ah... Were you using "-o seal"? The encrypted data is held in a buffer formed
from a folioq with a series of folios in it.
David
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-24 14:22 ` David Howells
@ 2025-06-24 16:05 ` Stefan Metzmacher
2025-06-25 8:07 ` Stefan Metzmacher
2025-06-25 12:47 ` David Howells
2 siblings, 0 replies; 11+ messages in thread
From: Stefan Metzmacher @ 2025-06-24 16:05 UTC (permalink / raw)
To: David Howells
Cc: linux-cifs@vger.kernel.org, netfs, linux-fsdevel, Steve French
Hi David,
>>> read_rfc1002_done:
>>> + /* SMBDirect will read it all or nothing */
>>> + msg->msg_iter.count = 0;
>>
>> And this iov_iter_truncate(0);
>
> Actually, it should probably have been iov_iter_advance().
>
>> While I'm wondering why we had this at all.
>>
>> It seems all callers of cifs_read_iter_from_socket()
>> don't care and the code path via sock_recvmsg() doesn't
>> truncate it just calls copy_to_iter() via this chain:
>> ->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
>> ->simple_copy_to_iter->copy_to_iter()
>>
>> I think the old code should have called
>> iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
>>
>> But the new code doesn't need it as copy_to_iter()
>> calls iterate_and_advance().
>
> Yeah, it should. I seem to remember that there were situations in which it
> didn't, but it's possible I managed to get rid of them.
>
>>> - default:
>>> - /* It's a bug in upper layer to get there */
>>> - cifs_dbg(VFS, "Invalid msg type %d\n",
>>> - iov_iter_type(&msg->msg_iter));
>>> - rc = -EINVAL;
>>> - }
>>
>> I guess this is actually a real fix as I just saw
>> CIFS: VFS: Invalid msg type 4
>> in logs while running the cifs/001 test.
>> And 4 is ITER_FOLIOQ.
>
> Ah... Were you using "-o seal"? The encrypted data is held in a buffer formed
> from a folioq with a series of folios in it.
In local.config I have this:
[smb3-1-rdma]
FSTYP=cifs
TEST_DEV=//172.31.9.1/TEST
TEST_DIR=/mnt/test
TEST_FS_MOUNT_OPTS='-ousername=administrator,password=...,rdma,noperm,vers=3.0,mfsymlinks,actimeo=0'
export MOUNT_OPTIONS='-ousername=administrator,password=...,rdma,noperm,mfsymlinks,actimeo=0'
export SCRATCH_DEV=//172.31.9.1/SCRATCH
export SCRATCH_MNT=/mnt/scratch
And called:
./check -s smb3-1-rdma -E tests/cifs/exclude.incompatible-smb3.txt -E tests/cifs/exclude.very-slow.txt cifs/001
So I don't think it used seal, I'm also not seeing encrypted stuff in the capture,
so maybe the folioq iter comes from a higher layer (via cifs_readv_receive()) instead
of receive_encrypted_read().
metze
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-24 14:22 ` David Howells
2025-06-24 16:05 ` Stefan Metzmacher
@ 2025-06-25 8:07 ` Stefan Metzmacher
2025-06-25 10:10 ` Stefan Metzmacher
2025-06-25 12:47 ` David Howells
2 siblings, 1 reply; 11+ messages in thread
From: Stefan Metzmacher @ 2025-06-25 8:07 UTC (permalink / raw)
To: David Howells
Cc: linux-cifs@vger.kernel.org, netfs, linux-fsdevel, Steve French
Am 24.06.25 um 16:22 schrieb David Howells:
> Stefan Metzmacher <metze@samba.org> wrote:
>
>>> read_rfc1002_done:
>>> + /* SMBDirect will read it all or nothing */
>>> + msg->msg_iter.count = 0;
>>
>> And this iov_iter_truncate(0);
>
> Actually, it should probably have been iov_iter_advance().
>
>> While I'm wondering why we had this at all.
>>
>> It seems all callers of cifs_read_iter_from_socket()
>> don't care and the code path via sock_recvmsg() doesn't
>> truncate it just calls copy_to_iter() via this chain:
>> ->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
>> ->simple_copy_to_iter->copy_to_iter()
>>
>> I think the old code should have called
>> iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
>>
>> But the new code doesn't need it as copy_to_iter()
>> calls iterate_and_advance().
>
> Yeah, it should. I seem to remember that there were situations in which it
> didn't, but it's possible I managed to get rid of them.
>
>>> - default:
>>> - /* It's a bug in upper layer to get there */
>>> - cifs_dbg(VFS, "Invalid msg type %d\n",
>>> - iov_iter_type(&msg->msg_iter));
>>> - rc = -EINVAL;
>>> - }
>>
>> I guess this is actually a real fix as I just saw
>> CIFS: VFS: Invalid msg type 4
>> in logs while running the cifs/001 test.
>> And 4 is ITER_FOLIOQ.
>
> Ah... Were you using "-o seal"? The encrypted data is held in a buffer formed
> from a folioq with a series of folios in it.
I know tested it standalone in this tree:
https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=46a31189b8b059b3595a9586714761e6e76ba7c4
Doing following mount:
mount -t cifs -ousername=administrator,password=...,rdma,noperm,vers=3.0,mfsymlinks,actimeo=0 //172.31.9.1/test /mnt/test/
It's using the siw driver (with modifications to work against the chelsio t404-bt card on windows) from
here:
https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=5b89ff89f440ec36cf2c5ed2212be0d8523a4c9b
But the siw difference should not really matter.
This realiable generates this:
[ 922.048997] [ T6639] CIFS: Attempting to mount //172.31.9.1/test
[ 922.188445] [ T6639] CIFS: VFS: RDMA transport established
[ 922.217974] [ T6642] usercopy: Kernel memory exposure attempt detected from SLUB object 'smbd_response_0000000091e24ea1' (offset 81, size 63)!
[ 922.218221] [ T6642] ------------[ cut here ]------------
[ 922.218230] [ T6642] kernel BUG at mm/usercopy.c:102!
[ 922.218299] [ T6642] Oops: invalid opcode: 0000 [#1] SMP KASAN PTI
[ 922.218439] [ T6642] CPU: 1 UID: 0 PID: 6642 Comm: cifsd Kdump: loaded Tainted: G OE 6.16.0-rc3-metze.01+ #1 PREEMPT(voluntary)
[ 922.218585] [ T6642] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 922.218635] [ T6642] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 922.218704] [ T6642] RIP: 0010:usercopy_abort+0x6c/0x80
[ 922.218783] [ T6642] Code: fa 91 51 48 c7 c2 c0 d4 fa 91 41 52 48 c7 c7 40 d5 fa 91 48 0f 45 d6 48 c7 c6 00 d5 fa 91 48 89 c1 49 0f 45 f3 e8 84 aa 6b ff <0f> 0b 49 c7
c1 c0 d3 fa 91 4d 89 ca 4d 89 c8 eb a8 0f 1f 00 90 90
[ 922.218925] [ T6642] RSP: 0018:ffffc90001887820 EFLAGS: 00010246
[ 922.218983] [ T6642] RAX: 0000000000000079 RBX: 0000000000000051 RCX: 0000000000000000
[ 922.219046] [ T6642] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 922.219108] [ T6642] RBP: ffffc90001887838 R08: 0000000000000000 R09: 0000000000000000
[ 922.219201] [ T6642] R10: 0000000000000000 R11: 0000000000000000 R12: 000000000000003f
[ 922.219261] [ T6642] R13: ffff88801f579280 R14: 0000000000000001 R15: ffffea0000163340
[ 922.219323] [ T6642] FS: 0000000000000000(0000) GS:ffff8881466e8000(0000) knlGS:0000000000000000
[ 922.219415] [ T6642] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 922.219469] [ T6642] CR2: 000075a216d19bb8 CR3: 000000000f5f6004 CR4: 00000000000726f0
[ 922.219560] [ T6642] Call Trace:
[ 922.219591] [ T6642] <TASK>
[ 922.219624] [ T6642] __check_heap_object+0xe3/0x120
[ 922.221090] [ T6642] __check_object_size+0x4dc/0x6d0
[ 922.222547] [ T6642] smbd_recv+0x77f/0xfe0 [cifs]
[ 922.224416] [ T6642] ? __pfx_smbd_recv+0x10/0x10 [cifs]
[ 922.226195] [ T6642] ? __kasan_check_write+0x14/0x30
[ 922.227722] [ T6642] ? _raw_spin_lock+0x81/0xf0
[ 922.229190] [ T6642] ? __pfx__raw_spin_lock+0x10/0x10
[ 922.230699] [ T6642] ? sched_clock_noinstr+0x9/0x10
[ 922.232248] [ T6642] cifs_readv_from_socket+0x276/0x8f0 [cifs]
[ 922.234149] [ T6642] ? __pfx_cifs_readv_from_socket+0x10/0x10 [cifs]
[ 922.236222] [ T6642] ? mempool_alloc_slab+0x15/0x20
[ 922.237705] [ T6642] cifs_read_from_socket+0xcd/0x120 [cifs]
[ 922.239559] [ T6642] ? __pfx_cifs_read_from_socket+0x10/0x10 [cifs]
[ 922.241403] [ T6642] ? __pfx_mempool_alloc_noprof+0x10/0x10
[ 922.242827] [ T6642] ? __kasan_check_write+0x14/0x30
[ 922.244141] [ T6642] ? cifs_small_buf_get+0x62/0x90 [cifs]
[ 922.245500] [ T6642] ? allocate_buffers+0x216/0x390 [cifs]
[ 922.246810] [ T6642] cifs_demultiplex_thread+0x7e9/0x2d50 [cifs]
[ 922.248150] [ T6642] ? _raw_spin_lock_irqsave+0x95/0x100
[ 922.249143] [ T6642] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
[ 922.250163] [ T6642] ? __pfx___schedule+0x10/0x10
[ 922.250977] [ T6642] ? _raw_spin_lock_irqsave+0x95/0x100
[ 922.251715] [ T6642] ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[ 922.252415] [ T6642] ? __pfx_try_to_wake_up+0x10/0x10
[ 922.253094] [ T6642] ? __kasan_check_read+0x11/0x20
[ 922.253766] [ T6642] ? __kthread_parkme+0xa0/0x190
[ 922.254344] [ T6642] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
[ 922.255073] [ T6642] kthread+0x396/0x830
[ 922.255584] [ T6642] ? __pfx__raw_spin_lock_irq+0x10/0x10
[ 922.256070] [ T6642] ? __pfx_kthread+0x10/0x10
[ 922.256568] [ T6642] ? __kasan_check_write+0x14/0x30
[ 922.257047] [ T6642] ? recalc_sigpending+0x180/0x210
[ 922.257535] [ T6642] ? _raw_spin_unlock_irq+0xe/0x50
[ 922.258015] [ T6642] ? calculate_sigpending+0x84/0xb0
[ 922.258509] [ T6642] ? __pfx_kthread+0x10/0x10
[ 922.258976] [ T6642] ret_from_fork+0x2b8/0x3b0
[ 922.259377] [ T6642] ? __pfx_kthread+0x10/0x10
[ 922.259757] [ T6642] ret_from_fork_asm+0x1a/0x30
[ 922.260133] [ T6642] </TASK>
[ 922.260514] [ T6642] Modules linked in: cifs(OE) ccm cmac nls_utf8 cifs_arc4 nls_ucs2_utils rdma_cm iw_cm ib_cm cifs_md4 netfs siw(OE) ib_uverbs ib_core softdog vboxsf
vboxguest intel_rapl_msr intel_rapl_common intel_uncore_frequency_common intel_pmc_core pmt_telemetry pmt_class intel_pmc_ssram_telemetry intel_vsec polyval_clmulni
ghash_clmulni_intel sha1_ssse3 aesni_intel rapl i2c_piix4 i2c_smbus input_leds joydev mac_hid sunrpc binfmt_misc kvm_intel kvm irqbypass sch_fq_codel efi_pstore nfnetlink
vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vsock vmw_vmci dmi_sysfs ip_tables x_tables autofs4 hid_generic vboxvideo drm_vram_helper usbhid
drm_ttm_helper vga16fb hid vgastate ahci ttm libahci video pata_acpi psmouse serio_raw wmi [last unloaded: cifs(OE)]
Reverting it fixes it again.
metze
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-25 8:07 ` Stefan Metzmacher
@ 2025-06-25 10:10 ` Stefan Metzmacher
2025-06-25 11:25 ` David Howells
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Metzmacher @ 2025-06-25 10:10 UTC (permalink / raw)
To: David Howells
Cc: linux-cifs@vger.kernel.org, netfs, linux-fsdevel, Steve French
Am 25.06.25 um 10:07 schrieb Stefan Metzmacher:
> Am 24.06.25 um 16:22 schrieb David Howells:
>> Stefan Metzmacher <metze@samba.org> wrote:
>>
>>>> read_rfc1002_done:
>>>> + /* SMBDirect will read it all or nothing */
>>>> + msg->msg_iter.count = 0;
>>>
>>> And this iov_iter_truncate(0);
>>
>> Actually, it should probably have been iov_iter_advance().
>>
>>> While I'm wondering why we had this at all.
>>>
>>> It seems all callers of cifs_read_iter_from_socket()
>>> don't care and the code path via sock_recvmsg() doesn't
>>> truncate it just calls copy_to_iter() via this chain:
>>> ->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
>>> ->simple_copy_to_iter->copy_to_iter()
>>>
>>> I think the old code should have called
>>> iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
>>>
>>> But the new code doesn't need it as copy_to_iter()
>>> calls iterate_and_advance().
>>
>> Yeah, it should. I seem to remember that there were situations in which it
>> didn't, but it's possible I managed to get rid of them.
>>
>>>> - default:
>>>> - /* It's a bug in upper layer to get there */
>>>> - cifs_dbg(VFS, "Invalid msg type %d\n",
>>>> - iov_iter_type(&msg->msg_iter));
>>>> - rc = -EINVAL;
>>>> - }
>>>
>>> I guess this is actually a real fix as I just saw
>>> CIFS: VFS: Invalid msg type 4
>>> in logs while running the cifs/001 test.
>>> And 4 is ITER_FOLIOQ.
>>
>> Ah... Were you using "-o seal"? The encrypted data is held in a buffer formed
>> from a folioq with a series of folios in it.
>
> I know tested it standalone in this tree:
> https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=46a31189b8b059b3595a9586714761e6e76ba7c4
It also happens with this:
https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=442dcd18dc1bf8d1e39f53d20810ca0a4958d139
Which contains your netfs fixes...
> Doing following mount:
>
> mount -t cifs -ousername=administrator,password=...,rdma,noperm,vers=3.0,mfsymlinks,actimeo=0 //172.31.9.1/test /mnt/test/
>
> It's using the siw driver (with modifications to work against the chelsio t404-bt card on windows) from
> here:
> https://git.samba.org/?p=metze/linux/wip.git;a=shortlog;h=5b89ff89f440ec36cf2c5ed2212be0d8523a4c9b
>
> But the siw difference should not really matter.
>
> This realiable generates this:
>
> [ 922.048997] [ T6639] CIFS: Attempting to mount //172.31.9.1/test
> [ 922.188445] [ T6639] CIFS: VFS: RDMA transport established
> [ 922.217974] [ T6642] usercopy: Kernel memory exposure attempt detected from SLUB object 'smbd_response_0000000091e24ea1' (offset 81, size 63)!
> [ 922.218221] [ T6642] ------------[ cut here ]------------
> [ 922.218230] [ T6642] kernel BUG at mm/usercopy.c:102!
> [ 922.218299] [ T6642] Oops: invalid opcode: 0000 [#1] SMP KASAN PTI
> [ 922.218439] [ T6642] CPU: 1 UID: 0 PID: 6642 Comm: cifsd Kdump: loaded Tainted: G OE 6.16.0-rc3-metze.01+ #1 PREEMPT(voluntary)
> [ 922.218585] [ T6642] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> [ 922.218635] [ T6642] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> [ 922.218704] [ T6642] RIP: 0010:usercopy_abort+0x6c/0x80
> [ 922.218783] [ T6642] Code: fa 91 51 48 c7 c2 c0 d4 fa 91 41 52 48 c7 c7 40 d5 fa 91 48 0f 45 d6 48 c7 c6 00 d5 fa 91 48 89 c1 49 0f 45 f3 e8 84 aa 6b ff <0f> 0b 49 c7
> c1 c0 d3 fa 91 4d 89 ca 4d 89 c8 eb a8 0f 1f 00 90 90
> [ 922.218925] [ T6642] RSP: 0018:ffffc90001887820 EFLAGS: 00010246
> [ 922.218983] [ T6642] RAX: 0000000000000079 RBX: 0000000000000051 RCX: 0000000000000000
> [ 922.219046] [ T6642] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [ 922.219108] [ T6642] RBP: ffffc90001887838 R08: 0000000000000000 R09: 0000000000000000
> [ 922.219201] [ T6642] R10: 0000000000000000 R11: 0000000000000000 R12: 000000000000003f
> [ 922.219261] [ T6642] R13: ffff88801f579280 R14: 0000000000000001 R15: ffffea0000163340
> [ 922.219323] [ T6642] FS: 0000000000000000(0000) GS:ffff8881466e8000(0000) knlGS:0000000000000000
> [ 922.219415] [ T6642] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 922.219469] [ T6642] CR2: 000075a216d19bb8 CR3: 000000000f5f6004 CR4: 00000000000726f0
> [ 922.219560] [ T6642] Call Trace:
> [ 922.219591] [ T6642] <TASK>
> [ 922.219624] [ T6642] __check_heap_object+0xe3/0x120
> [ 922.221090] [ T6642] __check_object_size+0x4dc/0x6d0
> [ 922.222547] [ T6642] smbd_recv+0x77f/0xfe0 [cifs]
> [ 922.224416] [ T6642] ? __pfx_smbd_recv+0x10/0x10 [cifs]
> [ 922.226195] [ T6642] ? __kasan_check_write+0x14/0x30
> [ 922.227722] [ T6642] ? _raw_spin_lock+0x81/0xf0
> [ 922.229190] [ T6642] ? __pfx__raw_spin_lock+0x10/0x10
> [ 922.230699] [ T6642] ? sched_clock_noinstr+0x9/0x10
> [ 922.232248] [ T6642] cifs_readv_from_socket+0x276/0x8f0 [cifs]
> [ 922.234149] [ T6642] ? __pfx_cifs_readv_from_socket+0x10/0x10 [cifs]
> [ 922.236222] [ T6642] ? mempool_alloc_slab+0x15/0x20
> [ 922.237705] [ T6642] cifs_read_from_socket+0xcd/0x120 [cifs]
> [ 922.239559] [ T6642] ? __pfx_cifs_read_from_socket+0x10/0x10 [cifs]
> [ 922.241403] [ T6642] ? __pfx_mempool_alloc_noprof+0x10/0x10
> [ 922.242827] [ T6642] ? __kasan_check_write+0x14/0x30
> [ 922.244141] [ T6642] ? cifs_small_buf_get+0x62/0x90 [cifs]
> [ 922.245500] [ T6642] ? allocate_buffers+0x216/0x390 [cifs]
> [ 922.246810] [ T6642] cifs_demultiplex_thread+0x7e9/0x2d50 [cifs]
> [ 922.248150] [ T6642] ? _raw_spin_lock_irqsave+0x95/0x100
> [ 922.249143] [ T6642] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
> [ 922.250163] [ T6642] ? __pfx___schedule+0x10/0x10
> [ 922.250977] [ T6642] ? _raw_spin_lock_irqsave+0x95/0x100
> [ 922.251715] [ T6642] ? __pfx__raw_spin_lock_irqsave+0x10/0x10
> [ 922.252415] [ T6642] ? __pfx_try_to_wake_up+0x10/0x10
> [ 922.253094] [ T6642] ? __kasan_check_read+0x11/0x20
> [ 922.253766] [ T6642] ? __kthread_parkme+0xa0/0x190
> [ 922.254344] [ T6642] ? __pfx_cifs_demultiplex_thread+0x10/0x10 [cifs]
> [ 922.255073] [ T6642] kthread+0x396/0x830
> [ 922.255584] [ T6642] ? __pfx__raw_spin_lock_irq+0x10/0x10
> [ 922.256070] [ T6642] ? __pfx_kthread+0x10/0x10
> [ 922.256568] [ T6642] ? __kasan_check_write+0x14/0x30
> [ 922.257047] [ T6642] ? recalc_sigpending+0x180/0x210
> [ 922.257535] [ T6642] ? _raw_spin_unlock_irq+0xe/0x50
> [ 922.258015] [ T6642] ? calculate_sigpending+0x84/0xb0
> [ 922.258509] [ T6642] ? __pfx_kthread+0x10/0x10
> [ 922.258976] [ T6642] ret_from_fork+0x2b8/0x3b0
> [ 922.259377] [ T6642] ? __pfx_kthread+0x10/0x10
> [ 922.259757] [ T6642] ret_from_fork_asm+0x1a/0x30
> [ 922.260133] [ T6642] </TASK>
> [ 922.260514] [ T6642] Modules linked in: cifs(OE) ccm cmac nls_utf8 cifs_arc4 nls_ucs2_utils rdma_cm iw_cm ib_cm cifs_md4 netfs siw(OE) ib_uverbs ib_core softdog vboxsf
> vboxguest intel_rapl_msr intel_rapl_common intel_uncore_frequency_common intel_pmc_core pmt_telemetry pmt_class intel_pmc_ssram_telemetry intel_vsec polyval_clmulni
> ghash_clmulni_intel sha1_ssse3 aesni_intel rapl i2c_piix4 i2c_smbus input_leds joydev mac_hid sunrpc binfmt_misc kvm_intel kvm irqbypass sch_fq_codel efi_pstore nfnetlink
> vsock_loopback vmw_vsock_virtio_transport_common vmw_vsock_vmci_transport vsock vmw_vmci dmi_sysfs ip_tables x_tables autofs4 hid_generic vboxvideo drm_vram_helper usbhid
> drm_ttm_helper vga16fb hid vgastate ahci ttm libahci video pata_acpi psmouse serio_raw wmi [last unloaded: cifs(OE)]
>
>
> Reverting it fixes it again.
>
> metze
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-24 14:22 ` David Howells
2025-06-24 16:05 ` Stefan Metzmacher
2025-06-25 8:07 ` Stefan Metzmacher
@ 2025-06-25 12:47 ` David Howells
2 siblings, 0 replies; 11+ messages in thread
From: David Howells @ 2025-06-25 12:47 UTC (permalink / raw)
Cc: dhowells, Stefan Metzmacher, linux-cifs@vger.kernel.org, netfs,
linux-fsdevel, Steve French
David Howells <dhowells@redhat.com> wrote:
> > And 4 is ITER_FOLIOQ.
I dumped some of the fields from the MID involved:
CIFS: VFS: Invalid msg type 4 (mid=a4 optype=0 command=8)
CIFS: VFS: - rcv=cifs_readv_receive+0x0/0x270 cb=smb2_readv_callback+0x0/0x480 hand=smb3_handle_read_data+0x0/0x40
So the ITER_FOLIOQ is from netfslib. I've attached corresponding trace log,
edited down a bit to remove some columns. Note that the EINVAL error gets
discarded by cifs_demultiplex_thread() and replaced with EAGAIN by netfslib.
David
---
diff-6828: netfs_rreq_ref: R=0000000c NEW r=2
diff-6828: netfs_read: R=0000000c READPAGE c=00000000 ni=1d4072 s=0 l=1000 sz=400
diff-6828: netfs_rreq_ref: R=0000000c GET SUBREQ r=3
diff-6828: smb3_rw_credits: R=0000000c[1] rd-submit cred=16 chg=0 pool=1688 ifl=1
diff-6828: netfs_sreq: R=0000000c[1] DOWN PREP f=000 s=0 0/400 s=0 e=0
diff-6828: smb3_rw_credits: R=0000000c[1] rd-issu-adj cred=16 chg=-15 pool=1688 ifl=1
diff-6828: netfs_sreq: R=0000000c[1] DOWN SUBMT f=102 s=0 0/400 s=0 e=0
diff-6828: netfs_rreq_ref: R=0000000c GET SUBREQ r=4
diff-6828: netfs_sreq: R=0000000c[2] ZERO SUBMT f=000 s=400 0/c00 s=0 e=0
diff-6828: netfs_sreq: R=0000000c[2] ZERO TERM f=102 s=400 c00/c00 s=1 e=0
diff-6828: netfs_collect_stream: R=0000000c[0:] cto=0 frn=0
diff-6828: netfs_rreq: R=0000000c RP WAIT-IP f=03
cifsd-6506: netfs_sreq: R=0000000c[1] DOWN I-RTR f=102 s=0 0/400 s=0 e=0
cifsd-6506: smb3_read_err: R=0000000c[1] xid=200 sid=0x8 tid=0x2 fid=0xa0952 offset=0x0 len=0x400 rc=-11
cifsd-6506: smb3_rw_credits: R=0000000c[1] rd-resp-clr cred=1 chg=0 pool=1703 ifl=1
cifsd-6506: netfs_sreq: R=0000000c[1] DOWN I-OK f=302 s=0 0/400 s=0 e=-11
cifsd-6506: netfs_failure: R=0000000c[1] DOWN f=302 s=0 0/400 read e=-11
cifsd-6506: netfs_rreq: R=0000000c RP PAUSE f=03
cifsd-6506: netfs_sreq: R=0000000c[1] DOWN TERM f=702 s=0 0/400 s=0 e=-11
cifsd-6506: netfs_rreq: R=0000000c RP WAKE-Q f=07
cifsd-6506: smb3_rw_credits: R=0000000c[1] rd-resp-add cred=0 chg=0 pool=1703 ifl=1
diff-6828: netfs_collect_stream: R=0000000c[0:] cto=0 frn=0
diff-6828: netfs_rreq: R=0000000c RP COLLECT f=07
diff-6828: netfs_collect: R=0000000c s=0-1000
diff-6828: netfs_collect_sreq: R=0000000c[0:01] s=0 t=0/400
diff-6828: netfs_rreq: R=0000000c RP S-ABNDN f=07
diff-6828: netfs_sreq: R=0000000c[1] DOWN ABNDN f=602 s=0 400/400 s=0 e=-11
diff-6828: netfs_sreq: R=0000000c[1] DOWN FREE f=602 s=0 400/400 s=0 e=-11
diff-6828: netfs_rreq_ref: R=0000000c PUT SUBREQ r=3
diff-6828: netfs_collect_sreq: R=0000000c[0:02] s=400 t=c00/c00
diff-6828: netfs_sreq: R=0000000c[2] ZERO ABNDN f=002 s=400 c00/c00 s=1 e=0
diff-6828: netfs_sreq: R=0000000c[2] ZERO FREE f=002 s=400 c00/c00 s=1 e=0
diff-6828: netfs_rreq_ref: R=0000000c PUT SUBREQ r=2
diff-6828: netfs_collect_stream: R=0000000c[0:] cto=1000 frn=ffffffff
diff-6828: netfs_collect_state: R=0000000c col=1000 cln=1000 n=8c
diff-6828: netfs_rreq: R=0000000c RP UNPAUSE f=0b
diff-6828: netfs_collect_stream: R=0000000c[0:] cto=1000 frn=ffffffff
diff-6828: netfs_collect_state: R=0000000c col=1000 cln=1000 n=8
diff-6828: netfs_rreq: R=0000000c RP COMPLET f=0b
diff-6828: netfs_rreq: R=0000000c RP WAKE-IP f=0a
diff-6828: netfs_rreq: R=0000000c RP DONE f=0a
diff-6828: netfs_rreq_ref: R=0000000c PUT WORK IP r=1
diff-6828: netfs_rreq: R=0000000c RP DONE-IP f=0a
diff-6828: netfs_rreq_ref: R=0000000c PUT RETURN r=0
kworker/u16:0-12 : netfs_rreq: R=0000000c RP FREE f=0a
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-24 12:25 ` Stefan Metzmacher
2025-06-24 14:22 ` David Howells
@ 2025-06-25 14:18 ` Stefan Metzmacher
2025-06-25 16:00 ` David Howells
1 sibling, 1 reply; 11+ messages in thread
From: Stefan Metzmacher @ 2025-06-25 14:18 UTC (permalink / raw)
To: David Howells
Cc: linux-cifs@vger.kernel.org, netfs, linux-fsdevel, Steve French
Am 24.06.25 um 14:25 schrieb Stefan Metzmacher:
> Hi David,
>
> this looks very useful! Just a few comments below...
>
>> Collapse smbd_recv_buf() and smbd_recv_page() into smbd_recv() and just use
>> copy_to_iter() instead of memcpy().
>>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> cc: Steve French <stfrench@microsoft.com>
>> cc: Tom Talpey <tom@talpey.com>
>> cc: Stefan Metzmacher <metze@samba.org>
>> cc: Paulo Alcantara (Red Hat) <pc@manguebit.com>
>> cc: Matthew Wilcox <willy@infradead.org>
>> cc: linux-cifs@vger.kernel.org
>> cc: netfs@lists.linux.dev
>> cc: linux-fsdevel@vger.kernel.org
>> ---
>> fs/smb/client/smbdirect.c | 116 +++++++---------------------------------------
>> 1 file changed, 20 insertions(+), 96 deletions(-)
>>
>> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
>> index 5ae847919da5..dc64c337aae0 100644
>> --- a/fs/smb/client/smbdirect.c
>> +++ b/fs/smb/client/smbdirect.c
>> @@ -1747,35 +1747,39 @@ struct smbd_connection *smbd_get_connection(
>> }
>> /*
>> - * Receive data from receive reassembly queue
>> + * Receive data from the transport's receive reassembly queue
>> * All the incoming data packets are placed in reassembly queue
>> - * buf: the buffer to read data into
>> + * iter: the buffer to read data into
>> * size: the length of data to read
>> * return value: actual data read
>> - * Note: this implementation copies the data from reassebmly queue to receive
>> + *
>> + * Note: this implementation copies the data from reassembly queue to receive
>> * buffers used by upper layer. This is not the optimal code path. A better way
>> * to do it is to not have upper layer allocate its receive buffers but rather
>> * borrow the buffer from reassembly queue, and return it after data is
>> * consumed. But this will require more changes to upper layer code, and also
>> * need to consider packet boundaries while they still being reassembled.
>> */
>> -static int smbd_recv_buf(struct smbd_connection *info, char *buf,
>> - unsigned int size)
>> +int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
>> {
>> struct smbdirect_socket *sc = &info->socket;
>> struct smbd_response *response;
>> struct smbdirect_data_transfer *data_transfer;
>> + size_t size = msg->msg_iter.count;
>
> I think this should be iov_iter_count()?
>
>> int to_copy, to_read, data_read, offset;
>> u32 data_length, remaining_data_length, data_offset;
>> int rc;
>> + if (WARN_ON_ONCE(iov_iter_rw(&msg->msg_iter) == WRITE))
>> + return -EINVAL; /* It's a bug in upper layer to get there */
>> +
>> again:
>> /*
>> * No need to hold the reassembly queue lock all the time as we are
>> * the only one reading from the front of the queue. The transport
>> * may add more entries to the back of the queue at the same time
>> */
>> - log_read(INFO, "size=%d info->reassembly_data_length=%d\n", size,
>> + log_read(INFO, "size=%zd info->reassembly_data_length=%d\n", size,
>> info->reassembly_data_length);
>> if (info->reassembly_data_length >= size) {
>> int queue_length;
>> @@ -1811,9 +1815,12 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
>> * transport layer is added
>> */
>> if (response->first_segment && size == 4) {
>> - unsigned int rfc1002_len =
>> + unsigned int len =
Please keep the rfc1002_len variable as it's used in the log_read message below
and it should by host byteorder.
I'd propose a diff like this:
@@ -1846,8 +1850,11 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
if (response->first_segment && size == 4) {
unsigned int rfc1002_len =
data_length + remaining_data_length;
- *((__be32 *)buf) = cpu_to_be32(rfc1002_len);
+ __be32 rfc1002_hdr = cpu_to_be32(rfc1002_len);
data_read = 4;
+ if (copy_to_iter(&rfc1002_hdr, sizeof(rfc1002_hdr),
+ &msg->msg_iter) != data_read)
+ return -EFAULT;
response->first_segment = false;
log_read(INFO, "returning rfc1002 length %d\n",
rfc1002_len);
>> data_length + remaining_data_length;
>> - *((__be32 *)buf) = cpu_to_be32(rfc1002_len);
>> + __be32 rfc1002_len = cpu_to_be32(len);
>> + if (copy_to_iter(&rfc1002_len, sizeof(rfc1002_len),
>> + &msg->msg_iter) != sizeof(rfc1002_len))
>> + return -EFAULT;
>> data_read = 4;
>> response->first_segment = false;
>> log_read(INFO, "returning rfc1002 length %d\n",
>> @@ -1822,10 +1829,9 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
>> }
>> to_copy = min_t(int, data_length - offset, to_read);
>> - memcpy(
>> - buf + data_read,
>> - (char *)data_transfer + data_offset + offset,
>> - to_copy);
>> + if (copy_to_iter((char *)data_transfer + data_offset + offset,
>> + to_copy, &msg->msg_iter) != to_copy)
>> + return -EFAULT;
>> /* move on to the next buffer? */
>> if (to_copy == data_length - offset) {
>> @@ -1870,6 +1876,8 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
>> data_read, info->reassembly_data_length,
>> info->first_entry_offset);
>> read_rfc1002_done:
>> + /* SMBDirect will read it all or nothing */
>> + msg->msg_iter.count = 0;
>
> And this iov_iter_truncate(0);
>
> While I'm wondering why we had this at all.
>
> It seems all callers of cifs_read_iter_from_socket()
> don't care and the code path via sock_recvmsg() doesn't
> truncate it just calls copy_to_iter() via this chain:
> ->inet_recvmsg->tcp_recvmsg->skb_copy_datagram_msg->skb_copy_datagram_iter
> ->simple_copy_to_iter->copy_to_iter()
>
> I think the old code should have called
> iov_iter_advance(rc) instead of msg->msg_iter.count = 0.
>
> But the new code doesn't need it as copy_to_iter()
> calls iterate_and_advance().
>
>> return data_read;
>> }
>> @@ -1890,90 +1898,6 @@ static int smbd_recv_buf(struct smbd_connection *info, char *buf,
>> goto again;
>> }
>> -/*
>> - * Receive a page from receive reassembly queue
>> - * page: the page to read data into
>> - * to_read: the length of data to read
>> - * return value: actual data read
>> - */
>> -static int smbd_recv_page(struct smbd_connection *info,
>> - struct page *page, unsigned int page_offset,
>> - unsigned int to_read)
>> -{
>> - struct smbdirect_socket *sc = &info->socket;
>> - int ret;
>> - char *to_address;
>> - void *page_address;
>> -
>> - /* make sure we have the page ready for read */
>> - ret = wait_event_interruptible(
>> - info->wait_reassembly_queue,
>> - info->reassembly_data_length >= to_read ||
>> - sc->status != SMBDIRECT_SOCKET_CONNECTED);
>> - if (ret)
>> - return ret;
>> -
>> - /* now we can read from reassembly queue and not sleep */
>> - page_address = kmap_atomic(page);
>> - to_address = (char *) page_address + page_offset;
>> -
>> - log_read(INFO, "reading from page=%p address=%p to_read=%d\n",
>> - page, to_address, to_read);
>> -
>> - ret = smbd_recv_buf(info, to_address, to_read);
>> - kunmap_atomic(page_address);
>> -
>> - return ret;
>> -}
>> -
>> -/*
>> - * Receive data from transport
>> - * msg: a msghdr point to the buffer, can be ITER_KVEC or ITER_BVEC
>> - * return: total bytes read, or 0. SMB Direct will not do partial read.
>> - */
>> -int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
>> -{
>> - char *buf;
>> - struct page *page;
>> - unsigned int to_read, page_offset;
>> - int rc;
>> -
>> - if (iov_iter_rw(&msg->msg_iter) == WRITE) {
>> - /* It's a bug in upper layer to get there */
>> - cifs_dbg(VFS, "Invalid msg iter dir %u\n",
>> - iov_iter_rw(&msg->msg_iter));
>> - rc = -EINVAL;
>> - goto out;
>> - }
>> -
>> - switch (iov_iter_type(&msg->msg_iter)) {
>> - case ITER_KVEC:
>> - buf = msg->msg_iter.kvec->iov_base;
>> - to_read = msg->msg_iter.kvec->iov_len;
>> - rc = smbd_recv_buf(info, buf, to_read);
>> - break;
>> -
>> - case ITER_BVEC:
>> - page = msg->msg_iter.bvec->bv_page;
>> - page_offset = msg->msg_iter.bvec->bv_offset;
>> - to_read = msg->msg_iter.bvec->bv_len;
>> - rc = smbd_recv_page(info, page, page_offset, to_read);
>> - break;
>> -
>> - default:
>> - /* It's a bug in upper layer to get there */
>> - cifs_dbg(VFS, "Invalid msg type %d\n",
>> - iov_iter_type(&msg->msg_iter));
>> - rc = -EINVAL;
>> - }
>
> I guess this is actually a real fix as I just saw
> CIFS: VFS: Invalid msg type 4
> in logs while running the cifs/001 test.
> And 4 is ITER_FOLIOQ.
>
> So there might be something broken when ITER_FOLIOQ was
> introduced, but I wasn't able to find a specific commit.
> Maybe it was also already broken when using
> smb3 encryption over smbdirect, when ITER_XARRAY was still used.
>
> metze
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter()
2025-06-25 14:18 ` Stefan Metzmacher
@ 2025-06-25 16:00 ` David Howells
0 siblings, 0 replies; 11+ messages in thread
From: David Howells @ 2025-06-25 16:00 UTC (permalink / raw)
To: Stefan Metzmacher
Cc: dhowells, linux-cifs@vger.kernel.org, netfs, linux-fsdevel,
Steve French
Stefan Metzmacher <metze@samba.org> wrote:
> Please keep the rfc1002_len variable as it's used in the log_read message
> below and it should by host byteorder.
So this change on top of the patch I posted?
@@ -1838,11 +1838,11 @@ int smbd_recv(struct smbd_connection *info, struct msghdr *msg)
* transport layer is added
*/
if (response->first_segment && size == 4) {
- unsigned int len =
+ unsigned int rfc1002_len =
data_length + remaining_data_length;
- __be32 rfc1002_len = cpu_to_be32(len);
- if (copy_to_iter(&rfc1002_len, sizeof(rfc1002_len),
- &msg->msg_iter) != sizeof(rfc1002_len))
+ __be32 rfc1002_hdr = cpu_to_be32(rfc1002_len);
+ if (copy_to_iter(&rfc1002_hdr, sizeof(rfc1002_hdr),
+ &msg->msg_iter) != sizeof(rfc1002_hdr))
return -EFAULT;
data_read = 4;
response->first_segment = false;
Btw, I'm changing the patch subject and description to:
cifs: Fix reading into an ITER_FOLIOQ from the smbdirect code
When performing a file read from RDMA, smbd_recv() prints an "Invalid msg
type 4" error and fails the I/O. This is due to the switch-statement there
not handling the ITER_FOLIOQ handed down from netfslib.
Fix this by collapsing smbd_recv_buf() and smbd_recv_page() into
smbd_recv() and just using copy_to_iter() instead of memcpy(). This
future-proofs the function too, in case more ITER_* types are added.
Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading")
Reported-by: Stefan Metzmacher <metze@samba.org>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <stfrench@microsoft.com>
cc: Tom Talpey <tom@talpey.com>
cc: Paulo Alcantara (Red Hat) <pc@manguebit.com>
cc: Matthew Wilcox <willy@infradead.org>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
David
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-06-25 16:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23 13:04 [PATCH] cifs: Collapse smbd_recv_*() into smbd_recv() and just use copy_to_iter() David Howells
2025-06-24 12:25 ` Stefan Metzmacher
2025-06-24 14:22 ` David Howells
2025-06-24 16:05 ` Stefan Metzmacher
2025-06-25 8:07 ` Stefan Metzmacher
2025-06-25 10:10 ` Stefan Metzmacher
2025-06-25 11:25 ` David Howells
2025-06-25 11:51 ` Stefan Metzmacher
2025-06-25 12:47 ` David Howells
2025-06-25 14:18 ` Stefan Metzmacher
2025-06-25 16:00 ` David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox