[v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

[PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Caleb Sander Mateos 5 days, 10 hours ago

nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
checks that the return value from recvmsg() is non-negative. If the
sender closes the TCP connection or sends fewer than 128 bytes, this
check will pass even though the full PDU wasn't received.

Ensure the full ICResp PDU is received by checking that recvmsg()
returns the expected 128 bytes.

Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
split the ICResp over multiple TCP frames. Without MSG_WAITALL,
recvmsg() could return prematurely with only part of the PDU.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
---
v4: keep recvmsg() error return value
v3: fix return value to indicate error
v2: add Fixes tag

 drivers/nvme/host/tcp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index e9ff6babc540..56679eb8c0d6 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 	iov.iov_len = sizeof(*icresp);
 	if (nvme_tcp_queue_tls(queue)) {
 		msg.msg_control = cbuf;
 		msg.msg_controllen = sizeof(cbuf);
 	}
+	msg.msg_flags = MSG_WAITALL;
 	ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
 			iov.iov_len, msg.msg_flags);
-	if (ret < 0) {
+	if (ret < sizeof(*icresp)) {
 		pr_warn("queue %d: failed to receive icresp, error %d\n",
 			nvme_tcp_queue_id(queue), ret);
+		if (ret >= 0)
+			ret = -ECONNRESET;
 		goto free_icresp;
 	}
 	ret = -ENOTCONN;
 	if (nvme_tcp_queue_tls(queue)) {
 		ctype = tls_get_record_type(queue->sock->sk,
-- 
2.45.2

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Hannes Reinecke 2 days, 21 hours ago

On 1/24/25 19:43, Caleb Sander Mateos wrote:
> nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
> checks that the return value from recvmsg() is non-negative. If the
> sender closes the TCP connection or sends fewer than 128 bytes, this
> check will pass even though the full PDU wasn't received.
> 
> Ensure the full ICResp PDU is received by checking that recvmsg()
> returns the expected 128 bytes.
> 
> Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
> split the ICResp over multiple TCP frames. Without MSG_WAITALL,
> recvmsg() could return prematurely with only part of the PDU.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
> ---
> v4: keep recvmsg() error return value
> v3: fix return value to indicate error
> v2: add Fixes tag
> 
>   drivers/nvme/host/tcp.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index e9ff6babc540..56679eb8c0d6 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>   	iov.iov_len = sizeof(*icresp);
>   	if (nvme_tcp_queue_tls(queue)) {
>   		msg.msg_control = cbuf;
>   		msg.msg_controllen = sizeof(cbuf);
>   	}
> +	msg.msg_flags = MSG_WAITALL;
>   	ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>   			iov.iov_len, msg.msg_flags);

But won't we have to wait for a TCP timeout now if the sender sends less 
than 128 bytes? With this patch we always wait for 128 bytes, and
possibly wait for TCP timeout if not.
Testcase for this would be nice ...

And I need to check if secure concatenation is affected here; with
secure concatenation we need to peek at the first packet to check
if it's an ICRESP or a TLS negotiation.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Caleb Sander 2 days, 11 hours ago

On Sun, Jan 26, 2025 at 11:37 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 1/24/25 19:43, Caleb Sander Mateos wrote:
> > nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
> > checks that the return value from recvmsg() is non-negative. If the
> > sender closes the TCP connection or sends fewer than 128 bytes, this
> > check will pass even though the full PDU wasn't received.
> >
> > Ensure the full ICResp PDU is received by checking that recvmsg()
> > returns the expected 128 bytes.
> >
> > Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
> > split the ICResp over multiple TCP frames. Without MSG_WAITALL,
> > recvmsg() could return prematurely with only part of the PDU.
> >
> > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
> > ---
> > v4: keep recvmsg() error return value
> > v3: fix return value to indicate error
> > v2: add Fixes tag
> >
> >   drivers/nvme/host/tcp.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index e9ff6babc540..56679eb8c0d6 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
> >       iov.iov_len = sizeof(*icresp);
> >       if (nvme_tcp_queue_tls(queue)) {
> >               msg.msg_control = cbuf;
> >               msg.msg_controllen = sizeof(cbuf);
> >       }
> > +     msg.msg_flags = MSG_WAITALL;
> >       ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> >                       iov.iov_len, msg.msg_flags);
>
> But won't we have to wait for a TCP timeout now if the sender sends less
> than 128 bytes? With this patch we always wait for 128 bytes, and
> possibly wait for TCP timeout if not.

Yes, if the NVMe/TCP controller sends less than 128 bytes, we need to
wait for it to send the remainder of the ICResp PDU. That's just how
the NVMe/TCP protocol works. If we want to protect against
buggy/malicious controllers that don't send a full ICResp, we need a
timeout mechanism. That's the purpose of the existing
`queue->sock->sk->sk_rcvtimeo = 10 * HZ;` in nvme_tcp_alloc_queue().
Note that recvmsg() timing out was already possible in the original
code if the controller didn't send anything on the TCP connection
after accepting it.

> Testcase for this would be nice ...
>
> And I need to check if secure concatenation is affected here; with
> secure concatenation we need to peek at the first packet to check
> if it's an ICRESP or a TLS negotiation.

Are you saying that with secure concatenation we don't know in advance
whether the connection is using TLS between the TCP and NVMe/TCP
protocol layers? Wouldn't the host already need to know that when it
sent its ICReq PDU?
If TLS is being used, my assumption was that `struct kvec iov` will
receive the decrypted (NVMe/TCP protocol) bytes rather than the
encrypted (TLS protocol) bytes. If that's not the case, then I think
there would be another existing bug, as the code interprets the
received bytes as an ICResp PDU regardless of whether TLS is in use.

Thanks,
Caleb

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Hannes Reinecke 1 day, 20 hours ago

On 1/27/25 18:38, Caleb Sander wrote:
> On Sun, Jan 26, 2025 at 11:37 PM Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 1/24/25 19:43, Caleb Sander Mateos wrote:
>>> nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
>>> checks that the return value from recvmsg() is non-negative. If the
>>> sender closes the TCP connection or sends fewer than 128 bytes, this
>>> check will pass even though the full PDU wasn't received.
>>>
>>> Ensure the full ICResp PDU is received by checking that recvmsg()
>>> returns the expected 128 bytes.
>>>
>>> Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
>>> split the ICResp over multiple TCP frames. Without MSG_WAITALL,
>>> recvmsg() could return prematurely with only part of the PDU.
>>>
>>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>>> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
>>> ---
>>> v4: keep recvmsg() error return value
>>> v3: fix return value to indicate error
>>> v2: add Fixes tag
>>>
>>>    drivers/nvme/host/tcp.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index e9ff6babc540..56679eb8c0d6 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
>>>        iov.iov_len = sizeof(*icresp);
>>>        if (nvme_tcp_queue_tls(queue)) {
>>>                msg.msg_control = cbuf;
>>>                msg.msg_controllen = sizeof(cbuf);
>>>        }
>>> +     msg.msg_flags = MSG_WAITALL;
>>>        ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>>>                        iov.iov_len, msg.msg_flags);
>>
>> But won't we have to wait for a TCP timeout now if the sender sends less
>> than 128 bytes? With this patch we always wait for 128 bytes, and
>> possibly wait for TCP timeout if not.
> 
> Yes, if the NVMe/TCP controller sends less than 128 bytes, we need to
> wait for it to send the remainder of the ICResp PDU. That's just how
> the NVMe/TCP protocol works. If we want to protect against
> buggy/malicious controllers that don't send a full ICResp, we need a
> timeout mechanism. That's the purpose of the existing
> `queue->sock->sk->sk_rcvtimeo = 10 * HZ;` in nvme_tcp_alloc_queue().
> Note that recvmsg() timing out was already possible in the original
> code if the controller didn't send anything on the TCP connection
> after accepting it.
> 
Hmm. With checking the code 'rcvtimeo' is only evaluated if MSG_WAITALL
is _not_ set. Makes me wonder why we do set it...
But that's beside the point.

>> Testcase for this would be nice ...
>>
>> And I need to check if secure concatenation is affected here; with
>> secure concatenation we need to peek at the first packet to check
>> if it's an ICRESP or a TLS negotiation.
> 
> Are you saying that with secure concatenation we don't know in advance
> whether the connection is using TLS between the TCP and NVMe/TCP
> protocol layers? Wouldn't the host already need to know that when it
> sent its ICReq PDU?

No, the host doesn't need to know. TLS is enabled by the lower
layers.

But upon further checking, I guess it'll be okay with secure
concatenation.

Nevertheless, I would vastly prefer to have a receive loop here
instead of waiting to receive the full amount as per MSG_WAITALL.
The entire tcp code is using nonblocking calls, so I'd rather
keep it that way and implement a receive loop here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Caleb Sander 1 day, 6 hours ago

On Tue, Jan 28, 2025 at 12:28 AM Hannes Reinecke <hare@suse.de> wrote:
>
> On 1/27/25 18:38, Caleb Sander wrote:
> > On Sun, Jan 26, 2025 at 11:37 PM Hannes Reinecke <hare@suse.de> wrote:
> >>
> >> On 1/24/25 19:43, Caleb Sander Mateos wrote:
> >>> nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
> >>> checks that the return value from recvmsg() is non-negative. If the
> >>> sender closes the TCP connection or sends fewer than 128 bytes, this
> >>> check will pass even though the full PDU wasn't received.
> >>>
> >>> Ensure the full ICResp PDU is received by checking that recvmsg()
> >>> returns the expected 128 bytes.
> >>>
> >>> Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
> >>> split the ICResp over multiple TCP frames. Without MSG_WAITALL,
> >>> recvmsg() could return prematurely with only part of the PDU.
> >>>
> >>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> >>> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
> >>> ---
> >>> v4: keep recvmsg() error return value
> >>> v3: fix return value to indicate error
> >>> v2: add Fixes tag
> >>>
> >>>    drivers/nvme/host/tcp.c | 5 ++++-
> >>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> >>> index e9ff6babc540..56679eb8c0d6 100644
> >>> --- a/drivers/nvme/host/tcp.c
> >>> +++ b/drivers/nvme/host/tcp.c
> >>> @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
> >>>        iov.iov_len = sizeof(*icresp);
> >>>        if (nvme_tcp_queue_tls(queue)) {
> >>>                msg.msg_control = cbuf;
> >>>                msg.msg_controllen = sizeof(cbuf);
> >>>        }
> >>> +     msg.msg_flags = MSG_WAITALL;
> >>>        ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> >>>                        iov.iov_len, msg.msg_flags);
> >>
> >> But won't we have to wait for a TCP timeout now if the sender sends less
> >> than 128 bytes? With this patch we always wait for 128 bytes, and
> >> possibly wait for TCP timeout if not.
> >
> > Yes, if the NVMe/TCP controller sends less than 128 bytes, we need to
> > wait for it to send the remainder of the ICResp PDU. That's just how
> > the NVMe/TCP protocol works. If we want to protect against
> > buggy/malicious controllers that don't send a full ICResp, we need a
> > timeout mechanism. That's the purpose of the existing
> > `queue->sock->sk->sk_rcvtimeo = 10 * HZ;` in nvme_tcp_alloc_queue().
> > Note that recvmsg() timing out was already possible in the original
> > code if the controller didn't send anything on the TCP connection
> > after accepting it.
> >
> Hmm. With checking the code 'rcvtimeo' is only evaluated if MSG_WAITALL
> is _not_ set. Makes me wonder why we do set it...
> But that's beside the point.

