Re: [PATCH] vhost/scsi: Fix improper cleanup in vhost_scsi_set_endpoint()

[Kuan-Wei Chiu <visitorckw@gmail.com>]

Hi Haoran,

On Sat, Jan 11, 2025 at 11:34:18AM +0800, Haoran Zhang wrote:
> Since commit 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of session"), a bug can be triggered when the host sends a duplicate VHOST_SCSI_SET_ENDPOINT ioctl command.
> 
> In vhost_scsi_set_endpoint(), if the new `vhost_wwpn` matches the old tpg's tport_name but the tpg is still held by current vhost_scsi(i.e. it is busy), the active `tpg` will be unreferenced. Subsequently, if the owner releases vhost_scsi, the assertion `BUG_ON(sd->s_dependent_count < 1)` will be triggerred, terminating the  target_undepend_item() procedure and leaving `configfs_dirent_lock` locked. If user enters configfs afterward, the CPU will become locked up.
> This issue occurs because vhost_scsi_set_endpoint() allocates a new `vs_tpg` to hold the tpg array and copies all the old tpg entries into it before proceeding. When the new target is busy, the controw flow falls back to the `undepend` label, cause ing all the target `tpg` entries to be unreferenced, including the old one, which is still in use.
> 
> The backtrace is:
> 
> [   60.085044] kernel BUG at fs/configfs/dir.c:1179!
> [   60.087729] RIP: 0010:configfs_undepend_item+0x76/0x80
> [   60.094735] Call Trace:
> [   60.094926]  <TASK>
> [   60.098232]  target_undepend_item+0x1a/0x30
> [   60.098745]  vhost_scsi_clear_endpoint+0x363/0x3e0
> [   60.099342]  vhost_scsi_release+0xea/0x1a0
> [   60.099860]  ? __pfx_vhost_scsi_release+0x10/0x10
> [   60.100459]  ? __pfx_locks_remove_file+0x10/0x10
> [   60.101025]  ? __pfx_task_work_add+0x10/0x10
> [   60.101565]  ? evm_file_release+0xc8/0xe0
> [   60.102074]  ? __pfx_vhost_scsi_release+0x10/0x10
> [   60.102661]  __fput+0x222/0x5a0
> [   60.102925]  ____fput+0x1e/0x30
> [   60.103187]  task_work_run+0x133/0x1c0
> [   60.103479]  ? __pfx_task_work_run+0x10/0x10
> [   60.103813]  ? pick_next_task_fair+0xe1/0x6f0
> [   60.104179]  syscall_exit_to_user_mode+0x235/0x240
> [   60.104542]  do_syscall_64+0x8a/0x170
> [   60.113301]  </TASK>
> [   60.113931] ---[ end trace 0000000000000000 ]---
> [   60.121517] note: poc[2363] exited with preempt_count 1
> 
> To fix this issue, the controw flow should be redirected to the `free_vs_tpg` label to ensure proper cleanup.
> 
> Fixes: 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of session")
> Signed-off-by: Haoran Zhang <wh1sper@zju.edu.cn>

checkpatch.pl generated the following errors and warnings:

WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#59:
Since commit 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of session"), a bug can be triggered when the host sends a duplicate VHOST_SCSI_SET_ENDPOINT ioctl command.

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from control queue handler")'
#59:
Since commit 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of session"), a bug can be triggered when the host sends a duplicate VHOST_SCSI_SET_ENDPOINT ioctl command.

WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 3f8ca2e115e5 ("vhost/scsi: Extract common handling code from control queue handler")'
#91:
Fixes: 3f8ca2e115e55 ("vhost scsi: alloc cmds per vq instead of session")

total: 1 errors, 2 warnings, 15 lines checked


Regards,
Kuan-Wei