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

Taehee Yoo
2023-01-04T08:45:35Z

Hi Sagi,
Thank you so much for your review!

On 1/3/23 19:54, Sagi Grimberg wrote:
>
>> 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.

I tested with a reproducer and a debug patch like below.

#SHELL 1
while :
do
echo 1 > /sys/class/nvme/nvme0/reset_controller
done

#SHELL 2
mount /dev/nvme0n1p66 ~/mnt
while :
do
echo "aaaaaaaaaaaaa" > ~/mnt/test.txt
done

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index cc05c094de22..0cdf83b636a2 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1428,8 +1428,13 @@ static void
nvmet_tcp_free_cmd_data_in_buffers(struct nvmet_tcp_queue *queue)
int i;

for (i = 0; i < queue->nr_cmds; i++, cmd++) {
- if (nvmet_tcp_need_data_in(cmd))
+ if (nvmet_tcp_need_data_in(cmd)) {
nvmet_tcp_free_cmd_buffers(cmd);
+ } else {
+ if (cmd->req.sg) {
+ printk("[TEST]%s %u cmd = %x exec =
%pS\n", __func__, __LINE__, cmd->cmd_pdu->cmd.common.opcode,
cmd->req.execute);
+ }
+ }
}

Results:
[ 393.244102] [TEST]nvmet_tcp_free_cmd_data_in_buffers 1435 cmd = 1
exec = nvmet_bdev_execute_rw+0x0/0xc20
[ 393.607385] [TEST]nvmet_tcp_free_cmd_data_in_buffers 1435 cmd = 1
exec = nvmet_bdev_execute_rw+0x0/0xc20
[ 393.717213] [TEST]nvmet_tcp_free_cmd_data_in_buffers 1435 cmd = 1
exec = nvmet_bdev_execute_rw+0x0/0xc20
[ 393.825434] [TEST]nvmet_tcp_free_cmd_data_in_buffers 1435 cmd = 1
exec = nvmet_bdev_execute_rw+0x0/0xc20
[ 393.952042] [TEST]nvmet_tcp_free_cmd_data_in_buffers 1435 cmd = 1
exec = nvmet_bdev_execute_rw+0x0/0xc20

I think these are not only the leaking case.
Possibly there are more cases.
But I can't test all the cases.

I'm not sure that this info is what really you wanted.
If this is not satisfactory info, please let me know.

Thanks,
Taehee Yoo