I am not seeing where sk_rcvtimeo is ignored when MSG_WAITALL is used,
can you point me to the code?
It is certainly ignored for *non-blocking* recvmsg() (MSG_DONTWAIT),
but I don't see why it would be ignored for MSG_WAITALL. man 7 socket
also suggests it applies to all blocking socket I/O.

static inline long sock_rcvtimeo(const struct sock *sk, bool noblock)
{
        return noblock ? 0 : sk->sk_rcvtimeo;
}

In tcp_recvmsg_locked():
timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);

>
> >> Testcase for this would be nice ...
> >>
> >> And I need to check if secure concatenation is affected here; with
> >> secure concatenation we need to peek at the first packet to check
> >> if it's an ICRESP or a TLS negotiation.
> >
> > Are you saying that with secure concatenation we don't know in advance
> > whether the connection is using TLS between the TCP and NVMe/TCP
> > protocol layers? Wouldn't the host already need to know that when it
> > sent its ICReq PDU?
>
> No, the host doesn't need to know. TLS is enabled by the lower
> layers.
>
> But upon further checking, I guess it'll be okay with secure
> concatenation.
>
> Nevertheless, I would vastly prefer to have a receive loop here
> instead of waiting to receive the full amount as per MSG_WAITALL.
> The entire tcp code is using nonblocking calls, so I'd rather
> keep it that way and implement a receive loop here.

