On 9/18/23 5:58 PM, Junxiao Bi wrote:
> The following call trace shown a deadlock issue due to recursive locking
> of mutex "device_mutex" , first lock acquire in target_for_each_device()
> and second in target_free_device().
>
> PID: 148266 TASK: ffff8be21ffb5d00 CPU: 10 COMMAND: "iscsi_ttx"
> #0 [ffffa2bfc9ec3b18] __schedule at ffffffffa8060e7f
> #1 [ffffa2bfc9ec3ba0] schedule at ffffffffa8061224
> #2 [ffffa2bfc9ec3bb8] schedule_preempt_disabled at ffffffffa80615ee
> #3 [ffffa2bfc9ec3bc8] __mutex_lock at ffffffffa8062fd7
> #4 [ffffa2bfc9ec3c40] __mutex_lock_slowpath at ffffffffa80631d3
> #5 [ffffa2bfc9ec3c50] mutex_lock at ffffffffa806320c
> #6 [ffffa2bfc9ec3c68] target_free_device at ffffffffc0935998 [target_core_mod]
> #7 [ffffa2bfc9ec3c90] target_core_dev_release at ffffffffc092f975 [target_core_mod]
> #8 [ffffa2bfc9ec3ca0] config_item_put at ffffffffa79d250f
> #9 [ffffa2bfc9ec3cd0] config_item_put at ffffffffa79d2583
> #10 [ffffa2bfc9ec3ce0] target_devices_idr_iter at ffffffffc0933f3a [target_core_mod]
> #11 [ffffa2bfc9ec3d00] idr_for_each at ffffffffa803f6fc
> #12 [ffffa2bfc9ec3d60] target_for_each_device at ffffffffc0935670 [target_core_mod]
> #13 [ffffa2bfc9ec3d98] transport_deregister_session at ffffffffc0946408 [target_core_mod]
> #14 [ffffa2bfc9ec3dc8] iscsit_close_session at ffffffffc09a44a6 [iscsi_target_mod]
> #15 [ffffa2bfc9ec3df0] iscsit_close_connection at ffffffffc09a4a88 [iscsi_target_mod]
> #16 [ffffa2bfc9ec3df8] finish_task_switch at ffffffffa76e5d07
> #17 [ffffa2bfc9ec3e78] iscsit_take_action_for_connection_exit at ffffffffc0991c23 [iscsi_target_mod]
> #18 [ffffa2bfc9ec3ea0] iscsi_target_tx_thread at ffffffffc09a403b [iscsi_target_mod]
> #19 [ffffa2bfc9ec3f08] kthread at ffffffffa76d8080
> #20 [ffffa2bfc9ec3f50] ret_from_fork at ffffffffa8200364
>
> Fixes: 36d4cb460bcb ("scsi: target: Avoid that EXTENDED COPY commands trigger lock inversion")
> Signed-off-by: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
> ---
> drivers/target/target_core_device.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index b7ac60f4a219..b6523d4b9259 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -843,7 +843,6 @@ sector_t target_to_linux_sector(struct se_device *dev, sector_t lb)
> EXPORT_SYMBOL(target_to_linux_sector);
>
> struct devices_idr_iter {
> - struct config_item *prev_item;
> int (*fn)(struct se_device *dev, void *data);
> void *data;
> };
> @@ -853,11 +852,9 @@ static int target_devices_idr_iter(int id, void *p, void *data)
> {
> struct devices_idr_iter *iter = data;
> struct se_device *dev = p;
> + struct config_item *item;
> int ret;
>
> - config_item_put(iter->prev_item);
I can't see why we did that (I had reviewed/signed-off on the patch that added
the bug) and I don't see any reason why we would have needed it in the past.
Looks like idr_for_each would have handled it ok back then as well. So patch looks
ok to me.
Reviewed-by: Mike Christie <michael.christie@xxxxxxxxxx>
