[PATCH 55/98] nvme-tcp: don't act on PDUs for a command being sent

[Jens Axboe <axboe@kernel.dk>]

From: Josef Bacik <josef@toxicpanda.com>

A request belongs to the send path from the moment it's put on req_list
until its last byte is handed to the socket, but the receive path acts
on whatever the controller sends for it. A response completes the
request while the send side is still reading from its bio:

  BUG: KASAN: slab-use-after-free in nvme_tcp_try_send_data+0x175/0x6b0
   nvme_tcp_try_send+0x4af/0x600
   nvme_tcp_io_work+0x249/0x380

and an R2T resets and requeues it, since the queued check only looks at
the two lists and a request being sent is on neither, so it ends up
completed twice. An honest controller doesn't do that, but its answer
can legitimately overtake a sender that has handed over the last byte
and not yet cleared queue->request.

Make such a PDU wait: don't consume the bytes that complete its header,
so it's seen again by the next receive. For that nvme_tcp_req_busy()
has to find the request in one of the three places at all times, so
nvme_tcp_fetch_request() sets queue->request before taking it off the
lists. io_work reruns when the sender was io_work or the socket was
full, waits for send_mutex when the submitter was sending, and a poller
just polls again.

Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
Cc: Keith Busch <kbusch@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/tcp.c | 87 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 73 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 64f563686938..06c205703cf5 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -123,6 +123,7 @@ struct nvme_tcp_queue {
 	size_t			data_remaining;
 	size_t			ddgst_remaining;
 	unsigned int		nr_cqe;
+	bool			rcv_parked;
 
 	/* send state */
 	struct nvme_tcp_request *request;
@@ -465,11 +466,29 @@ nvme_tcp_fetch_request(struct nvme_tcp_queue *queue)
 			return NULL;
 	}
 
+	WRITE_ONCE(queue->request, req);
+	/* nvme_tcp_req_busy() must find the request in one place at any time */
+	smp_wmb();
 	list_del_init(&req->entry);
 	init_llist_node(&req->lentry);
 	return req;
 }
 
+/*
+ * Does the send path still hold @req? As long as it does the request must
+ * neither be completed nor be queued once more, whatever the controller says.
+ */
+static bool nvme_tcp_req_busy(struct nvme_tcp_queue *queue,
+			      struct nvme_tcp_request *req)
+{
+	if (llist_on_list(&req->lentry) || !list_empty(&req->entry))
+		return true;
+	/* pairs with the barrier in nvme_tcp_fetch_request() */
+	smp_rmb();
+	/* pairs with the release in nvme_tcp_done_send_req() */
+	return smp_load_acquire(&queue->request) == req;
+}
+
 #define NVME_TCP_CRC_SEED (~0)
 
 static inline void nvme_tcp_ddgst_update(u32 *crcp,
@@ -672,6 +691,9 @@ static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
 	}
 
 	req = blk_mq_rq_to_pdu(rq);
+	if (unlikely(nvme_tcp_req_busy(queue, req)))
+		return -EBUSY;
+
 	if (req->status == cpu_to_le16(NVME_SC_SUCCESS))
 		req->status = cqe->status;
 
@@ -707,6 +729,9 @@ static int nvme_tcp_handle_c2h_data(struct nvme_tcp_queue *queue,
 	}
 
 	req = blk_mq_rq_to_pdu(rq);
