[PATCH 1/2] nbd: mark the socket dead when a partial send times out

[PATCH 1/2] nbd: mark the socket dead when a partial send times out

[Joseph Qi]

nbd_pending_cmd_work() completes the request with BLK_STS_IOERR once its
retry loop passes req->deadline, but leaves nsock->pending set.  Nothing
else clears it: nbd_send_cmd() only does so at its out: label after a
complete send, and nbd_requeue_cmd() does not touch it.

Every later request on that socket then takes this branch in
nbd_handle_cmd():

	if (unlikely(nsock->pending && nsock->pending != req)) {
		nbd_requeue_cmd(cmd);

and is requeued again, forever, since the requeue path does not clear
->pending either.  If the tag is recycled first, ->pending matches the new
request instead and nbd_send_cmd() resumes it in place of the abandoned
one, skipping its header and leaving cmd_cookie alone.

Neither case leaves a usable socket.  The header went out and the rest of
the payload never will, so the stream no longer matches what the server
expects.  Mark the socket dead, which shuts it down and clears the partial
send state.  nbd_xmit_timeout() does the same for a timed out request and
only skips it here because NBD_CMD_PARTIAL_SEND makes it defer to this
work function.

Without this, a write issued after the deadline fires never completes and
the device has to be torn down to recover.

Reproduced by making the resumed send never progress, so the retry loop
runs out req->deadline.  With the socket marked dead a later request is
dispatched instead of requeued, and a reconnect afterwards does clean
O_DIRECT I/O with no oops or warning.

Fixes: 8337b029f788 ("nbd: fix partial sending")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 drivers/block/nbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ffce519bf008..c2c3dbdd631f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -838,6 +838,14 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 		/* don't bother timeout handler for partial sending */
 		if (READ_ONCE(jiffies) + msecs_to_jiffies(wait_ms) >= deadline) {
 			cmd->status = BLK_STS_IOERR;
+			/*
+			 * The header is on the wire but the rest of the payload
+			 * never will be, so the stream is out of sync with the
+			 * server.  Marking the socket dead also drops the stale
+			 * nsock->pending, which would otherwise make
+			 * nbd_handle_cmd() requeue every later request forever.
+			 */
+			nbd_mark_nsock_dead(nbd, nsock, 1);
 			blk_mq_complete_request(req);
 			break;
 		}
-- 
2.39.3

[PATCH 2/2] nbd: fix NULL pointer dereference in nbd_pending_cmd_work()

[Joseph Qi]

nbd_pending_cmd_work() turns nsock->pending into a nbd_cmd and an
nbd_device before it takes any lock:

	struct request *req = nsock->pending;
	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
	struct nbd_device *nbd = cmd->nbd;

nbd_mark_nsock_dead() clears ->pending, and it can run between
schedule_work() in nbd_sched_pending_work() and the work function
starting: recv_work() on a receive error, sock_shutdown(), or
nbd_xmit_timeout() giving up on some other command, as it defers to
this work function for the one that is partial sending.  Since
blk_mq_rq_to_pdu() is rq + 1, a NULL ->pending becomes a bogus pointer
and reading cmd->nbd oopses:

  BUG: kernel NULL pointer dereference, address: 0000000000000088
  Workqueue: events nbd_pending_cmd_work [nbd]
  RIP: 0010:nbd_pending_cmd_work+0x1b/0x120 [nbd]

The race was forced by clearing ->pending immediately before
schedule_work().  The address is sizeof(struct request), since nbd is
the first member of struct nbd_cmd and blk_mq_rq_to_pdu() is rq + 1.

cancel_work_sync() cannot close the window.  Every nbd_mark_nsock_dead()
caller holds nsock->tx_lock, nbd_xmit_timeout() holds cmd->lock as well,
and the work function takes cmd->lock before tx_lock.  There is also no
teardown point that could cancel it, since the work holds a config_refs
reference and nbd_config_put() therefore never reaches the code that
frees the socket while the work is queued.

Track the request in a field of its own that teardown does not clear, and
fail the request if the socket is gone by the time the work runs.  Its
header is already on the wire, so it cannot complete successfully.

Complete the request only once cmd->lock has been dropped, the way
nbd_xmit_timeout() does.  blk_mq_complete_request() runs nbd_complete_rq()
inline whenever blk_mq_complete_request_remote() decides not to redirect
it, and nbd_complete_rq() ends the request without taking cmd->lock, so
holding that lock does not keep cmd alive.  Anything the work function
still writes in cmd afterwards can land in a request whose tag has already
been recycled.  The deadline branch had the same problem.

Clear partial_req under tx_lock, where nbd_sched_pending_work() writes
it.  cmd->lock is per-command, so clearing it after tx_lock is dropped
would race with a store from another command and could hand the freshly
armed work a NULL request to dereference.

Also make re-arming from inside the work loop a no-op rather than a second
schedule_work().  test_and_set_bit() tells the two cases apart, and the
loop re-reads nsock->sent on its next pass, so the resumed send continues
from where it stopped.  This removes the WARN_ON_ONCE that fired on that
path and avoids taking a second config reference for a work item that is
already running.

Tested with the race forced as above: the request completes with EIO
instead of oopsing, and a reconnect afterwards does clean O_DIRECT
reads and writes.  Also re-ran the partial send happy path (64
requests, 64 work invocations, no requeues), the deadline path, a
reconnect with 256 MiB of O_DIRECT writes and reads, and rmmod, all
with no oops, warning or leaked config reference.

Fixes: 8337b029f788 ("nbd: fix partial sending")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 drivers/block/nbd.c | 53 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c2c3dbdd631f..7b9c0de23201 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -63,6 +63,7 @@ struct nbd_sock {
 	int fallback_index;
 	int cookie;
 	struct work_struct work;
+	struct request *partial_req;
 };
 
 struct recv_thread_args {
@@ -637,12 +638,24 @@ static void nbd_sched_pending_work(struct nbd_device *nbd,
 {
 	struct request *req = blk_mq_rq_from_pdu(cmd);
 
-	/* pending work should be scheduled only once */
-	WARN_ON_ONCE(test_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags));
-
 	nsock->pending = req;
 	nsock->sent = sent;
-	set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
+
+	/*
+	 * Already armed: this is nbd_pending_cmd_work() re-entering because the
+	 * resumed send was interrupted again.  Its work is still running, so
+	 * just refresh the resume point above and let its loop pick it up
+	 * instead of taking another config reference and requeueing the work.
+	 */
+	if (test_and_set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags))
+		return;
+
+	/*
+	 * nbd_mark_nsock_dead() clears ->pending, so the work function cannot
+	 * rely on it to find the request it owns.  Keep a copy that only it
+	 * clears.
+	 */
+	nsock->partial_req = req;
 	refcount_inc(&nbd->config_refs);
 	schedule_work(&nsock->work);
 }
@@ -817,11 +830,12 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
 static void nbd_pending_cmd_work(struct work_struct *work)
 {
 	struct nbd_sock *nsock = container_of(work, struct nbd_sock, work);
-	struct request *req = nsock->pending;
+	struct request *req = nsock->partial_req;
 	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
 	struct nbd_device *nbd = cmd->nbd;
 	unsigned long deadline = READ_ONCE(req->deadline);
 	unsigned int wait_ms = 2;
+	bool complete = false;
 
 	mutex_lock(&cmd->lock);
 
@@ -830,6 +844,18 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 		goto out;
 
 	mutex_lock(&nsock->tx_lock);
+	/*
+	 * nbd_mark_nsock_dead() can tear the socket down between schedule_work()
+	 * and here, and it clears ->pending and ->sent.  The header is already
+	 * on the wire so this request can never be answered; fail it rather than
+	 * resuming a send on a socket that is gone.
+	 */
+	if (!nsock->pending) {
+		cmd->status = BLK_STS_IOERR;
+		__clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
+		complete = true;
+		goto unlock;
+	}
 	while (true) {
 		nbd_send_cmd(nbd, cmd, cmd->index);
 		if (!nsock->pending)
@@ -846,17 +872,32 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 			 * nbd_handle_cmd() requeue every later request forever.
 			 */
 			nbd_mark_nsock_dead(nbd, nsock, 1);
-			blk_mq_complete_request(req);
+			complete = true;
 			break;
 		}
 		msleep(wait_ms);
 		wait_ms *= 2;
 	}
+unlock:
+	/*
+	 * nbd_sched_pending_work() writes partial_req under tx_lock, and
+	 * cmd->lock is per-command, so this has to be under tx_lock too.
+	 */
+	nsock->partial_req = NULL;
 	mutex_unlock(&nsock->tx_lock);
 	clear_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
 out:
 	mutex_unlock(&cmd->lock);
 	nbd_config_put(nbd);
+
+	/*
+	 * Complete last.  blk_mq_complete_request() runs nbd_complete_rq()
+	 * inline when blk_mq_complete_request_remote() does not redirect it,
+	 * and nbd_complete_rq() ends the request without taking cmd->lock, so
+	 * cmd must not be touched again after this point.
+	 */
+	if (complete)
+		blk_mq_complete_request(req);
 }
 
 static int nbd_read_reply(struct nbd_device *nbd, struct socket *sock,
-- 
2.39.3