The driver uses non-blocking socket I/O in the I/O path, but not the
connect path. nvme_tcp_init_connection() is already using blocking
socket I/O to send the ICReq PDU and receive the ICResp. I suppose we
could change the connect path to register poll interest and use
non-blocking operations, but that seems like a more involved code
change and orthogonal to the issue of receiving the full ICResp.

Best,
Caleb

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Maurizio Lombardi 2 days, 11 hours ago

po 27. 1. 2025 v 18:39 odesílatel Caleb Sander <csander@purestorage.com> napsal:
> > > +     msg.msg_flags = MSG_WAITALL;
> > >       ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> > >                       iov.iov_len, msg.msg_flags);
> >
> > But won't we have to wait for a TCP timeout now if the sender sends less
> > than 128 bytes? With this patch we always wait for 128 bytes, and
> > possibly wait for TCP timeout if not.
>
> Yes, if the NVMe/TCP controller sends less than 128 bytes, we need to
> wait for it to send the remainder of the ICResp PDU.

I guess what Hannes wanted to ask is whether it makes sense to
check if ret is a number greater than 0 but less than 128,
given that the MSG_WAITALL flag has been set.

Maurizio

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Hannes Reinecke 2 days, 20 hours ago

On 1/27/25 08:37, Hannes Reinecke wrote:
> On 1/24/25 19:43, Caleb Sander Mateos wrote:
>> nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
>> checks that the return value from recvmsg() is non-negative. If the
>> sender closes the TCP connection or sends fewer than 128 bytes, this
>> check will pass even though the full PDU wasn't received.
>>
>> Ensure the full ICResp PDU is received by checking that recvmsg()
>> returns the expected 128 bytes.
>>
>> Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
>> split the ICResp over multiple TCP frames. Without MSG_WAITALL,
>> recvmsg() could return prematurely with only part of the PDU.
>>
>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
>> ---
>> v4: keep recvmsg() error return value
>> v3: fix return value to indicate error
>> v2: add Fixes tag
>>
>>   drivers/nvme/host/tcp.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index e9ff6babc540..56679eb8c0d6 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct 
>> nvme_tcp_queue *queue)
>>       iov.iov_len = sizeof(*icresp);
>>       if (nvme_tcp_queue_tls(queue)) {
>>           msg.msg_control = cbuf;
>>           msg.msg_controllen = sizeof(cbuf);
>>       }
>> +    msg.msg_flags = MSG_WAITALL;
>>       ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>>               iov.iov_len, msg.msg_flags);
> 
> But won't we have to wait for a TCP timeout now if the sender sends less 
> than 128 bytes? With this patch we always wait for 128 bytes, and
> possibly wait for TCP timeout if not.
> Testcase for this would be nice ...
> 
> And I need to check if secure concatenation is affected here; with
> secure concatenation we need to peek at the first packet to check
> if it's an ICRESP or a TLS negotiation.
> 
And wouldn't this patch be sufficient here?

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 841238f38fdd..85b1328a8757 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1451,12 +1451,13 @@ static int nvme_tcp_init_connection(struct 
nvme_tcp_queue *queue)
         }
         ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
                         iov.iov_len, msg.msg_flags);
