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

[Taehee Yoo <ap420073@gmail.com>]

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