[PATCH v3 0/2] ksmbd: fix named-pipe reply loss and operation races

[PATCH v3 0/2] ksmbd: fix named-pipe reply loss and operation races

[Wang Zhaolong]

This is v3 of the named-pipe fix series. v2 was sent earlier; v3 is
rebased onto current mainline and keeps the new mainline RPC pipe
ownership checks.

The first problem is that RPC IPC replies are matched only by the pipe
handle. READ, WRITE, FSCTL_PIPE_TRANSCEIVE and CLOSE on one pipe all use
that handle, and the session RPC lock is dropped before an IPC request
waits for its reply. There is no separate IPC correlation ID, so two
operations can overlap and interfere with reply delivery. CLOSE can also
release the handle while an earlier operation is still in flight.
Protecting only the handle table is not enough: an active pipe needs
per-pipe serialization and lifetime.

Patch 1 serializes operations per pipe, keeps the pipe state and handle
alive until all users finish, and rechecks cancellation, connection
release and session teardown after taking the pipe lock. It erases a
closed pipe before waiting for its active operation and releases the
handle id only after the last user. Session logoff now clears the RPC
pipe list after the other requests have drained. Failed pipe operations
use standard SMB2 error replies while preserving the selected status.
The existing foreign-handle checks and STATUS_FILE_CLOSED behavior added
by current mainline are retained.

The second problem is reply loss with small output buffers.
FSCTL_PIPE_TRANSCEIVE copies at most MaxOutputResponse bytes and returns
STATUS_BUFFER_OVERFLOW when the RPC reply is larger, but then frees the
whole IPC reply. The unread bytes are no longer available to a follow-up
READ, so the RPC call can fail even though userspace produced a complete
reply. READ also ignores the requested Length when copying an IPC reply.

Patch 2 builds on the per-pipe lifetime from patch 1 and keeps the
original reply allocation and an offset on the pipe. READ and TRANSCEIVE
consume only the bytes that fit and retain the rest. The retained reply
is freed only when drained, or when the pipe is closed, logged off or
disconnected. A failed response preparation or input request does not
discard output the client has not received. An IPC failure is reported
as an I/O error instead of an empty read or a successful write without a
reply. No kernel RPC queue is added; additional reply queueing remains
userspace's responsibility.

Changes in v3:
- Rebased from 21a7f44fd076 to 7d37252fc49a.
- Preserved current mainline's ksmbd_session_rpc_method() and
  STATUS_FILE_CLOSED checks for foreign RPC pipe handles. The new
  lifetime lookup maps its -ENOENT race result to STATUS_FILE_CLOSED.
- No other functional change from v2. The W=1 kernel-doc warning fix is
  retained.
- Revalidated both reports on current mainline.

Reproduction and validation on 7d37252fc49a:
- Short output: a 2632-byte NetrShareEnum reply with a 512-byte
  MaxOutputResponse returns 512 bytes; the base loses the remaining 2120
  bytes and the follow-up READ returns STATUS_UNEXPECTED_IO_ERROR. With
  the series, the follow-up READ returns the remaining 2120 bytes and the
  reassembled reply is the exact 2632-byte control reply.
- Same-pipe overlap: SIGSTOP ksmbd.mountd, send two TRANSCEIVE requests
  with different RPC call IDs, then SIGCONT. Five base runs all failed
  (three call-ID swaps, two empty bodies); five runs with the series all
  passed with each response carrying its own call ID and the full reply.
- Both tests used the same baseline ksmbd.mountd binary and the same
  16-share configuration. No test-only kernel delay patch was used.

The series was built and tested on x86-64 in an isolated Debian QEMU
guest. It is kernel-only and does not require a userspace protocol
change. It does not change the IPC timeout, quotas, on-disk format or
protocol ABI.

Bugzilla references:
  222196: short PIPE_TRANSCEIVE output loses unread RPC reply bytes
  222197: same-pipe RPC replies can reach the wrong request

Wang Zhaolong (2):
  ksmbd: avoid races between named-pipe operations
  ksmbd: fix named-pipe reply loss with small output buffers

 fs/smb/server/mgmt/user_session.c | 128 +++++++++-----
 fs/smb/server/mgmt/user_session.h |  18 ++
 fs/smb/server/smb2pdu.c           | 283 ++++++++++++++++++++----------
 fs/smb/server/transport_ipc.c     |  45 +++--
 fs/smb/server/transport_ipc.h     |  14 +-
 5 files changed, 325 insertions(+), 163 deletions(-)

base-commit: 7d37252fc49a939d005a7495b81d1bb337c29e45
-- 
2.56.0

[PATCH v3 1/2] ksmbd: avoid races between named-pipe operations

[Wang Zhaolong]

RPC IPC replies are matched by pipe handle. READ, WRITE, TRANSCEIVE and
CLOSE on the same pipe all use that handle. The session RPC lock protects
the handle table, but is dropped before a pipe I/O request waits for its
reply. Another operation, including CLOSE, can issue a request with the
same handle while that wait is still in progress.

These requests have no separate IPC correlation ID, so overlapping waits
can interfere with reply delivery. CLOSE can also release the handle
before an earlier operation finishes. Protecting table access alone does
not provide the serialization or lifetime needed by an active pipe.

Serialize operations per pipe and keep its state and handle alive until
all users finish. Avoid holding the session table lock across IPC waits,
so a blocked pipe does not hold up unrelated pipes. This also provides
the lifetime needed to retain unread replies in the following patch.

Recheck cancellation, connection release and session teardown after taking
the pipe lock, and mark a removed pipe closed before waiting for its active
operation. Close all session RPC pipes on logoff after the other requests
have drained; keeping the TCP connection open must not retain old handles.
Use standard SMB2 error replies for failed pipe operations while preserving
the selected status.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=222197
Signed-off-by: Wang Zhaolong <wangzhaolong3@outlook.com>
---
 fs/smb/server/mgmt/user_session.c | 127 +++++++++++++-------
 fs/smb/server/mgmt/user_session.h |  15 +++
 fs/smb/server/smb2pdu.c           | 190 +++++++++++++++++++++---------
 fs/smb/server/transport_ipc.c     |  45 ++++---
 fs/smb/server/transport_ipc.h     |  14 ++-
 5 files changed, 263 insertions(+), 128 deletions(-)

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index 3ff3671cd019..4db69809b9fd 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/err.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/rwsem.h>
@@ -43,11 +44,6 @@ void ksmbd_session_release_slot(struct ksmbd_conn *conn)
 	atomic64_dec(&conn->session_count);
 }
 
-struct ksmbd_session_rpc {
-	int			id;
-	unsigned int		method;
-};
-
 #ifdef CONFIG_PROC_FS
 
 static const struct ksmbd_const_name ksmbd_sess_cap_const_names[] = {
@@ -301,32 +297,34 @@ static void free_channel_list(struct ksmbd_session *sess)
 	up_write(&sess->chann_lock);
 }
 
-static void __session_rpc_close(struct ksmbd_session *sess,
-				struct ksmbd_session_rpc *entry)
+static void ksmbd_session_rpc_free(struct ksmbd_session_rpc *entry)
+{
+	if (refcount_dec_and_test(&entry->refcount)) {
+		ksmbd_rpc_id_free(entry->id);
+		kfree(entry);
+	}
+}
+
+static void __session_rpc_close(struct ksmbd_session_rpc *entry)
 {
 	struct ksmbd_rpc_command *resp;
 
-	resp = ksmbd_rpc_close(sess, entry->id);
+	mutex_lock(&entry->lock);
+	resp = ksmbd_rpc_close(entry);
 	if (!resp)
 		pr_err("Unable to close RPC pipe %d\n", entry->id);
-
 	kvfree(resp);
-	ksmbd_rpc_id_free(entry->id);
-	kfree(entry);
+	mutex_unlock(&entry->lock);
+	ksmbd_session_rpc_free(entry);
 }
 