+	if (unlikely(nvme_tcp_req_busy(queue, req)))
+		return -EBUSY;
+
 	if (!blk_rq_payload_bytes(rq) || !req->curr_bio || !req->data_len) {
 		dev_err(queue->ctrl->ctrl.device,
 			"queue %d tag %#x unexpected data\n",
@@ -800,6 +825,8 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue,
 		return -ENOENT;
 	}
 	req = blk_mq_rq_to_pdu(rq);
+	if (unlikely(nvme_tcp_req_busy(queue, req)))
+		return -EBUSY;
 
 	if (unlikely(rq_data_dir(rq) != WRITE)) {
 		dev_err(queue->ctrl->ctrl.device,
@@ -829,14 +856,6 @@ static int nvme_tcp_handle_r2t(struct nvme_tcp_queue *queue,
 		return -EPROTO;
 	}
 
-	if (llist_on_list(&req->lentry) ||
-	    !list_empty(&req->entry)) {
-		dev_err(queue->ctrl->ctrl.device,
-			"req %d unexpected r2t while processing request\n",
-			rq->tag);
-		return -EPROTO;
-	}
-
 	req->pdu_len = 0;
 	req->h2cdata_left = r2t_length;
 	req->h2cdata_offset = r2t_offset;
@@ -939,17 +958,32 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb,
 
 	switch (hdr->type) {
 	case nvme_tcp_c2h_data:
-		return nvme_tcp_handle_c2h_data(queue, (void *)queue->pdu);
+		ret = nvme_tcp_handle_c2h_data(queue, (void *)queue->pdu);
+		break;
 	case nvme_tcp_rsp:
-		nvme_tcp_init_recv_ctx(queue);
-		return nvme_tcp_handle_comp(queue, (void *)queue->pdu);
+		ret = nvme_tcp_handle_comp(queue, (void *)queue->pdu);
+		break;
 	case nvme_tcp_r2t:
-		nvme_tcp_init_recv_ctx(queue);
-		return nvme_tcp_handle_r2t(queue, (void *)queue->pdu);
+		ret = nvme_tcp_handle_r2t(queue, (void *)queue->pdu);
+		break;
 	default:
 		goto unsupported_pdu;
 	}
 
+	if (unlikely(ret == -EBUSY)) {
+		/*
+		 * The command is still being sent. Leave the end of the header
+		 * in the socket and look at it again when that is done.
+		 */
+		queue->pdu_remaining += rcv_len;
+		queue->pdu_offset -= rcv_len;
+		*offset -= rcv_len;
+		*len += rcv_len;
+	} else if (hdr->type != nvme_tcp_c2h_data) {
+		nvme_tcp_init_recv_ctx(queue);
+	}
+	return ret;
+
 unsupported_pdu:
 	dev_err(queue->ctrl->ctrl.device,
 		"unsupported pdu type (%d)\n", hdr->type);
@@ -1110,6 +1144,10 @@ static int nvme_tcp_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
 		default:
 			result = -EFAULT;
 		}
+		if (unlikely(result == -EBUSY)) {
+			queue->rcv_parked = true;
+			return consumed - len;
+		}
 		if (result) {
 			dev_err(queue->ctrl->ctrl.device,
 				"receive failed:  %d\n", result);
@@ -1182,7 +1220,8 @@ static void nvme_tcp_state_change(struct sock *sk)
 
 static inline void nvme_tcp_done_send_req(struct nvme_tcp_queue *queue)
 {
-	queue->request = NULL;
+	/* pairs with nvme_tcp_req_busy() */
+	smp_store_release(&queue->request, NULL);
 }
 
 static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
@@ -1430,6 +1469,7 @@ static int nvme_tcp_read_sock(struct nvme_tcp_queue *queue)
 	rd_desc.count = 1;
 	lock_sock(sk);
 	queue->nr_cqe = 0;
+	queue->rcv_parked = false;
 	consumed = sock->ops->read_sock(sk, &rd_desc, nvme_tcp_recv_skb);
 	release_sock(sk);
 	return consumed;
@@ -1527,6 +1567,22 @@ static int nvme_tcp_try_recv(struct nvme_tcp_queue *queue)
 	return consumed == -EAGAIN ? 0 : consumed;
 }
 
+/*
+ * A PDU has been left in the socket because its command is still being sent.
+ * A sender in another context does not wake us when it is done: wait for it.
+ * If there is something left to send then, either the socket is full and tells
+ * us when it has room, or io_work has been queued.
+ */
+static bool nvme_tcp_recv_again(struct nvme_tcp_queue *queue)
+{
+	bool again;
+
+	mutex_lock(&queue->send_mutex);
+	again = !queue->request && !nvme_tcp_queue_has_pending(queue);
+	mutex_unlock(&queue->send_mutex);
+	return again;
+}
+
 static void nvme_tcp_io_work(struct work_struct *w)
 {
 	struct nvme_tcp_queue *queue =
@@ -1552,6 +1608,9 @@ static void nvme_tcp_io_work(struct work_struct *w)
 		else if (unlikely(result < 0))
 			return;
 
+		if (unlikely(queue->rcv_parked) && nvme_tcp_recv_again(queue))
+			pending = true;
+
 		/* did we get some space after spending time in recv? */
 		if (nvme_tcp_queue_has_pending(queue) &&
 		    sk_stream_is_writeable(queue->sock->sk))
-- 
2.56.0