| Message ID | 20250111033454.26596-1-wh1sper@zju.edu.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vhost/scsi: Fix improper cleanup in vhost_scsi_set_endpoint() | expand |
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
Hi Kuan-Wei, On Sat, Jan 11, 2025 at 13:45:50 +0800, Kuan-Wei Chiu wrote: > 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 Thanks for your suggestion, I will send a corrected patch later. Best regards, Haoran Zhang
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 718fa4e0b31e..b994138837f2 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1726,7 +1726,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, mutex_unlock(&tpg->tv_tpg_mutex); mutex_unlock(&vhost_scsi_mutex); ret = -EEXIST; - goto undepend; + goto free_vs_tpg; } /* * In order to ensure individual vhost-scsi configfs @@ -1802,6 +1802,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs, target_undepend_item(&tpg->se_tpg.tpg_group.cg_item); } } +free_vs_tpg: kfree(vs_tpg); out: mutex_unlock(&vs->dev.mutex);
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> --- drivers/vhost/scsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)