+       if (ret == 0)
+               ret = -ENOTCONN;
         if (ret < 0) {
                 pr_warn("queue %d: failed to receive icresp, error %d\n",
                         nvme_tcp_queue_id(queue), ret);
                 goto free_icresp;
         }
-       ret = -ENOTCONN;
         if (nvme_tcp_queue_tls(queue)) {
                 ctype = tls_get_record_type(queue->sock->sk,
                                             (struct cmsghdr *)cbuf);

icresp validity is checked later in the code, so if we haven't received
a full icresp we _should_ fail those tests. And then we don't really
have to check how many bytes we've received.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Caleb Sander 2 days, 10 hours ago

On Mon, Jan 27, 2025 at 12:09 AM Hannes Reinecke <hare@suse.de> wrote:
>
> On 1/27/25 08:37, Hannes Reinecke wrote:
> > On 1/24/25 19:43, Caleb Sander Mateos wrote:
> >> nvme_tcp_init_connection() attempts to receive an ICResp PDU but only
> >> checks that the return value from recvmsg() is non-negative. If the
> >> sender closes the TCP connection or sends fewer than 128 bytes, this
> >> check will pass even though the full PDU wasn't received.
> >>
> >> Ensure the full ICResp PDU is received by checking that recvmsg()
> >> returns the expected 128 bytes.
> >>
> >> Additionally set the MSG_WAITALL flag for recvmsg(), as a sender could
> >> split the ICResp over multiple TCP frames. Without MSG_WAITALL,
> >> recvmsg() could return prematurely with only part of the PDU.
> >>
> >> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> >> Fixes: 3f2304f8c6d6 ("nvme-tcp: add NVMe over TCP host driver")
> >> ---
> >> v4: keep recvmsg() error return value
> >> v3: fix return value to indicate error
> >> v2: add Fixes tag
> >>
> >>   drivers/nvme/host/tcp.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> >> index e9ff6babc540..56679eb8c0d6 100644
> >> --- a/drivers/nvme/host/tcp.c
> >> +++ b/drivers/nvme/host/tcp.c
> >> @@ -1446,15 +1446,18 @@ static int nvme_tcp_init_connection(struct
> >> nvme_tcp_queue *queue)
> >>       iov.iov_len = sizeof(*icresp);
> >>       if (nvme_tcp_queue_tls(queue)) {
> >>           msg.msg_control = cbuf;
> >>           msg.msg_controllen = sizeof(cbuf);
> >>       }
> >> +    msg.msg_flags = MSG_WAITALL;
> >>       ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
> >>               iov.iov_len, msg.msg_flags);
> >
> > But won't we have to wait for a TCP timeout now if the sender sends less
> > than 128 bytes? With this patch we always wait for 128 bytes, and
> > possibly wait for TCP timeout if not.
> > Testcase for this would be nice ...
> >
> > And I need to check if secure concatenation is affected here; with
> > secure concatenation we need to peek at the first packet to check
> > if it's an ICRESP or a TLS negotiation.
> >
> And wouldn't this patch be sufficient here?