-static void ksmbd_session_rpc_clear_list(struct ksmbd_session *sess)
+void ksmbd_session_rpc_clear_list(struct ksmbd_session *sess)
 {
 	struct ksmbd_session_rpc *entry;
-	long index;
-
-	down_write(&sess->rpc_lock);
-	xa_for_each(&sess->rpc_handle_list, index, entry) {
-		xa_erase(&sess->rpc_handle_list, index);
-		__session_rpc_close(sess, entry);
-	}
-	up_write(&sess->rpc_lock);
+	unsigned long index;
 
+	xa_for_each(&sess->rpc_handle_list, index, entry)
+		ksmbd_session_rpc_close(sess, index);
 	xa_destroy(&sess->rpc_handle_list);
 }
 
@@ -356,9 +354,9 @@ static int __rpc_method(char *rpc_name)
 
 int ksmbd_session_rpc_open(struct ksmbd_session *sess, char *rpc_name)
 {
-	struct ksmbd_session_rpc *entry, *old;
+	struct ksmbd_session_rpc *entry;
 	struct ksmbd_rpc_command *resp;
-	int method, id;
+	int method, id, ret;
 
 	method = __rpc_method(rpc_name);
 	if (method < 0)
@@ -370,31 +368,29 @@ int ksmbd_session_rpc_open(struct ksmbd_session *sess, char *rpc_name)
 
 	entry->method = method;
 	entry->id = id = ksmbd_ipc_id_alloc();
-	if (id < 0)
-		goto free_entry;
-
-	down_write(&sess->rpc_lock);
-	old = xa_store(&sess->rpc_handle_list, id, entry, KSMBD_DEFAULT_GFP);
-	if (xa_is_err(old)) {
-		up_write(&sess->rpc_lock);
-		goto free_id;
+	if (id < 0) {
+		kfree(entry);
+		return id;
 	}
+	mutex_init(&entry->lock);
+	refcount_set(&entry->refcount, 1);
 
-	resp = ksmbd_rpc_open(sess, id);
+	resp = ksmbd_rpc_open(entry);
 	if (!resp) {
-		xa_erase(&sess->rpc_handle_list, entry->id);
-		up_write(&sess->rpc_lock);
-		goto free_id;
+		ksmbd_session_rpc_free(entry);
+		return -EINVAL;
 	}
+	kvfree(resp);
 
+	down_write(&sess->rpc_lock);
+	ret = xa_err(xa_store(&sess->rpc_handle_list, id, entry,
+			      KSMBD_DEFAULT_GFP));
 	up_write(&sess->rpc_lock);
-	kvfree(resp);
+	if (ret) {
+		__session_rpc_close(entry);
+		return ret;
+	}
 	return id;
-free_id:
-	ksmbd_rpc_id_free(entry->id);
-free_entry:
-	kfree(entry);
-	return -EINVAL;
 }
 
 void ksmbd_session_rpc_close(struct ksmbd_session *sess, int id)
@@ -404,8 +400,53 @@ void ksmbd_session_rpc_close(struct ksmbd_session *sess, int id)
 	down_write(&sess->rpc_lock);
 	entry = xa_erase(&sess->rpc_handle_list, id);
 	if (entry)
-		__session_rpc_close(sess, entry);
+		WRITE_ONCE(entry->closed, true);
 	up_write(&sess->rpc_lock);
+	if (entry)
+		__session_rpc_close(entry);
+}
+
+/* The reference pins the entry while waiting without holding rpc_lock. */
+struct ksmbd_session_rpc *ksmbd_session_rpc_get(struct ksmbd_work *work,
+					     int id)
+{
+	struct ksmbd_session_rpc *entry;
+	int err;
+
+	down_read(&work->sess->rpc_lock);
+	entry = xa_load(&work->sess->rpc_handle_list, id);
+	if (entry)
+		refcount_inc(&entry->refcount);
+	up_read(&work->sess->rpc_lock);
+	if (!entry)
+		return ERR_PTR(-ENOENT);
+
+	mutex_lock(&entry->lock);
+	if (READ_ONCE(work->state) == KSMBD_WORK_CANCELLED) {
+		err = -ECANCELED;
+		goto err_put;
+	}
+	if (ksmbd_conn_exiting(work->conn) ||
+	    ksmbd_conn_releasing(work->conn) ||
+	    READ_ONCE(work->sess->tearing_down)) {
+		err = -ESHUTDOWN;
+		goto err_put;
+	}
+	if (READ_ONCE(entry->closed)) {
+		err = -EBADF;
+		goto err_put;
+	}
+	return entry;
+
+err_put:
+	ksmbd_session_rpc_put(entry);
+	return ERR_PTR(err);
+}
+
+void ksmbd_session_rpc_put(struct ksmbd_session_rpc *entry)
+{
+	mutex_unlock(&entry->lock);
+	ksmbd_session_rpc_free(entry);
 }
 
 int ksmbd_session_rpc_method(struct ksmbd_session *sess, int id)
@@ -904,7 +945,7 @@ void destroy_previous_session(struct ksmbd_conn *conn,
 		up_write(&prev_sess->chann_lock);
 		goto out;
 	}
-	prev_sess->tearing_down = true;
+	WRITE_ONCE(prev_sess->tearing_down, true);
 	up_write(&prev_sess->chann_lock);
 	ksmbd_user_session_get(prev_sess);
 	pinned = true;
