[PATCH] nvme-tcp: check for allocated queue in nvme_tcp_get_address()

[PATCH] nvme-tcp: check for allocated queue in nvme_tcp_get_address()

[Hannes Reinecke]

We need to check if the queue is allocated, otherwise we get
a kernel crash as 'queue->sock' is not allocated.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/tcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 4ddf3e7b252a..487ea3e8b6c4 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2859,6 +2859,9 @@ static int nvme_tcp_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
 	struct sockaddr_storage src_addr;
 	int ret, len;
 
+	if (!test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
+		return -EINVAL;
+
 	len = nvmf_get_address(ctrl, buf, size);
 
 	ret = kernel_getsockname(queue->sock, (struct sockaddr *)&src_addr);
-- 
2.35.3

Re: [PATCH] nvme-tcp: check for allocated queue in nvme_tcp_get_address()

[Christoph Hellwig]

On Tue, Feb 07, 2023 at 03:55:48PM +0100, Hannes Reinecke wrote:
> We need to check if the queue is allocated, otherwise we get
> a kernel crash as 'queue->sock' is not allocated.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/host/tcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4ddf3e7b252a..487ea3e8b6c4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2859,6 +2859,9 @@ static int nvme_tcp_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
>  	struct sockaddr_storage src_addr;
>  	int ret, len;
>  
> +	if (!test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
> +		return -EINVAL;

I'm not sure this is enough.  The queue could be freed right after
this check.  I think we have to cache the address information
somewhere instead and ensure that it is always valid.

Re: [PATCH] nvme-tcp: check for allocated queue in nvme_tcp_get_address()

[Sagi Grimberg]

>> We need to check if the queue is allocated, otherwise we get
>> a kernel crash as 'queue->sock' is not allocated.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/tcp.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 4ddf3e7b252a..487ea3e8b6c4 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2859,6 +2859,9 @@ static int nvme_tcp_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
>>   	struct sockaddr_storage src_addr;
>>   	int ret, len;
>>   
>> +	if (!test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
>> +		return -EINVAL;
> 
> I'm not sure this is enough.  The queue could be freed right after
> this check.  I think we have to cache the address information
> somewhere instead and ensure that it is always valid.

The destination address is cached in ctrl->opts (passed to the host).

What is referenced afterwards is the socket src_addr of the admin queue
(was added for debug information afaict). So it has the same lifetime
of the queue.

The patch should return call nvmf_get_address and if the queue is
LIVE, append the src_addr info under the queue_lock.