If the controller sends the ICResp across multiple TCP packets, the
recvmsg() will only receive the first one. There are 2 different
problematic outcomes:
- The validation of the icresp buffer may fail because it hasn't been
fully initialized from the PDU
- Even if the validation passes, the remaining bytes of the PDU are
still in the socket buffer. Subsequent receive operations will return
those bytes instead of the following PDUs they meant to receive.

It is important to receive the full ICResp to avoid connection
establishment failure, and to ensure there are no remaining bytes in
the socket buffer.

>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 841238f38fdd..85b1328a8757 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1451,12 +1451,13 @@ static int nvme_tcp_init_connection(struct
> nvme_tcp_queue *queue)
>          }
>          ret = kernel_recvmsg(queue->sock, &msg, &iov, 1,
>                          iov.iov_len, msg.msg_flags);
> +       if (ret == 0)
> +               ret = -ENOTCONN;

It is possible for ret to be positive but less than sizeof(ICResp).
For example, a controller is allowed to send the 8-byte Common Header
and the 120-byte ICResp PDU Specific Header in separate TCP packets.
In that case, recvmsg() would only receive the Common Header and
return 8. We need to receive the full 128 bytes of the ICResp PDU
before processing it and receiving subsequent PDUs from the socket.

>          if (ret < 0) {
>                  pr_warn("queue %d: failed to receive icresp, error %d\n",
>                          nvme_tcp_queue_id(queue), ret);
>                  goto free_icresp;
>          }
> -       ret = -ENOTCONN;
>          if (nvme_tcp_queue_tls(queue)) {
>                  ctype = tls_get_record_type(queue->sock->sk,
>                                              (struct cmsghdr *)cbuf);
>
> icresp validity is checked later in the code, so if we haven't received
> a full icresp we _should_ fail those tests. And then we don't really
> have to check how many bytes we've received.

The ICResp is only validated up to the MAXH2CDATA field. The
validation would fail if that field is 0, so it is true that we would
catch a recvmsg() that received 12 bytes or fewer. However, the
remaining 115 bytes of the ICResp PDU are not validated, so a passing
validation does not ensure all 128 bytes were received from the
socket.

Thanks,
Caleb

Re: [PATCH v4] nvme-tcp: fix connect failure on receiving partial ICResp PDU

Posted by Sagi Grimberg 3 days, 20 hours ago

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>