@@ -923,7 +964,7 @@ void destroy_previous_session(struct ksmbd_conn *conn,
 	down_write(&conn->session_lock);
 	if (err) {
 		down_write(&prev_sess->chann_lock);
-		prev_sess->tearing_down = false;
+		WRITE_ONCE(prev_sess->tearing_down, false);
 		up_write(&prev_sess->chann_lock);
 		ksmbd_all_conn_set_status(prev_sess, KSMBD_SESS_GOOD);
 		goto out;
diff --git a/fs/smb/server/mgmt/user_session.h b/fs/smb/server/mgmt/user_session.h
index 8e4ddb04f0e6..38073ea9089d 100644
--- a/fs/smb/server/mgmt/user_session.h
+++ b/fs/smb/server/mgmt/user_session.h
@@ -7,6 +7,8 @@
 #define __USER_SESSION_MANAGEMENT_H__
 
 #include <linux/hashtable.h>
+#include <linux/mutex.h>
+#include <linux/refcount.h>
 #include <linux/xarray.h>
 
 #include "../smb_common.h"
@@ -17,6 +19,15 @@
 #define PREAUTH_HASHVALUE_SIZE		64
 
 struct ksmbd_file_table;
+struct ksmbd_work;
+
+struct ksmbd_session_rpc {
+	int			id;
+	unsigned int		method;
+	struct mutex		lock;
+	refcount_t		refcount;
+	bool			closed;
+};
 
 struct channel {
 	char			sess_key[CIFS_KEY_SIZE];
@@ -133,7 +144,11 @@ void ksmbd_release_tree_conn_id(struct ksmbd_session *sess, int id);
 
 int ksmbd_session_rpc_open(struct ksmbd_session *sess, char *rpc_name);
 void ksmbd_session_rpc_close(struct ksmbd_session *sess, int id);
+void ksmbd_session_rpc_clear_list(struct ksmbd_session *sess);
 int ksmbd_session_rpc_method(struct ksmbd_session *sess, int id);
+struct ksmbd_session_rpc *ksmbd_session_rpc_get(struct ksmbd_work *work,
+					     int id);
+void ksmbd_session_rpc_put(struct ksmbd_session_rpc *entry);
 void ksmbd_user_session_get(struct ksmbd_session *sess);
 void ksmbd_user_session_put(struct ksmbd_session *sess);
 int create_proc_sessions(void);
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index f5fe3cf8ba32..66123b7afd5d 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -3234,7 +3234,7 @@ int smb2_session_logoff(struct ksmbd_work *work)
 		smb2_set_err_rsp(work);
 		return -ENOENT;
 	}
-	sess->tearing_down = true;
+	WRITE_ONCE(sess->tearing_down, true);
 	up_write(&sess->chann_lock);
 
 	ksmbd_all_conn_set_status(sess, KSMBD_SESS_NEED_RECONNECT);
@@ -3243,7 +3243,7 @@ int smb2_session_logoff(struct ksmbd_work *work)
 	err = ksmbd_conn_wait_idle_sess(conn, sess);
 	if (err) {
 		down_write(&sess->chann_lock);
-		sess->tearing_down = false;
+		WRITE_ONCE(sess->tearing_down, false);
 		up_write(&sess->chann_lock);
 		ksmbd_all_conn_set_status(sess, KSMBD_SESS_GOOD);
 		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
@@ -3252,6 +3252,7 @@ int smb2_session_logoff(struct ksmbd_work *work)
 	}
 
 	ksmbd_close_session_fds(work);
+	ksmbd_session_rpc_clear_list(sess);
 
 	if (ksmbd_tree_conn_session_logoff(sess)) {
 		ksmbd_debug(SMB, "Invalid tid %d\n", req->hdr.Id.SyncId.TreeId);
@@ -9246,6 +9247,21 @@ int smb2_set_info(struct ksmbd_work *work)
 	return rc;
 }
 
+static __le32 smb2_pipe_error_status(int err, __le32 fallback)
+{
+	switch (err) {
+	case -ECANCELED:
+		return STATUS_CANCELLED;
+	case -ESHUTDOWN:
+		return STATUS_CONNECTION_DISCONNECTED;
+	case -EBADF:
+	case -ENOENT:
+		return STATUS_FILE_CLOSED;
+	default:
+		return fallback;
+	}
+}
+
 /**
  * smb2_read_pipe() - handler for smb2 read from IPC pipe
  * @work:	smb work containing read IPC pipe command buffer
@@ -9257,6 +9273,7 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 	int nbytes = 0, err;
 	u64 id;
 	struct ksmbd_rpc_command *rpc_resp;
+	struct ksmbd_session_rpc *rpc;
 	struct smb2_read_req *req;
 	struct smb2_read_rsp *rsp;
 
@@ -9274,13 +9291,19 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 	}
 	up_read(&work->sess->rpc_lock);
 
-	rpc_resp = ksmbd_rpc_read(work->sess, id);
+	rpc = ksmbd_session_rpc_get(work, id);
+	if (IS_ERR(rpc)) {
+		err = PTR_ERR(rpc);
+		rpc_resp = NULL;
+		goto out;
+	}
+	rpc_resp = ksmbd_rpc_read(work->sess, rpc);
 	if (rpc_resp) {
 		void *aux_payload_buf;
 
 		if (rpc_resp->flags != KSMBD_RPC_OK) {
 			err = -EINVAL;
-			goto out;
+			goto out_put;
 		}
 
 		aux_payload_buf =
@@ -9288,7 +9311,7 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 				 KSMBD_DEFAULT_GFP);
 		if (!aux_payload_buf) {
 			err = -ENOMEM;
-			goto out;
+			goto out_put;
 		}
 
 		memcpy(aux_payload_buf, rpc_resp->payload, rpc_resp->payload_sz);
@@ -9303,14 +9326,12 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 					     aux_payload_buf, nbytes);
 		if (err) {
 			kvfree(aux_payload_buf);
-			goto out;
+			goto out_put;
 		}
 		kvfree(rpc_resp);
 	} else {
-		err = ksmbd_iov_pin_rsp(work, (void *)rsp,
-					offsetof(struct smb2_read_rsp, Buffer));
-		if (err)
-			goto out;
+		err = -EIO;
+		goto out_put;
 	}
 
 	rsp->StructureSize = cpu_to_le16(17);
@@ -9319,10 +9340,14 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 	rsp->DataLength = cpu_to_le32(nbytes);
 	rsp->DataRemaining = 0;
 	rsp->Flags = 0;
+	ksmbd_session_rpc_put(rpc);
 	return 0;
 
+out_put:
+	ksmbd_session_rpc_put(rpc);
 out:
-	rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
+	rsp->hdr.Status = smb2_pipe_error_status(err,
+						STATUS_UNEXPECTED_IO_ERROR);
 	smb2_set_err_rsp(work);
 	kvfree(rpc_resp);
 	return err;
@@ -9883,6 +9908,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
 	struct smb2_write_req *req;
 	struct smb2_write_rsp *rsp;
 	struct ksmbd_rpc_command *rpc_resp;
+	struct ksmbd_session_rpc *rpc;
 	u64 id = 0;
 	int err = 0, ret = 0;
 	char *data_buf;
@@ -9917,22 +9943,34 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
 	data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
 			   data_offset);
 
-	rpc_resp = ksmbd_rpc_write(work->sess, id, data_buf, length);
-	if (rpc_resp) {
-		if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
-			rsp->hdr.Status = STATUS_NOT_SUPPORTED;
-			kvfree(rpc_resp);
-			smb2_set_err_rsp(work);
-			return -EOPNOTSUPP;
-		}
-		if (rpc_resp->flags != KSMBD_RPC_OK) {
-			rsp->hdr.Status = STATUS_INVALID_HANDLE;
-			smb2_set_err_rsp(work);
-			kvfree(rpc_resp);
-			return ret;
-		}
+	rpc = ksmbd_session_rpc_get(work, id);
+	if (IS_ERR(rpc)) {
+		err = PTR_ERR(rpc);
+		rsp->hdr.Status = smb2_pipe_error_status(err,
+							STATUS_INVALID_HANDLE);
+		smb2_set_err_rsp(work);
+		return err;
+	}
+	rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf, length);
+	ksmbd_session_rpc_put(rpc);
+	if (!rpc_resp) {
+		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
+		smb2_set_err_rsp(work);
+		return -EIO;
+	}
+	if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
+		rsp->hdr.Status = STATUS_NOT_SUPPORTED;
+		kvfree(rpc_resp);
+		smb2_set_err_rsp(work);
+		return -EOPNOTSUPP;
+	}
+	if (rpc_resp->flags != KSMBD_RPC_OK) {
+		rsp->hdr.Status = STATUS_INVALID_HANDLE;
+		smb2_set_err_rsp(work);
 		kvfree(rpc_resp);
+		return ret;
 	}
+	kvfree(rpc_resp);
 
 	rsp->StructureSize = cpu_to_le16(17);
 	rsp->DataOffset = 0;
@@ -11378,49 +11416,87 @@ static int fsctl_query_allocated_ranges(struct ksmbd_work *work, u64 id,
 	return ret;
 }
 
+static int smb2_ioctl_rsp(struct ksmbd_work *work, struct smb2_ioctl_rsp *rsp,
+			  unsigned int cnt_code, unsigned int nbytes)
+{
+	rsp->CtlCode = cpu_to_le32(cnt_code);
+	rsp->InputCount = cpu_to_le32(0);
+	rsp->InputOffset = cpu_to_le32(112);
+	rsp->OutputOffset = cpu_to_le32(112);
+	rsp->OutputCount = cpu_to_le32(nbytes);
+	rsp->StructureSize = cpu_to_le16(49);
+	rsp->Reserved = cpu_to_le16(0);
+	rsp->Flags = cpu_to_le32(0);
+	rsp->Reserved2 = cpu_to_le32(0);
+	return ksmbd_iov_pin_rsp(work, rsp,
+				 sizeof(struct smb2_ioctl_rsp) + nbytes);
+}
+
 static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
 				 unsigned int out_buf_len,
 				 struct smb2_ioctl_req *req,
 				 struct smb2_ioctl_rsp *rsp)
 {
 	struct ksmbd_rpc_command *rpc_resp;
+	struct ksmbd_session_rpc *rpc;
 	char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
-	int nbytes = 0;
+	unsigned int nbytes;
+	int err;
 
-	rpc_resp = ksmbd_rpc_ioctl(work->sess, id, data_buf,
+	rpc = ksmbd_session_rpc_get(work, id);
+	if (IS_ERR(rpc)) {
+		err = PTR_ERR(rpc);
+		rsp->hdr.Status = smb2_pipe_error_status(err,
+							STATUS_INVALID_PARAMETER);
+		smb2_set_err_rsp(work);
+		return 0;
+	}
+
+	rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
 				   le32_to_cpu(req->InputCount));
-	if (rpc_resp) {
-		if (rpc_resp->flags == KSMBD_RPC_SOME_NOT_MAPPED) {
-			/*
-			 * set STATUS_SOME_NOT_MAPPED response
-			 * for unknown domain sid.
-			 */
-			rsp->hdr.Status = STATUS_SOME_NOT_MAPPED;
-		} else if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
-			rsp->hdr.Status = STATUS_NOT_SUPPORTED;
-			goto out;
-		} else if (rpc_resp->flags != KSMBD_RPC_OK) {
-			rsp->hdr.Status = STATUS_INVALID_PARAMETER;
-			goto out;
-		}
+	if (!rpc_resp) {
+		ksmbd_session_rpc_put(rpc);
+		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
+		smb2_set_err_rsp(work);
+		return 0;
+	}
 
