[PATCH] nvme-tcp: fix possible data corruption with bio merges

[PATCH] nvme-tcp: fix possible data corruption with bio merges

[Sagi Grimberg]

When a bio merges, we can get a request that spans multiple
bios, and the overall request payload size is the sum of
all bios. When we calculate how much we need to send
from the existing bio (and bvec), we did not take into
account the iov_iter byte count cap.

Since multipage bvecs support, bvecs can split in the middle
which means that when we account for the last bvec send we
should also take the iov_iter byte count cap as it might be
lower than the last bvec size.

Reported-by: Hao Wang <pkuwangh@gmail.com>
Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
Tested-by: Hao Wang <pkuwangh@gmail.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/tcp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 979ee31b8dd1..ad0332477986 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -201,8 +201,9 @@ static inline size_t nvme_tcp_req_cur_offset(struct nvme_tcp_request *req)
 
 static inline size_t nvme_tcp_req_cur_length(struct nvme_tcp_request *req)
 {
-	return min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
-			req->pdu_len - req->pdu_sent);
+	return min_t(size_t, req->iter.count,
+			min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
+				req->pdu_len - req->pdu_sent));
 }
 
 static inline size_t nvme_tcp_req_offset(struct nvme_tcp_request *req)
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-tcp: fix possible data corruption with bio merges

[Christoph Hellwig]

>  static inline size_t nvme_tcp_req_cur_length(struct nvme_tcp_request *req)
>  {
> -	return min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
> -			req->pdu_len - req->pdu_sent);
> +	return min_t(size_t, req->iter.count,
> +			min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
> +				req->pdu_len - req->pdu_sent));

We are not supposed to directly poke into an iov_iter.

I suspect this should be something like:

	return min_t(size_t, iov_iter_single_seg_count(&req->iter),
		     req->pdu_len - req->pdu_sent);

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-tcp: fix possible data corruption with bio merges

[Sagi Grimberg]

>>   static inline size_t nvme_tcp_req_cur_length(struct nvme_tcp_request *req)
>>   {
>> -	return min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
>> -			req->pdu_len - req->pdu_sent);
>> +	return min_t(size_t, req->iter.count,
>> +			min_t(size_t, req->iter.bvec->bv_len - req->iter.iov_offset,
>> +				req->pdu_len - req->pdu_sent));
> 
> We are not supposed to directly poke into an iov_iter.
> 
> I suspect this should be something like:
> 
> 	return min_t(size_t, iov_iter_single_seg_count(&req->iter),
> 		     req->pdu_len - req->pdu_sent);

You are absolutely right, will send out a v2. thanks.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme