Re: [PATCH 4/4] nvmet-tcp: fix memory leak in nvmet_tcp_free_cmd_data_in_buffers()

Sagi Grimberg
2023-01-03T11:48:28Z

> While tcp socket is being released, nvmet_tcp_release_queue_work() is
> called.
> It calls nvmet_tcp_free_cmd_data_in_buffers() to free CMD resources.
> But it may skip freeing resources due to unnecessary condition checks.
> So, the memory leak will occur.
>
> In order to fix this problem, it removes unnecessary condition checks
> in nvmet_tcp_free_cmd_data_in_buffers().
>
> This memory leak issue will occur in the target machine when a host
> sends reset command to a target.
>
> Reproducer:
> while :
> do
> echo 1 > /sys/class/nvme/nvme<NS>/reset_controller
> done
>
> unreferenced object 0xffff88814a5c6da0 (size 32):
> comm "kworker/2:1H", pid 176, jiffies 4305953739 (age 72707.743s)
> hex dump (first 32 bytes):
> 82 84 c8 04 00 ea ff ff 00 00 00 00 00 04 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffaa795cf7>] __kmalloc+0x47/0xc0
> [<ffffffffab030a72>] sgl_alloc_order+0x82/0x3a0
> [<ffffffffc086593c>] nvmet_tcp_map_data+0x1bc/0x570 [nvmet_tcp]
> [<ffffffffc086a7d4>] nvmet_tcp_try_recv_pdu+0x7f4/0x1e20 [nvmet_tcp]
> [<ffffffffc086c5a0>] nvmet_tcp_io_work+0x120/0x3272 [nvmet_tcp]
> [<ffffffffaa1b059d>] process_one_work+0x81d/0x1450
> [<ffffffffaa1b177c>] worker_thread+0x5ac/0xed0
> [<ffffffffaa1c8ccf>] kthread+0x29f/0x340
> [<ffffffffaa0034cf>] ret_from_fork+0x1f/0x30
> unreferenced object 0xffff888153f3e1c0 (size 16):
> comm "kworker/2:1H", pid 176, jiffies 4305953739 (age 72707.743s)
> hex dump (first 16 bytes):
> 80 84 c8 04 00 ea ff ff 00 04 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffffaa795cf7>] __kmalloc+0x47/0xc0
> [<ffffffffc0865a80>] nvmet_tcp_map_data+0x300/0x570 [nvmet_tcp]
> [<ffffffffc086a7d4>] nvmet_tcp_try_recv_pdu+0x7f4/0x1e20 [nvmet_tcp]
> [<ffffffffc086c5a0>] nvmet_tcp_io_work+0x120/0x3272 [nvmet_tcp]
> [<ffffffffaa1b059d>] process_one_work+0x81d/0x1450
> [<ffffffffaa1b177c>] worker_thread+0x5ac/0xed0
> [<ffffffffaa1c8ccf>] kthread+0x29f/0x340
> [<ffffffffaa0034cf>] ret_from_fork+0x1f/0x30
>
> Fixes: db94f240280c ("nvmet-tcp: fix NULL pointer dereference during release")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> drivers/nvme/target/tcp.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index cc05c094de22..dac08603fec9 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1427,13 +1427,10 @@ static void nvmet_tcp_free_cmd_data_in_buffers(struct nvmet_tcp_queue *queue)
> struct nvmet_tcp_cmd *cmd = queue->cmds;
> int i;
>
> - for (i = 0; i < queue->nr_cmds; i++, cmd++) {
> - if (nvmet_tcp_need_data_in(cmd))
> - nvmet_tcp_free_cmd_buffers(cmd);
> - }
> + for (i = 0; i < queue->nr_cmds; i++, cmd++)
> + nvmet_tcp_free_cmd_buffers(cmd);
>
> - if (!queue->nr_cmds && nvmet_tcp_need_data_in(&queue->connect))
> - nvmet_tcp_free_cmd_buffers(&queue->connect);
> + nvmet_tcp_free_cmd_buffers(&queue->connect);
> }
>
> static void nvmet_tcp_release_queue_work(struct work_struct *w)

Would it be possible to understand what are the commands that are
leaking here? commands that do not wait for data from the host, should
have a normal path of release, if that is not happening, we may be in
risk for use-after-free or still have a leak somewhere.