-		nbytes = rpc_resp->payload_sz;
-		if (rpc_resp->payload_sz > out_buf_len) {
-			rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
-			nbytes = out_buf_len;
-		}
+	if (rpc_resp->flags == KSMBD_RPC_SOME_NOT_MAPPED) {
+		/*
+		 * set STATUS_SOME_NOT_MAPPED response
+		 * for unknown domain sid.
+		 */
+		rsp->hdr.Status = STATUS_SOME_NOT_MAPPED;
+	} else if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
+		rsp->hdr.Status = STATUS_NOT_SUPPORTED;
+		goto error;
+	} else if (rpc_resp->flags != KSMBD_RPC_OK) {
+		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
+		goto error;
+	}
 
-		if (!rpc_resp->payload_sz) {
-			rsp->hdr.Status =
-				STATUS_UNEXPECTED_IO_ERROR;
-			goto out;
-		}
+	nbytes = rpc_resp->payload_sz;
+	if (rpc_resp->payload_sz > out_buf_len) {
+		rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
+		nbytes = out_buf_len;
+	}
 
-		memcpy((char *)rsp->Buffer, rpc_resp->payload, nbytes);
+	if (!rpc_resp->payload_sz) {
+		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
+		goto error;
 	}
-out:
+
+	memcpy((char *)rsp->Buffer, rpc_resp->payload, nbytes);
+	err = smb2_ioctl_rsp(work, rsp, FSCTL_PIPE_TRANSCEIVE, nbytes);
+	ksmbd_session_rpc_put(rpc);
 	kvfree(rpc_resp);
-	return nbytes;
+	return err;
+
+error:
+	ksmbd_session_rpc_put(rpc);
+	kvfree(rpc_resp);
+	smb2_set_err_rsp(work);
+	return 0;
 }
 
 static inline int fsctl_set_sparse(struct ksmbd_work *work, u64 id,
@@ -11752,8 +11828,10 @@ int smb2_ioctl(struct ksmbd_work *work)
 		rsp->PersistentFileId = pid;
 		rsp->VolatileFileId = id;
 		out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
-		nbytes = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
-		break;
+		ret = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
+		if (ret < 0)
+			goto out;
+		return 0;
 	case FSCTL_VALIDATE_NEGOTIATE_INFO:
 		if (conn->dialect < SMB30_PROT_ID) {
 			ret = -EOPNOTSUPP;
diff --git a/fs/smb/server/transport_ipc.c b/fs/smb/server/transport_ipc.c
index eb07cdf17410..e8dfc42a1d08 100644
--- a/fs/smb/server/transport_ipc.c
+++ b/fs/smb/server/transport_ipc.c
@@ -834,7 +834,7 @@ ksmbd_ipc_share_config_request(const char *name)
 	return resp;
 }
 
-struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session *sess, int handle)
+struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session_rpc *entry)
 {
 	struct ksmbd_ipc_msg *msg;
 	struct ksmbd_rpc_command *req;
@@ -846,8 +846,8 @@ struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session *sess, int handle)
 
 	msg->type = KSMBD_EVENT_RPC_REQUEST;
 	req = (struct ksmbd_rpc_command *)msg->payload;
-	req->handle = handle;
-	req->flags = ksmbd_session_rpc_method(sess, handle);
+	req->handle = entry->id;
+	req->flags = entry->method;
 	req->flags |= KSMBD_RPC_OPEN_METHOD;
 	req->payload_sz = 0;
 
@@ -856,7 +856,7 @@ struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session *sess, int handle)
 	return resp;
 }
 
-struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session *sess, int handle)
+struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session_rpc *entry)
 {
 	struct ksmbd_ipc_msg *msg;
 	struct ksmbd_rpc_command *req;
@@ -868,8 +868,8 @@ struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session *sess, int handle
 
 	msg->type = KSMBD_EVENT_RPC_REQUEST;
 	req = (struct ksmbd_rpc_command *)msg->payload;
-	req->handle = handle;
-	req->flags = ksmbd_session_rpc_method(sess, handle);
+	req->handle = entry->id;
+	req->flags = entry->method;
 	req->flags |= KSMBD_RPC_CLOSE_METHOD;
 	req->payload_sz = 0;
 
@@ -878,7 +878,8 @@ struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session *sess, int handle
 	return resp;
 }
 
-struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess, int handle,
+struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess,
+					  struct ksmbd_session_rpc *entry,
 					  void *payload, size_t payload_sz)
 {
 	struct ksmbd_ipc_msg *msg;
@@ -892,25 +893,24 @@ struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess, int handle
 	if (!msg)
 		return NULL;
 
-	lockdep_assert_not_held(&sess->rpc_lock);
+	lockdep_assert_held(&entry->lock);
 
-	down_read(&sess->rpc_lock);
 	msg->type = KSMBD_EVENT_RPC_REQUEST;
 	req = (struct ksmbd_rpc_command *)msg->payload;
-	req->handle = handle;
-	req->flags = ksmbd_session_rpc_method(sess, handle);
+	req->handle = entry->id;
+	req->flags = entry->method;
 	req->flags |= rpc_context_flags(sess);
 	req->flags |= KSMBD_RPC_WRITE_METHOD;
 	req->payload_sz = payload_sz;
 	memcpy(req->payload, payload, payload_sz);
-	up_read(&sess->rpc_lock);
 
 	resp = ipc_msg_send_request(msg, req->handle);
 	ipc_msg_free(msg);
 	return resp;
 }
 
-struct ksmbd_rpc_command *ksmbd_rpc_read(struct ksmbd_session *sess, int handle)
+struct ksmbd_rpc_command *ksmbd_rpc_read(struct ksmbd_session *sess,
+					  struct ksmbd_session_rpc *entry)
 {
 	struct ksmbd_ipc_msg *msg;
 	struct ksmbd_rpc_command *req;
@@ -920,24 +920,23 @@ struct ksmbd_rpc_command *ksmbd_rpc_read(struct ksmbd_session *sess, int handle)
 	if (!msg)
 		return NULL;
 
-	lockdep_assert_not_held(&sess->rpc_lock);
+	lockdep_assert_held(&entry->lock);
 
-	down_read(&sess->rpc_lock);
 	msg->type = KSMBD_EVENT_RPC_REQUEST;
 	req = (struct ksmbd_rpc_command *)msg->payload;
-	req->handle = handle;
-	req->flags = ksmbd_session_rpc_method(sess, handle);
+	req->handle = entry->id;
+	req->flags = entry->method;
 	req->flags |= rpc_context_flags(sess);
 	req->flags |= KSMBD_RPC_READ_METHOD;
 	req->payload_sz = 0;
-	up_read(&sess->rpc_lock);
 
 	resp = ipc_msg_send_request(msg, req->handle);
 	ipc_msg_free(msg);
 	return resp;
 }
 
-struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess, int handle,
+struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess,
+					  struct ksmbd_session_rpc *entry,
 					  void *payload, size_t payload_sz)
 {
 	struct ksmbd_ipc_msg *msg;
@@ -951,18 +950,16 @@ struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess, int handle
 	if (!msg)
 		return NULL;
 
-	lockdep_assert_not_held(&sess->rpc_lock);
+	lockdep_assert_held(&entry->lock);
 
-	down_read(&sess->rpc_lock);
 	msg->type = KSMBD_EVENT_RPC_REQUEST;
 	req = (struct ksmbd_rpc_command *)msg->payload;
-	req->handle = handle;
-	req->flags = ksmbd_session_rpc_method(sess, handle);
+	req->handle = entry->id;
+	req->flags = entry->method;
 	req->flags |= rpc_context_flags(sess);
 	req->flags |= KSMBD_RPC_IOCTL_METHOD;
 	req->payload_sz = payload_sz;
 	memcpy(req->payload, payload, payload_sz);
-	up_read(&sess->rpc_lock);
 
 	resp = ipc_msg_send_request(msg, req->handle);
 	ipc_msg_free(msg);
diff --git a/fs/smb/server/transport_ipc.h b/fs/smb/server/transport_ipc.h
index e51850f1423b..df94ba116d43 100644
--- a/fs/smb/server/transport_ipc.h
+++ b/fs/smb/server/transport_ipc.h
@@ -16,6 +16,7 @@ struct ksmbd_login_response_ext *
 ksmbd_ipc_login_request_ext(const char *account);
 
 struct ksmbd_session;
+struct ksmbd_session_rpc;
 struct ksmbd_share_config;
 struct ksmbd_tree_connect;
 struct sockaddr;
@@ -34,12 +35,15 @@ struct ksmbd_spnego_authen_response *
 ksmbd_ipc_spnego_authen_request(const char *spnego_blob, int blob_len);
 int ksmbd_ipc_id_alloc(void);
 void ksmbd_rpc_id_free(int handle);
-struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session *sess, int handle);
-struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session *sess, int handle);
-struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess, int handle,
+struct ksmbd_rpc_command *ksmbd_rpc_open(struct ksmbd_session_rpc *entry);
+struct ksmbd_rpc_command *ksmbd_rpc_close(struct ksmbd_session_rpc *entry);
+struct ksmbd_rpc_command *ksmbd_rpc_write(struct ksmbd_session *sess,
+					  struct ksmbd_session_rpc *entry,
 					  void *payload, size_t payload_sz);
-struct ksmbd_rpc_command *ksmbd_rpc_read(struct ksmbd_session *sess, int handle);
-struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess, int handle,
+struct ksmbd_rpc_command *ksmbd_rpc_read(struct ksmbd_session *sess,
+					 struct ksmbd_session_rpc *entry);
+struct ksmbd_rpc_command *ksmbd_rpc_ioctl(struct ksmbd_session *sess,
+					  struct ksmbd_session_rpc *entry,
 					  void *payload, size_t payload_sz);
 void ksmbd_ipc_release(void);
 void ksmbd_ipc_soft_reset(void);
-- 
2.56.0

[PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Wang Zhaolong]

FSCTL_PIPE_TRANSCEIVE copies at most MaxOutputResponse bytes from an RPC
reply and reports STATUS_BUFFER_OVERFLOW when it does not fit. It then
frees the complete IPC reply, including the bytes that were not returned.
The client cannot recover those bytes with a subsequent READ, so an RPC
call can fail even though userspace produced a complete reply. Pipe READ
also ignores the requested Length when copying an IPC reply.

In the reproduced Windows NetShareEnum case, a 2632-byte reply is limited
to 1024 bytes of SMB output. The remaining 1608 bytes are discarded
instead of being available to the follow-up READ. Increasing the
application-level enumeration budget does not change that SMB output
limit.

Keep unread reply data on the pipe and respect each operation's output
limit. Preserve it until successfully included in a response: response
preparation failures and later input must not discard output that the
client has yet to receive. Treat a failed IPC exchange as an I/O error,
rather than reporting an empty read or acknowledging a write without
an IPC reply. Additional RPC reply queueing remains userspace's
responsibility.

Keep cached bytes on failed WRITE or TRANSCEIVE requests, and consume them
only after the response is pinned successfully. Release the original reply
when drained or when its pipe is closed, logged off or disconnected.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=222196
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202610260610.5prVfJss-lkp@intel.com/
Signed-off-by: Wang Zhaolong <wangzhaolong3@outlook.com>
---
 fs/smb/server/mgmt/user_session.c |   1 +
 fs/smb/server/mgmt/user_session.h |   3 +
 fs/smb/server/smb2pdu.c           | 159 +++++++++++++++++-------------
 3 files changed, 95 insertions(+), 68 deletions(-)

diff --git a/fs/smb/server/mgmt/user_session.c b/fs/smb/server/mgmt/user_session.c
index 4db69809b9fd..3f75049be878 100644
--- a/fs/smb/server/mgmt/user_session.c
+++ b/fs/smb/server/mgmt/user_session.c
@@ -301,6 +301,7 @@ static void ksmbd_session_rpc_free(struct ksmbd_session_rpc *entry)
 {
 	if (refcount_dec_and_test(&entry->refcount)) {
 		ksmbd_rpc_id_free(entry->id);
+		kvfree(entry->pending);
 		kfree(entry);
 	}
 }
diff --git a/fs/smb/server/mgmt/user_session.h b/fs/smb/server/mgmt/user_session.h
index 38073ea9089d..5f0f6276e316 100644
--- a/fs/smb/server/mgmt/user_session.h
+++ b/fs/smb/server/mgmt/user_session.h
@@ -19,6 +19,7 @@
 #define PREAUTH_HASHVALUE_SIZE		64
 
 struct ksmbd_file_table;
+struct ksmbd_rpc_command;
 struct ksmbd_work;
 
 struct ksmbd_session_rpc {
@@ -27,6 +28,8 @@ struct ksmbd_session_rpc {
 	struct mutex		lock;
 	refcount_t		refcount;
 	bool			closed;
+	struct ksmbd_rpc_command *pending;
+	unsigned int		pending_offset;
 };
 
 struct channel {
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 66123b7afd5d..627b4ff58da2 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -9262,6 +9262,17 @@ static __le32 smb2_pipe_error_status(int err, __le32 fallback)
 	}
 }
 
