Re: [PATCH v2 12/16] cifs: Fix reading into an ITER_FOLIOQ from the smbdirect code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 25.06.25 um 18:42 schrieb David Howells:
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@xxxxxxxxx>
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
cc: Steve French <stfrench@xxxxxxxxxxxxx>
cc: Tom Talpey <tom@xxxxxxxxxx>
cc: Paulo Alcantara (Red Hat) <pc@xxxxxxxxxxxxx>
cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
cc: linux-cifs@xxxxxxxxxxxxxxx
cc: netfs@xxxxxxxxxxxxxxx
cc: linux-fsdevel@xxxxxxxxxxxxxxx
---
  fs/smb/client/smbdirect.c | 114 +++++++-------------------------------
  1 file changed, 19 insertions(+), 95 deletions(-)

diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
index a976bcf61226..5fa46b2e682c 100644
--- a/fs/smb/client/smbdirect.c
+++ b/fs/smb/client/smbdirect.c
@@ -1770,35 +1770,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 = iov_iter_count(&msg->msg_iter);
  	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;
@@ -1836,7 +1840,10 @@ 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);
+				if (copy_to_iter(&rfc1002_hdr, sizeof(rfc1002_hdr),
+						 &msg->msg_iter) != sizeof(rfc1002_hdr))
+					return -EFAULT;
  				data_read = 4;
  				response->first_segment = false;
  				log_read(INFO, "returning rfc1002 length %d\n",
@@ -1845,10 +1852,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) {
@@ -1893,6 +1899,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;

I think we should be remove this.

And I think this patch should come after the
CONFIG_HARDENED_USERCOPY change otherwise a bisect will trigger the problem.

metze




[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux