diff mbox series

scsi: target: fix deadlock by recursive locking

Message ID 20230918225848.66463-1-junxiao.bi@oracle.com (mailing list archive)
State Accepted
Commit a154f5f643c6ecddd44847217a7a3845b4350003
Headers show
Series scsi: target: fix deadlock by recursive locking | expand

Commit Message

Junxiao Bi Sept. 18, 2023, 10:58 p.m. UTC
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@oracle.com>
---
 drivers/target/target_core_device.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Mike Christie Sept. 23, 2023, 8:49 p.m. UTC | #1
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@oracle.com>
> ---
>  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@oracle.com>
Martin K. Petersen Sept. 27, 2023, 7:34 p.m. UTC | #2
On Mon, 18 Sep 2023 15:58:48 -0700, 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
> 
> [...]

Applied to 6.6/scsi-fixes, thanks!

[1/1] scsi: target: fix deadlock by recursive locking
      https://git.kernel.org/mkp/scsi/c/a154f5f643c6
diff mbox series

Patch

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);
-	iter->prev_item = NULL;
-
 	/*
 	 * We add the device early to the idr, so it can be used
 	 * by backend modules during configuration. We do not want
@@ -867,12 +864,13 @@  static int target_devices_idr_iter(int id, void *p, void *data)
 	if (!target_dev_configured(dev))
 		return 0;
 
-	iter->prev_item = config_item_get_unless_zero(&dev->dev_group.cg_item);
-	if (!iter->prev_item)
+	item = config_item_get_unless_zero(&dev->dev_group.cg_item);
+	if (!item)
 		return 0;
 	mutex_unlock(&device_mutex);
 
 	ret = iter->fn(dev, iter->data);
+	config_item_put(item);
 
 	mutex_lock(&device_mutex);
 	return ret;
@@ -895,7 +893,6 @@  int target_for_each_device(int (*fn)(struct se_device *dev, void *data),
 	mutex_lock(&device_mutex);
 	ret = idr_for_each(&devices_idr, target_devices_idr_iter, &iter);
 	mutex_unlock(&device_mutex);
-	config_item_put(iter.prev_item);
 	return ret;
 }