+static void smb2_pipe_response_done(struct ksmbd_session_rpc *rpc,
+				    unsigned int nbytes)
+{
+	rpc->pending_offset += nbytes;
+	if (rpc->pending_offset == rpc->pending->payload_sz) {
+		kvfree(rpc->pending);
+		rpc->pending = NULL;
+		rpc->pending_offset = 0;
+	}
+}
+
 /**
  * smb2_read_pipe() - handler for smb2 read from IPC pipe
  * @work:	smb work containing read IPC pipe command buffer
@@ -9270,12 +9281,14 @@ static __le32 smb2_pipe_error_status(int err, __le32 fallback)
  */
 static noinline int smb2_read_pipe(struct ksmbd_work *work)
 {
-	int nbytes = 0, err;
+	unsigned int nbytes = 0, length;
 	u64 id;
 	struct ksmbd_rpc_command *rpc_resp;
 	struct ksmbd_session_rpc *rpc;
 	struct smb2_read_req *req;
 	struct smb2_read_rsp *rsp;
+	void *aux_payload_buf;
+	int err;
 
 	WORK_BUFFERS(work, req, rsp);
 
@@ -9294,46 +9307,59 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 	rpc = ksmbd_session_rpc_get(work, id);
 	if (IS_ERR(rpc)) {
 		err = PTR_ERR(rpc);
-		rpc_resp = NULL;
 		goto out;
 	}
-	rpc_resp = ksmbd_rpc_read(work->sess, rpc);
-	if (rpc_resp) {
-		void *aux_payload_buf;
 
+	length = le32_to_cpu(req->Length);
+	if (!length)
+		goto empty;
+
+	if (!rpc->pending) {
+		rpc_resp = ksmbd_rpc_read(work->sess, rpc);
+		if (!rpc_resp) {
+			err = -EIO;
+			goto out_put;
+		}
 		if (rpc_resp->flags != KSMBD_RPC_OK) {
+			kvfree(rpc_resp);
 			err = -EINVAL;
 			goto out_put;
 		}
+		rpc->pending = rpc_resp;
+	}
 
-		aux_payload_buf =
-			kvmalloc(ALIGN(rpc_resp->payload_sz, 8),
-				 KSMBD_DEFAULT_GFP);
-		if (!aux_payload_buf) {
-			err = -ENOMEM;
-			goto out_put;
-		}
+	rpc_resp = rpc->pending;
+	nbytes = min(length, rpc_resp->payload_sz - rpc->pending_offset);
+	if (!nbytes) {
+		smb2_pipe_response_done(rpc, 0);
+		goto empty;
+	}
 
-		memcpy(aux_payload_buf, rpc_resp->payload, rpc_resp->payload_sz);
-		if (rpc_resp->payload_sz & 7)
-			memset(aux_payload_buf + rpc_resp->payload_sz, 0,
-			       ALIGN(rpc_resp->payload_sz, 8) -
-			       rpc_resp->payload_sz);
+	aux_payload_buf = kvmalloc(ALIGN(nbytes, 8), KSMBD_DEFAULT_GFP);
+	if (!aux_payload_buf) {
+		err = -ENOMEM;
+		goto out_put;
+	}
+	memcpy(aux_payload_buf, rpc_resp->payload + rpc->pending_offset, nbytes);
+	if (nbytes & 7)
+		memset(aux_payload_buf + nbytes, 0, ALIGN(nbytes, 8) - nbytes);
 
-		nbytes = rpc_resp->payload_sz;
-		err = ksmbd_iov_pin_rsp_read(work, (void *)rsp,
-					     offsetof(struct smb2_read_rsp, Buffer),
-					     aux_payload_buf, nbytes);
-		if (err) {
-			kvfree(aux_payload_buf);
-			goto out_put;
-		}
-		kvfree(rpc_resp);
-	} else {
-		err = -EIO;
+	err = ksmbd_iov_pin_rsp_read(work, rsp,
+				     offsetof(struct smb2_read_rsp, Buffer),
+				     aux_payload_buf, nbytes);
+	if (err) {
+		kvfree(aux_payload_buf);
 		goto out_put;
 	}
+	smb2_pipe_response_done(rpc, nbytes);
+	goto done;
 
+empty:
+	err = ksmbd_iov_pin_rsp(work, rsp,
+				offsetof(struct smb2_read_rsp, Buffer));
+	if (err)
+		goto out_put;
+done:
 	rsp->StructureSize = cpu_to_le16(17);
 	rsp->DataOffset = 80;
 	rsp->Reserved = 0;
@@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
 	rsp->hdr.Status = smb2_pipe_error_status(err,
 						STATUS_UNEXPECTED_IO_ERROR);
 	smb2_set_err_rsp(work);
-	kvfree(rpc_resp);
 	return err;
 }
 
@@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
 	if (IS_ERR(rpc)) {
 		err = PTR_ERR(rpc);
 		rsp->hdr.Status = smb2_pipe_error_status(err,
-							STATUS_INVALID_HANDLE);
+							 STATUS_INVALID_HANDLE);
 		smb2_set_err_rsp(work);
 		return err;
 	}
@@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
 	struct ksmbd_rpc_command *rpc_resp;
 	struct ksmbd_session_rpc *rpc;
 	char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
-	unsigned int nbytes;
-	int err;
+	unsigned int nbytes = 0;
+	int ret;
 
 	rpc = ksmbd_session_rpc_get(work, id);
 	if (IS_ERR(rpc)) {
-		err = PTR_ERR(rpc);
-		rsp->hdr.Status = smb2_pipe_error_status(err,
-							STATUS_INVALID_PARAMETER);
+		ret = PTR_ERR(rpc);
+		rsp->hdr.Status = smb2_pipe_error_status(ret,
+						 STATUS_INVALID_PARAMETER);
 		smb2_set_err_rsp(work);
 		return 0;
 	}
 
-	rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
-				   le32_to_cpu(req->InputCount));
+	/* Write new input without reading past an already buffered reply. */
+	if (rpc->pending)
+		rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
+					   le32_to_cpu(req->InputCount));
+	else
+		rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
+					   le32_to_cpu(req->InputCount));
 	if (!rpc_resp) {
 		ksmbd_session_rpc_put(rpc);
 		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
@@ -11462,39 +11492,41 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
 	}
 
 	if (rpc_resp->flags == KSMBD_RPC_SOME_NOT_MAPPED) {
-		/*
-		 * set STATUS_SOME_NOT_MAPPED response
-		 * for unknown domain sid.
-		 */
 		rsp->hdr.Status = STATUS_SOME_NOT_MAPPED;
 	} else if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
 		rsp->hdr.Status = STATUS_NOT_SUPPORTED;
-		goto error;
+		goto free_resp;
 	} else if (rpc_resp->flags != KSMBD_RPC_OK) {
 		rsp->hdr.Status = STATUS_INVALID_PARAMETER;
-		goto error;
+		goto free_resp;
 	}
 
-	nbytes = rpc_resp->payload_sz;
-	if (rpc_resp->payload_sz > out_buf_len) {
-		rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
-		nbytes = out_buf_len;
-	}
-
-	if (!rpc_resp->payload_sz) {
+	if (rpc->pending) {
+		kvfree(rpc_resp);
+	} else if (!rpc_resp->payload_sz) {
 		rsp->hdr.Status = STATUS_UNEXPECTED_IO_ERROR;
-		goto error;
+		goto free_resp;
+	} else {
+		rpc->pending = rpc_resp;
 	}
 
-	memcpy((char *)rsp->Buffer, rpc_resp->payload, nbytes);
-	err = smb2_ioctl_rsp(work, rsp, FSCTL_PIPE_TRANSCEIVE, nbytes);
+	rpc_resp = rpc->pending;
+	nbytes = rpc_resp->payload_sz - rpc->pending_offset;
+	if (nbytes > out_buf_len) {
+		rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
+		nbytes = out_buf_len;
+	}
+	memcpy(rsp->Buffer, rpc_resp->payload + rpc->pending_offset, nbytes);
+	ret = smb2_ioctl_rsp(work, rsp, FSCTL_PIPE_TRANSCEIVE, nbytes);
+	if (!ret)
+		smb2_pipe_response_done(rpc, nbytes);
 	ksmbd_session_rpc_put(rpc);
-	kvfree(rpc_resp);
-	return err;
+	return ret;
 
-error:
-	ksmbd_session_rpc_put(rpc);
+free_resp:
 	kvfree(rpc_resp);
+	rsp->hdr.Status = smb2_pipe_error_status(-EINVAL, rsp->hdr.Status);
+	ksmbd_session_rpc_put(rpc);
 	smb2_set_err_rsp(work);
 	return 0;
 }
@@ -11829,7 +11861,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 		rsp->VolatileFileId = id;
 		out_buf_len = min_t(u32, KSMBD_IPC_MAX_PAYLOAD, out_buf_len);
 		ret = fsctl_pipe_transceive(work, id, out_buf_len, req, rsp);
-		if (ret < 0)
+		if (ret)
 			goto out;
 		return 0;
 	case FSCTL_VALIDATE_NEGOTIATE_INFO:
@@ -12210,16 +12242,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 		goto out;
 	}
 
-	rsp->CtlCode = cpu_to_le32(cnt_code);
-	rsp->InputCount = cpu_to_le32(0);
-	rsp->InputOffset = cpu_to_le32(112);
-	rsp->OutputOffset = cpu_to_le32(112);
-	rsp->OutputCount = cpu_to_le32(nbytes);
-	rsp->StructureSize = cpu_to_le16(49);
-	rsp->Reserved = cpu_to_le16(0);
-	rsp->Flags = cpu_to_le32(0);
-	rsp->Reserved2 = cpu_to_le32(0);
-	ret = ksmbd_iov_pin_rsp(work, rsp, sizeof(struct smb2_ioctl_rsp) + nbytes);
+	ret = smb2_ioctl_rsp(work, rsp, cnt_code, nbytes);
 	if (!ret)
 		return ret;
 
-- 
2.56.0

Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Namjae Jeon]

> @@ -9294,46 +9307,59 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>         rpc = ksmbd_session_rpc_get(work, id);
>         if (IS_ERR(rpc)) {
>                 err = PTR_ERR(rpc);
> -               rpc_resp = NULL;
>                 goto out;
>         }
> -       rpc_resp = ksmbd_rpc_read(work->sess, rpc);
> -       if (rpc_resp) {
> -               void *aux_payload_buf;
>
> +       length = le32_to_cpu(req->Length);
> +       if (!length)
> +               goto empty;
> +
> +       if (!rpc->pending) {
> +               rpc_resp = ksmbd_rpc_read(work->sess, rpc);
> +               if (!rpc_resp) {
> +                       err = -EIO;
> +                       goto out_put;
> +               }
>                 if (rpc_resp->flags != KSMBD_RPC_OK) {
> +                       kvfree(rpc_resp);
>                         err = -EINVAL;
>                         goto out_put;
>                 }
> +               rpc->pending = rpc_resp;
> +       }
>
> -               aux_payload_buf =
> -                       kvmalloc(ALIGN(rpc_resp->payload_sz, 8),
> -                                KSMBD_DEFAULT_GFP);
> -               if (!aux_payload_buf) {
> -                       err = -ENOMEM;
> -                       goto out_put;
> -               }
> +       rpc_resp = rpc->pending;
> +       nbytes = min(length, rpc_resp->payload_sz - rpc->pending_offset);
> +       if (!nbytes) {
> +               smb2_pipe_response_done(rpc, 0);
> +               goto empty;
> +       }
>
> -               memcpy(aux_payload_buf, rpc_resp->payload, rpc_resp->payload_sz);
> -               if (rpc_resp->payload_sz & 7)
> -                       memset(aux_payload_buf + rpc_resp->payload_sz, 0,
> -                              ALIGN(rpc_resp->payload_sz, 8) -
> -                              rpc_resp->payload_sz);
> +       aux_payload_buf = kvmalloc(ALIGN(nbytes, 8), KSMBD_DEFAULT_GFP);
> +       if (!aux_payload_buf) {
> +               err = -ENOMEM;
> +               goto out_put;
> +       }
> +       memcpy(aux_payload_buf, rpc_resp->payload + rpc->pending_offset, nbytes);
> +       if (nbytes & 7)
> +               memset(aux_payload_buf + nbytes, 0, ALIGN(nbytes, 8) - nbytes);
>
> -               nbytes = rpc_resp->payload_sz;
> -               err = ksmbd_iov_pin_rsp_read(work, (void *)rsp,
> -                                            offsetof(struct smb2_read_rsp, Buffer),
> -                                            aux_payload_buf, nbytes);
> -               if (err) {
> -                       kvfree(aux_payload_buf);
> -                       goto out_put;
> -               }
> -               kvfree(rpc_resp);
> -       } else {
> -               err = -EIO;
> +       err = ksmbd_iov_pin_rsp_read(work, rsp,
> +                                    offsetof(struct smb2_read_rsp, Buffer),
> +                                    aux_payload_buf, nbytes);
> +       if (err) {
> +               kvfree(aux_payload_buf);
>                 goto out_put;
>         }
> +       smb2_pipe_response_done(rpc, nbytes);

if (rpc->pending)
    rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
Should we add this check ?

> +       goto done;
>
> +empty:
> +       err = ksmbd_iov_pin_rsp(work, rsp,
> +                               offsetof(struct smb2_read_rsp, Buffer));
> +       if (err)
> +               goto out_put;
> +done:
>         rsp->StructureSize = cpu_to_le16(17);
>         rsp->DataOffset = 80;
>         rsp->Reserved = 0;
> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>         rsp->hdr.Status = smb2_pipe_error_status(err,
>                                                 STATUS_UNEXPECTED_IO_ERROR);
>         smb2_set_err_rsp(work);
> -       kvfree(rpc_resp);
>         return err;
>  }
>
> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>         if (IS_ERR(rpc)) {
>                 err = PTR_ERR(rpc);
>                 rsp->hdr.Status = smb2_pipe_error_status(err,
> -                                                       STATUS_INVALID_HANDLE);
> +                                                        STATUS_INVALID_HANDLE);
>                 smb2_set_err_rsp(work);
>                 return err;
>         }
> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
>         struct ksmbd_rpc_command *rpc_resp;
>         struct ksmbd_session_rpc *rpc;
>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
> -       unsigned int nbytes;
> -       int err;
> +       unsigned int nbytes = 0;
> +       int ret;
>
>         rpc = ksmbd_session_rpc_get(work, id);
>         if (IS_ERR(rpc)) {
> -               err = PTR_ERR(rpc);
> -               rsp->hdr.Status = smb2_pipe_error_status(err,
> -                                                       STATUS_INVALID_PARAMETER);
> +               ret = PTR_ERR(rpc);
> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
> +                                                STATUS_INVALID_PARAMETER);
>                 smb2_set_err_rsp(work);
>                 return 0;
>         }
>
> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> -                                  le32_to_cpu(req->InputCount));
> +       /* Write new input without reading past an already buffered reply. */
> +       if (rpc->pending)
> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
> +                                          le32_to_cpu(req->InputCount));
> +       else
> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> +                                          le32_to_cpu(req->InputCount));
This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
cached reply is drained, the next TRANSCEIVE can return the previous
reply without processing its own input.

Thanks.

Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Wang Zhaolong]

Hi Namjae,

> 
> if (rpc->pending)
>     rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
> Should we add this check ?
> 
>> +       goto done;
>>
>> +empty:
>> +       err = ksmbd_iov_pin_rsp(work, rsp,
>> +                               offsetof(struct smb2_read_rsp, Buffer));
>> +       if (err)
>> +               goto out_put;
>> +done:
>>         rsp->StructureSize = cpu_to_le16(17);
>>         rsp->DataOffset = 80;
>>         rsp->Reserved = 0;
>> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>>         rsp->hdr.Status = smb2_pipe_error_status(err,
>>                                                 STATUS_UNEXPECTED_IO_ERROR);
>>         smb2_set_err_rsp(work);
>> -       kvfree(rpc_resp);
>>         return err;
>>  }
>>
>> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>>         if (IS_ERR(rpc)) {
>>                 err = PTR_ERR(rpc);
>>                 rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_HANDLE);
>> +                                                        STATUS_INVALID_HANDLE);
>>                 smb2_set_err_rsp(work);
>>                 return err;
>>         }
>> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
>>         struct ksmbd_rpc_command *rpc_resp;
>>         struct ksmbd_session_rpc *rpc;
>>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
>> -       unsigned int nbytes;
>> -       int err;
>> +       unsigned int nbytes = 0;
>> +       int ret;
>>
>>         rpc = ksmbd_session_rpc_get(work, id);
>>         if (IS_ERR(rpc)) {
>> -               err = PTR_ERR(rpc);
>> -               rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_PARAMETER);
>> +               ret = PTR_ERR(rpc);
>> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
>> +                                                STATUS_INVALID_PARAMETER);
>>                 smb2_set_err_rsp(work);
>>                 return 0;
>>         }
>>
>> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> -                                  le32_to_cpu(req->InputCount));
>> +       /* Write new input without reading past an already buffered reply. */
>> +       if (rpc->pending)
>> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
>> +       else
>> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
> This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
> cached reply is drained, the next TRANSCEIVE can return the previous
> reply without processing its own input.
> 
> Thanks.


Thanks for catching this.

I reproduced the sequence you described and confirmed the problem.

For the READ path, your suggested STATUS_BUFFER_OVERFLOW check looks
appropriate when a positive-length read leaves unread bytes from
the same reply.

For TRANSCEIVE, I think the correct fix needs some discussion.
One possibility is to return STATUS_PIPE_BUSY when cached reply
data remains, without issuing another IPC request or consuming the
cached data. The client would need to drain that reply through READ
before submitting another transaction. MS-FSCC §2.3.48[1] lists
PIPE_BUSY for a pipe containing unread data.

However, I would like to confirm that this is the intended behavior
for this SMB2 path before preparing v4. Simply omitting the WRITE
and returning cached data would still silently discard the input.

Would rejecting the transaction with STATUS_PIPE_BUSY be appropriate
here, or would you prefer a different approach?

[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/f48cecf1-98d8-48dc-926f-2f3a48f07cf6

Thanks,
Wang Zhaolong

Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Wang Zhaolong]

Hi Namjae,

>
> if (rpc->pending)
>     rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
> Should we add this check ?
>
>> +       goto done;
>>
>> +empty:
>> +       err = ksmbd_iov_pin_rsp(work, rsp,
>> +                               offsetof(struct smb2_read_rsp, Buffer));
>> +       if (err)
>> +               goto out_put;
>> +done:
>>         rsp->StructureSize = cpu_to_le16(17);
>>         rsp->DataOffset = 80;
>>         rsp->Reserved = 0;
>> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>>         rsp->hdr.Status = smb2_pipe_error_status(err,
>>                                                 STATUS_UNEXPECTED_IO_ERROR);
>>         smb2_set_err_rsp(work);
>> -       kvfree(rpc_resp);
>>         return err;
>>  }
>>
>> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>>         if (IS_ERR(rpc)) {
>>                 err = PTR_ERR(rpc);
>>                 rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_HANDLE);
>> +                                                        STATUS_INVALID_HANDLE);
>>                 smb2_set_err_rsp(work);
>>                 return err;
>>         }
>> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
>>         struct ksmbd_rpc_command *rpc_resp;
>>         struct ksmbd_session_rpc *rpc;
>>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
>> -       unsigned int nbytes;
>> -       int err;
>> +       unsigned int nbytes = 0;
>> +       int ret;
>>
>>         rpc = ksmbd_session_rpc_get(work, id);
>>         if (IS_ERR(rpc)) {
>> -               err = PTR_ERR(rpc);
>> -               rsp->hdr.Status = smb2_pipe_error_status(err,
>> -                                                       STATUS_INVALID_PARAMETER);
>> +               ret = PTR_ERR(rpc);
>> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
>> +                                                STATUS_INVALID_PARAMETER);
>>                 smb2_set_err_rsp(work);
>>                 return 0;
>>         }
>>
>> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> -                                  le32_to_cpu(req->InputCount));
>> +       /* Write new input without reading past an already buffered reply. */
>> +       if (rpc->pending)
>> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
>> +       else
>> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
>> +                                          le32_to_cpu(req->InputCount));
> This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
> cached reply is drained, the next TRANSCEIVE can return the previous
> reply without processing its own input.
>
> Thanks.


Thanks for catching this.

I reproduced the sequence you described and confirmed the problem.

For the READ path, your suggested STATUS_BUFFER_OVERFLOW check looks
appropriate when a positive-length read leaves unread bytes from
the same reply.

For TRANSCEIVE, I think the correct fix needs some discussion.
One possibility is to return STATUS_PIPE_BUSY when cached reply
data remains, without issuing another IPC request or consuming the
cached data. The client would need to drain that reply through READ
before submitting another transaction. MS-FSCC §2.3.48[1] lists
PIPE_BUSY for a pipe containing unread data.

However, I would like to confirm that this is the intended behavior
for this SMB2 path before preparing v4. Simply omitting the WRITE
and returning cached data would still silently discard the input.

Would rejecting the transaction with STATUS_PIPE_BUSY be appropriate
here, or would you prefer a different approach?

[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/f48cecf1-98d8-48dc-926f-2f3a48f07cf6

Thanks,
Wang Zhaolong

Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Namjae Jeon]

On Tue, Oct 27, 2026 at 12:20 AM Wang Zhaolong
<wangzhaolong633@gmail.com> wrote:
>
> Hi Namjae,
Hi Wang,
>
> >
> > if (rpc->pending)
> >     rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
> > Should we add this check ?
> >
> >> +       goto done;
> >>
> >> +empty:
> >> +       err = ksmbd_iov_pin_rsp(work, rsp,
> >> +                               offsetof(struct smb2_read_rsp, Buffer));
> >> +       if (err)
> >> +               goto out_put;
> >> +done:
> >>         rsp->StructureSize = cpu_to_le16(17);
> >>         rsp->DataOffset = 80;
> >>         rsp->Reserved = 0;
> >> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
> >>         rsp->hdr.Status = smb2_pipe_error_status(err,
> >>                                                 STATUS_UNEXPECTED_IO_ERROR);
> >>         smb2_set_err_rsp(work);
> >> -       kvfree(rpc_resp);
> >>         return err;
> >>  }
> >>
> >> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
> >>         if (IS_ERR(rpc)) {
> >>                 err = PTR_ERR(rpc);
> >>                 rsp->hdr.Status = smb2_pipe_error_status(err,
> >> -                                                       STATUS_INVALID_HANDLE);
> >> +                                                        STATUS_INVALID_HANDLE);
> >>                 smb2_set_err_rsp(work);
> >>                 return err;
> >>         }
> >> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
> >>         struct ksmbd_rpc_command *rpc_resp;
> >>         struct ksmbd_session_rpc *rpc;
> >>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
> >> -       unsigned int nbytes;
> >> -       int err;
> >> +       unsigned int nbytes = 0;
> >> +       int ret;
> >>
> >>         rpc = ksmbd_session_rpc_get(work, id);
> >>         if (IS_ERR(rpc)) {
> >> -               err = PTR_ERR(rpc);
> >> -               rsp->hdr.Status = smb2_pipe_error_status(err,
> >> -                                                       STATUS_INVALID_PARAMETER);
> >> +               ret = PTR_ERR(rpc);
> >> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
> >> +                                                STATUS_INVALID_PARAMETER);
> >>                 smb2_set_err_rsp(work);
> >>                 return 0;
> >>         }
> >>
> >> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> >> -                                  le32_to_cpu(req->InputCount));
> >> +       /* Write new input without reading past an already buffered reply. */
> >> +       if (rpc->pending)
> >> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
> >> +                                          le32_to_cpu(req->InputCount));
> >> +       else
> >> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> >> +                                          le32_to_cpu(req->InputCount));
> > This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
> > cached reply is drained, the next TRANSCEIVE can return the previous
> > reply without processing its own input.
> >
> > Thanks.
>
>
> Thanks for catching this.
>
> I reproduced the sequence you described and confirmed the problem.
>
> For the READ path, your suggested STATUS_BUFFER_OVERFLOW check looks
> appropriate when a positive-length read leaves unread bytes from
> the same reply.
>
> For TRANSCEIVE, I think the correct fix needs some discussion.
> One possibility is to return STATUS_PIPE_BUSY when cached reply
> data remains, without issuing another IPC request or consuming the
> cached data. The client would need to drain that reply through READ
> before submitting another transaction. MS-FSCC §2.3.48[1] lists
> PIPE_BUSY for a pipe containing unread data.
>
> However, I would like to confirm that this is the intended behavior
> for this SMB2 path before preparing v4. Simply omitting the WRITE
> and returning cached data would still silently discard the input.
>
> Would rejecting the transaction with STATUS_PIPE_BUSY be appropriate
> here, or would you prefer a different approach?
I think the former is correct.

Thanks.
>
> [1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/f48cecf1-98d8-48dc-926f-2f3a48f07cf6
>
> Thanks,
> Wang Zhaolong
>

Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Wang Zhaolong]

>>
>> Thanks for catching this.
>>
>> I reproduced the sequence you described and confirmed the problem.
>>
>> For the READ path, your suggested STATUS_BUFFER_OVERFLOW check looks
>> appropriate when a positive-length read leaves unread bytes from
>> the same reply.
>>
>> For TRANSCEIVE, I think the correct fix needs some discussion.
>> One possibility is to return STATUS_PIPE_BUSY when cached reply
>> data remains, without issuing another IPC request or consuming the
>> cached data. The client would need to drain that reply through READ
>> before submitting another transaction. MS-FSCC §2.3.48[1] lists
>> PIPE_BUSY for a pipe containing unread data.
>>
>> However, I would like to confirm that this is the intended behavior
>> for this SMB2 path before preparing v4. Simply omitting the WRITE
>> and returning cached data would still silently discard the input.
>>
>> Would rejecting the transaction with STATUS_PIPE_BUSY be appropriate
>> here, or would you prefer a different approach?
> I think the former is correct.
> 
> Thanks.

Okay, I will use the solution and send V4 after thorough testing.

Best regards,
Wang Zhaolong