Re: [PATCH v5 10/10] scsi: scsi_debug: Add param to control sdev's allow_restart

['Wenchao Hao' via open-iscsi]

On 2023/10/9 7:17, Douglas Gilbert wrote:

On 2023-09-22 05:29, Wenchao Hao wrote:

Add new module param "allow_restart" to control if setup
scsi_device's allow_restart flag. This is used to test scsi
command finished with sense_key 0x6, asc 0x4 and ascq 0x2

Signed-off-by: Wenchao Hao 


Hi,
Looked at this and verified that the allow_restart flag of scsi_debug
devices (disks ?) is usually 0 and when the scsi_debug module is
started with allow_restart=1 then the allow_restart flag does indeed
change to 1. For example:
    # cat /sys/class/scsi_disk/1\:0\:0\:0/allow_restart
    1

That ASC/ASCQ code means: "Logical unit not ready, initializing command
required" according to my library. Played around with sg_start but didn't
see any change in how it reacts. According to scsi_device.h that flag's
description is: "issue START_UNIT in error handler" which implies it
changes how the EH handler reacts.

Perhaps the 3 line patch description could say a little more about how
to use this new parameter...


Sorry I did not write in detail. As you mentioned above, this is to
determine if to trigger error. I would update the commit message to
following lines:

Add new module param "allow_restart" to control if setup scsi_device's
allow_restart flag, this flag determines if trigger EH after command
finished with sense_key 0x6, asc 0x4 and ascq 0x2, EH would be triggered
if allow_restart=1 in this condition.

The new param can be used with error inject added in patch6 to test how
commands finished with sense_key 0x6, asc 0x4 and ascq 0x2 are handled.



Tested-by: Douglas Gilbert 









--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/89365bd3-b4b0-de2d-f863-afbaad118649%40huawei.com.

Re: [PATCH v5 08/10] scsi: scsi_debug: Add new error injection reset lun failed

['Wenchao Hao' via open-iscsi]

On 2023/10/7 5:04, Douglas Gilbert wrote:

On 2023-09-22 05:29, Wenchao Hao wrote:

Add error injection type 3 to make scsi_debug_device_reset() return FAILED.
Fail abort command foramt:


s/foramt/format/



Examples:
 error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
 echo "0 -10 0x12" > ${error}


These examples are misleading. Same with the one in patch 7/10 . The example
should be showing an invocation that exercises _this_ patch. So the first
byte of the echo should be 4 not the 0 shown above.

Doug Gilbert



Would update in next version. Would you continue reviewing patch 9/10 and 10/10?

--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/fa567e0b-00ca-76ad-f9e7-a554714f813c%40huawei.com.

Re: [PATCH v5 01/10] scsi: scsi_debug: create scsi_debug directory in the debugfs filesystem

['Wenchao Hao' via open-iscsi]

On 2023/9/28 9:13, Douglas Gilbert wrote:

On 2023-09-22 05:28, Wenchao Hao wrote:

Create directory scsi_debug in the root of the debugfs filesystem.
Prepare to add interface for manage error injection.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
  drivers/scsi/scsi_debug.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9c0af50501f9..35c336271b13 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -41,6 +41,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
@@ -862,6 +863,8 @@ static const int device_qfull_result =
  static const int condition_met_result = SAM_STAT_CONDITION_MET;
+static struct dentry *sdebug_debugfs_root;
+
  /* Only do the extra work involved in logical block provisioning if one or
   * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
@@ -7011,6 +7014,8 @@ static int __init scsi_debug_init(void)
  goto driver_unreg;
  }
+    sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);


debugfs_create_dir() can fail and return NULL. Looking at other drivers, most
seem to assume it will work. Since the scsi_debug driver is often used to test
abnormal situations, perhaps adding something like:
     if (!sdebug_debugfs_root)
     pr_info("%s: failed to create initial debugfs directory\n", __func__);

might save someone a bit of time if a NULL dereference on sdebug_debugfs_root
follows later. That is what the mpt3sas driver does.



Yes, I would fix it by checking return value of debugfs related call
after your review suggestions for other patches.


Doug Gilbert


+
  for (k = 0; k < hosts_to_add; k++) {
  if (want_store && k == 0) {
  ret = sdebug_add_host_helper(idx);
@@ -7057,6 +7062,7 @@ static void __exit scsi_debug_exit(void)
  sdebug_erase_all_stores(false);
  xa_destroy(per_store_ap);
+    debugfs_remove(sdebug_debugfs_root);
  }
  device_initcall(scsi_debug_init);





--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/382fe161-95fb-3249-32cf-07058f81a4bc%40huawei.com.

Re: [PATCH 2/2] scsi: Add comment of target_destroy in scsi_host_template

['Wenchao Hao' via open-iscsi]

On 2023/9/22 22:53, Bart Van Assche wrote:

On 9/22/23 02:38, Wenchao Hao wrote:

Add comment to tell callback function target_destroy of
scsi_host_template is called in atomic context.

Signed-off-by: Wenchao Hao 
---
  include/scsi/scsi_host.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..a72248fa5adf 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -245,6 +245,9 @@ struct scsi_host_template {
   * midlayer calls this point so that the driver may deallocate
   * and terminate any references to the target.
   *
+ * Note: this callback in called with spin_lock held, so donot
+ * call functions might cause schedule
+ *


This comment should mention which spinlock is held.



Would update, thanks for your review suggestion.


Thanks,

Bart.




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f2fb9e25-c022-58d2-ac56-db35c2edfedf%40huawei.com.

[PATCH 0/2] cleanup patch

['Wenchao Hao' via open-iscsi]

This is a cleanup patchset, no logic changed.

The first patch just cleanup scsi_dev_queue_ready();
The second patch add comment for target_destroy callback of
scsi_host_template to tell it is called in atomic context.

Wenchao Hao (2):
  scsi: core: cleanup scsi_dev_queue_ready()
  scsi: Add comment of target_destroy in scsi_host_template

 drivers/scsi/scsi_lib.c  | 35 ++-
 include/scsi/scsi_host.h |  3 +++
 2 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922093842.2646157-1-haowenchao2%40huawei.com.

[PATCH 1/2] scsi: core: cleanup scsi_dev_queue_ready()

['Wenchao Hao' via open-iscsi]

This is just a cleanup for scsi_dev_queue_ready() to avoid
redundant goto and if statement, it did not change the origin
logic.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_lib.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ca5eb058d5c7..f3e388127dbd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1254,28 +1254,29 @@ static inline int scsi_dev_queue_ready(struct 
request_queue *q,
int token;
 
token = sbitmap_get(>budget_map);
-   if (atomic_read(>device_blocked)) {
-   if (token < 0)
-   goto out;
+   if (token < 0)
+   return -1;
 
-   if (scsi_device_busy(sdev) > 1)
-   goto out_dec;
+   /*
+* device_blocked is not set at mostly time, so check it first
+* and return token when it is not set.
+*/
+   if (!atomic_read(>device_blocked))
+   return token;
 
-   /*
-* unblock after device_blocked iterates to zero
-*/
-   if (atomic_dec_return(>device_blocked) > 0)
-   goto out_dec;
-   SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
-  "unblocking device at zero depth\n"));
+   /*
+* unblock after device_blocked iterates to zero
+*/
+   if (scsi_device_busy(sdev) > 1 ||
+   atomic_dec_return(>device_blocked) > 0) {
+   sbitmap_put(>budget_map, token);
+   return -1;
}
 
+   SCSI_LOG_MLQUEUE(3, sdev_printk(KERN_INFO, sdev,
+"unblocking device at zero depth\n"));
+
return token;
-out_dec:
-   if (token >= 0)
-   sbitmap_put(>budget_map, token);
-out:
-   return -1;
 }
 
 /*
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922093842.2646157-2-haowenchao2%40huawei.com.

[PATCH 2/2] scsi: Add comment of target_destroy in scsi_host_template

['Wenchao Hao' via open-iscsi]

Add comment to tell callback function target_destroy of
scsi_host_template is called in atomic context.

Signed-off-by: Wenchao Hao 
---
 include/scsi/scsi_host.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..a72248fa5adf 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -245,6 +245,9 @@ struct scsi_host_template {
 * midlayer calls this point so that the driver may deallocate
 * and terminate any references to the target.
 *
+* Note: this callback in called with spin_lock held, so donot
+* call functions might cause schedule
+*
 * Status: OPTIONAL
 */
void (* target_destroy)(struct scsi_target *);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922093842.2646157-3-haowenchao2%40huawei.com.

[PATCH 0/2] Fix two issue between removing device and error handle

['Wenchao Hao' via open-iscsi]

I am testing SCSI error handle with my previous scsi_debug error
injection patches, and found two issue when removing device and
error handler happened together.

The first patch fix IO hang because scsi_eh_flush_done_q() would
retry command if the device is in SDEV_CANCEL state;

The second patch fix the issue which device's eh_device_reset_handler
not called in recovery when device is in removing progress.

Wenchao Hao (2):
  scsi: core: scsi_device_online() return false if state is SDEV_CANCEL
  scsi: scsi_error: Fix device reset is not triggered

 drivers/infiniband/ulp/srp/ib_srp.c |  2 +-
 drivers/message/fusion/mptctl.c |  6 ++--
 drivers/message/fusion/mptsas.c |  8 ++---
 drivers/message/fusion/mptspi.c |  6 ++--
 drivers/s390/scsi/zfcp_fsf.c|  4 +--
 drivers/s390/scsi/zfcp_scsi.c   |  2 +-
 drivers/scsi/arm/acornscsi.c|  2 +-
 drivers/scsi/arm/fas216.c   |  4 +--
 drivers/scsi/bfa/bfad_im.c  |  4 +--
 drivers/scsi/ibmvscsi/ibmvfc.c  |  2 +-
 drivers/scsi/lpfc/lpfc_scsi.c   |  4 +--
 drivers/scsi/megaraid/megaraid_sas_base.c   |  2 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_ctl.c  |  4 +--
 drivers/scsi/mpt3sas/mpt3sas_scsih.c| 16 -
 drivers/scsi/myrb.c |  2 +-
 drivers/scsi/myrs.c |  2 +-
 drivers/scsi/scsi.c | 37 -
 drivers/scsi/scsi_debug.c   |  2 +-
 drivers/scsi/scsi_error.c   | 12 +++
 drivers/scsi/scsi_lib.c |  6 ++--
 drivers/scsi/scsi_scan.c|  2 +-
 drivers/scsi/scsi_transport_srp.c   |  2 +-
 drivers/scsi/ses.c  |  2 +-
 drivers/scsi/storvsc_drv.c  |  2 +-
 drivers/scsi/virtio_scsi.c  |  2 +-
 drivers/ufs/core/ufshcd.c   |  4 +--
 include/scsi/scsi_device.h  | 15 ++---
 28 files changed, 85 insertions(+), 73 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922093636.2645961-1-haowenchao2%40huawei.com.

[PATCH v5 10/10] scsi: scsi_debug: Add param to control sdev's allow_restart

['Wenchao Hao' via open-iscsi]

Add new module param "allow_restart" to control if setup
scsi_device's allow_restart flag. This is used to test scsi
command finished with sense_key 0x6, asc 0x4 and ascq 0x2

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ab4a6f7de1ef..52a9ddea57d3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -843,6 +843,7 @@ static bool have_dif_prot;
 static bool write_since_sync;
 static bool sdebug_statistics = DEF_STATISTICS;
 static bool sdebug_wp;
+static bool sdebug_allow_restart;
 /* Following enum: 0: no zbc, def; 1: host aware; 2: host managed */
 static enum blk_zoned_model sdeb_zbc_model = BLK_ZONED_NONE;
 static char *sdeb_zbc_model_s;
@@ -5469,6 +5470,9 @@ static int scsi_debug_slave_configure(struct scsi_device 
*sdp)
sdp->no_uld_attach = 1;
config_cdb_len(sdp);
 
+   if (sdebug_allow_restart)
+   sdp->allow_restart = 1;
+
devip->debugfs_entry = debugfs_create_dir(dev_name(>sdev_dev),
sdebug_debugfs_root);
debugfs_create_file("error", 0600, devip->debugfs_entry, sdp,
@@ -6186,6 +6190,7 @@ module_param_named(zone_cap_mb, sdeb_zbc_zone_cap_mb, 
int, S_IRUGO);
 module_param_named(zone_max_open, sdeb_zbc_max_open, int, S_IRUGO);
 module_param_named(zone_nr_conv, sdeb_zbc_nr_conv, int, S_IRUGO);
 module_param_named(zone_size_mb, sdeb_zbc_zone_size_mb, int, S_IRUGO);
+module_param_named(allow_restart, sdebug_allow_restart, bool, S_IRUGO | 
S_IWUSR);
 
 MODULE_AUTHOR("Eric Youngdale + Douglas Gilbert");
 MODULE_DESCRIPTION("SCSI debug adapter driver");
@@ -6258,6 +6263,7 @@ MODULE_PARM_DESC(zone_cap_mb, "Zone capacity in MiB 
(def=zone size)");
 MODULE_PARM_DESC(zone_max_open, "Maximum number of open zones; [0] for no 
limit (def=auto)");
 MODULE_PARM_DESC(zone_nr_conv, "Number of conventional zones (def=1)");
 MODULE_PARM_DESC(zone_size_mb, "Zone size in MiB (def=auto)");
+MODULE_PARM_DESC(allow_restart, "Set scsi_device's allow_restart flag(def=0)");
 
 #define SDEBUG_INFO_LEN 256
 static char sdebug_info[SDEBUG_INFO_LEN];
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-11-haowenchao2%40huawei.com.

[PATCH v5 05/10] scsi: scsi_debug: Return failed value if the error is injected

['Wenchao Hao' via open-iscsi]

If a fail queuecommand error is injected, return the failed value defined
in the rule from queuecommand.

Make queuecommand return format:
  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error type, fixed to 0x1  |
  ++--+---+
  |   2|  s32 | Error count   |
  ||  |  0: this rule will be ignored |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+
  |   4|  x32 | The queuecommand() return value we want   |
  ++--+---+

Examples:
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "1 1 0x12 0x1055" > ${error}
will make each INQUIRY command sent to that device return
0x1055 (SCSI_MLQUEUE_HOST_BUSY).

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 785ad5e5b4a4..dea40d983155 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7759,6 +7759,34 @@ static int sdebug_timeout_cmd(struct scsi_cmnd *cmnd)
return 0;
 }
 
+static int sdebug_fail_queue_cmd(struct scsi_cmnd *cmnd)
+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_FAIL_QUEUE_CMD &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = err->cnt ? err->queuecmd_ret : 0;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
   struct scsi_cmnd *scp)
 {
@@ -7778,6 +7806,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host 
*shost,
u8 opcode = cmd[0];
bool has_wlun_rl;
bool inject_now;
+   int ret = 0;
 
scsi_set_resid(scp, 0);
if (sdebug_statistics) {
@@ -7823,6 +7852,13 @@ static int scsi_debug_queuecommand(struct Scsi_Host 
*shost,
return 0;
}
 
+   ret = sdebug_fail_queue_cmd(scp);
+   if (ret) {
+   scmd_printk(KERN_INFO, scp, "fail queue command 0x%x with 
0x%x\n",
+   opcode, ret);
+   return ret;
+   }
+
if (unlikely(inject_now && !atomic_read(_inject_pending)))
atomic_set(_inject_pending, 1);
 
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-6-haowenchao2%40huawei.com.

[PATCH 2/2] scsi: scsi_error: Fix device reset is not triggered

['Wenchao Hao' via open-iscsi]

Fix the issue of skipping scsi_try_bus_device_reset() for devices
which is in progress of removing in following order:

T1: T2:scsi_error_handle
__scsi_remove_device
  scsi_device_set_state(sdev, SDEV_DEL)
// would skip device with SDEV_DEL state
shost_for_each_device()
  scsi_try_bus_device_reset
flush all commands
 ...
 scsi_device is released

Some drivers like smartpqi only implement eh_device_reset_handler,
if device reset is skipped, the commands which had been sent to
firmware or devices hardware are not cleared. The error handle
would flush all these commands in scsi_unjam_host().

When the commands are finished by hardware, use after free issue is
triggered.

Add parameter "check_state" to macro shost_for_each_device() to
determine if check device status when traversal scsi_device
of Scsi_Host, and set this parameter to false when traversal
in scsi_error_handle to address this issue.

Signed-off-by: Wenchao Hao 
---
 drivers/infiniband/ulp/srp/ib_srp.c |  2 +-
 drivers/message/fusion/mptctl.c |  6 ++--
 drivers/message/fusion/mptsas.c |  8 ++---
 drivers/message/fusion/mptspi.c |  6 ++--
 drivers/s390/scsi/zfcp_fsf.c|  4 +--
 drivers/s390/scsi/zfcp_scsi.c   |  2 +-
 drivers/scsi/arm/acornscsi.c|  2 +-
 drivers/scsi/arm/fas216.c   |  4 +--
 drivers/scsi/bfa/bfad_im.c  |  4 +--
 drivers/scsi/ibmvscsi/ibmvfc.c  |  2 +-
 drivers/scsi/lpfc/lpfc_scsi.c   |  4 +--
 drivers/scsi/megaraid/megaraid_sas_base.c   |  2 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_ctl.c  |  4 +--
 drivers/scsi/mpt3sas/mpt3sas_scsih.c| 16 -
 drivers/scsi/myrb.c |  2 +-
 drivers/scsi/myrs.c |  2 +-
 drivers/scsi/scsi.c | 37 -
 drivers/scsi/scsi_debug.c   |  2 +-
 drivers/scsi/scsi_error.c   | 12 +++
 drivers/scsi/scsi_lib.c |  6 ++--
 drivers/scsi/scsi_scan.c|  2 +-
 drivers/scsi/scsi_transport_srp.c   |  2 +-
 drivers/scsi/ses.c  |  2 +-
 drivers/scsi/storvsc_drv.c  |  2 +-
 drivers/scsi/virtio_scsi.c  |  2 +-
 drivers/ufs/core/ufshcd.c   |  4 +--
 include/scsi/scsi_device.h  | 12 ---
 28 files changed, 83 insertions(+), 72 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 1574218764e0..219516e33ca7 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3098,7 +3098,7 @@ static int srp_sdev_count(struct Scsi_Host *host)
struct scsi_device *sdev;
int c = 0;
 
-   shost_for_each_device(sdev, host)
+   shost_for_each_device(sdev, host, 1)
c++;
 
return c;
diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index dd028df4b283..58c4d27fb386 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -1310,7 +1310,7 @@ mptctl_getiocinfo (MPT_ADAPTER *ioc, unsigned long arg, 
unsigned int data_size)
  */
karg->numDevices = 0;
if (ioc->sh) {
-   shost_for_each_device(sdev, ioc->sh) {
+   shost_for_each_device(sdev, ioc->sh, 1) {
vdevice = sdev->hostdata;
if (vdevice == NULL || vdevice->vtarget == NULL)
continue;
@@ -1416,7 +1416,7 @@ mptctl_gettargetinfo (MPT_ADAPTER *ioc, unsigned long arg)
/* Get number of devices
  */
if (ioc->sh){
-   shost_for_each_device(sdev, ioc->sh) {
+   shost_for_each_device(sdev, ioc->sh, 1) {
if (!maxWordsLeft)
continue;
vdevice = sdev->hostdata;
@@ -1889,7 +1889,7 @@ mptctl_do_mpt_command (MPT_ADAPTER *ioc, struct 
mpt_ioctl_command karg, void __u
cpu_to_le32(ioc->sense_buf_low_dma
   + (req_idx * MPT_SENSE_BUFFER_ALLOC));
 
-   shost_for_each_device(sdev, ioc->sh) {
+   shost_for_each_device(sdev, ioc->sh, 1) {
struct scsi_target *starget = scsi_target(sdev);
VirtTarget *vtarget = starget->hostdata;
 
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 86f16f3ea478..0835fd31af7d 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -623,7 +623,7 @@ 

[PATCH 1/2] scsi: core: scsi_device_online() return false if state is SDEV_CANCEL

['Wenchao Hao' via open-iscsi]

SDEV_CANCEL is set when removing device and scsi_device_online() should
return false if sdev_state is SDEV_CANCEL.

IO hang would be caused if return true when state is SDEV_CANCEL with
following order:

T1: T2:scsi_error_handler
__scsi_remove_device()
  scsi_device_set_state(sdev, SDEV_CANCEL)
scsi_eh_flush_done_q()
if (scsi_device_online(sdev))
  scsi_queue_insert(scmd,...)

The command added by scsi_queue_insert() would never be handled any
more.

Signed-off-by: Wenchao Hao 
---
 include/scsi/scsi_device.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 75b2235b99e2..c498a12f7715 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -517,7 +517,8 @@ static inline int scsi_device_online(struct scsi_device 
*sdev)
 {
return (sdev->sdev_state != SDEV_OFFLINE &&
sdev->sdev_state != SDEV_TRANSPORT_OFFLINE &&
-   sdev->sdev_state != SDEV_DEL);
+   sdev->sdev_state != SDEV_DEL &&
+   sdev->sdev_state != SDEV_CANCEL);
 }
 static inline int scsi_device_blocked(struct scsi_device *sdev)
 {
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922093636.2645961-2-haowenchao2%40huawei.com.

[PATCH v5 06/10] scsi: scsi_debug: set command's result and sense data if the error is injected

['Wenchao Hao' via open-iscsi]

If a fail commnd error is injected, set the command's status and sense
data then finish this scsi command.

Set SCSI command's status and sense data format:
  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error type, fixed to 0x2  |
  ++--+---+
  |   2|  s32 | Error Count   |
  ||  |  0: the rule will be ignored  |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+
  |   4|  x8  | Host byte in scsi_cmd::status |
  ||  | [scsi_cmd::status has 32 bits holding these 3 bytes]  |
  ++--+---+
  |   5|  x8  | Driver byte in scsi_cmd::status   |
  ++--+---+
  |   6|  x8  | SCSI Status byte in scsi_cmd::status  |
  ++--+---+
  |   7|  x8  | SCSI Sense Key in scsi_cmnd   |
  ++--+---+
  |   8|  x8  | SCSI ASC in scsi_cmnd |
  ++--+---+
  |   9|  x8  | SCSI ASCQ in scsi_cmnd|
  ++--+---+
Examples:
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "2 -10 0x88 0 0 0x2 0x3 0x11 0x0" >${error}
will make device's read command return with media error with additional
sense of "Unrecovered read error" (UNC):

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 53 +++
 1 file changed, 53 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index dea40d983155..fe1f7421f617 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7787,6 +7787,48 @@ static int sdebug_fail_queue_cmd(struct scsi_cmnd *cmnd)
return 0;
 }
 
+static int sdebug_fail_cmd(struct scsi_cmnd *cmnd, int *retval,
+  struct sdebug_err_inject *info)
+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+   int result;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_FAIL_CMD &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   if (!err->cnt) {
+   rcu_read_unlock();
+   return 0;
+   }
+
+   ret = !!err->cnt;
+   rcu_read_unlock();
+   goto out_handle;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+
+out_handle:
+   if (err->cnt < 0)
+   err->cnt++;
+   mk_sense_buffer(cmnd, err->sense_key, err->asc, err->asq);
+   result = err->status_byte | err->host_byte << 16 | err->driver_byte << 
24;
+   *info = *err;
+   *retval = schedule_resp(cmnd, devip, result, NULL, 0, 0);
+
+   return ret;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
   struct scsi_cmnd *scp)
 {
@@ -7807,6 +7849,7 @@ static int scsi_debug_queuecommand(struct Scsi_Host 
*shost,
bool has_wlun_rl;
bool inject_now;
int ret = 0;
+   struct sdebug_err_inject err;
 
scsi_set_resid(scp, 0);
if (sdebug_statistics) {
@@ -7859,6 +7902,16 @@ static int scsi_debug_queuecommand(struct Scsi_Host 
*shost,
return ret;
}
 
+   if (sdebug_fail_cmd(scp, , )) {
+   scmd_printk(KERN_INFO, scp,
+   "fail command 0x%x with hostbyte=0x%x, "
+   "driverbyte=0x%x, statusbyte=0x%x, "
+   "sense_key=0x%x, asc=0x%x, asq=0x%x\n",
+   opcode, err.host_byte, err.driver_byte,
+  

[PATCH v5 09/10] scsi: scsi_debug: Add debugfs interface to fail target reset

['Wenchao Hao' via open-iscsi]

The interface is found at
/sys/kernel/debug/scsi_debug/target/fail_reset where 
identifies the target to inject errors on. It's a simple bool type
interface which would make this target's reset fail if set to 'Y'.

Signed-off-by: Wenchao Hao 
Reported-by: kernel test robot 
---
 drivers/scsi/scsi_debug.c | 106 +-
 1 file changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index db8ab6cad078..ab4a6f7de1ef 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -42,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -357,6 +358,11 @@ struct sdebug_dev_info {
struct list_head inject_err_list;
 };
 
+struct sdebug_target_info {
+   bool reset_fail;
+   struct dentry *debugfs_entry;
+};
+
 struct sdebug_host_info {
struct list_head host_list;
int si_idx; /* sdeb_store_info (per host) xarray index */
@@ -1082,6 +1088,83 @@ static const struct file_operations sdebug_error_fops = {
.release = single_release,
 };
 
+static int sdebug_target_reset_fail_show(struct seq_file *m, void *p)
+{
+   struct scsi_target *starget = (struct scsi_target *)m->private;
+   struct sdebug_target_info *targetip =
+   (struct sdebug_target_info *)starget->hostdata;
+
+   if (targetip)
+   seq_printf(m, "%c\n", targetip->reset_fail ? 'Y' : 'N');
+
+   return 0;
+}
+
+static int sdebug_target_reset_fail_open(struct inode *inode, struct file 
*file)
+{
+   return single_open(file, sdebug_target_reset_fail_show, 
inode->i_private);
+}
+
+static ssize_t sdebug_target_reset_fail_write(struct file *file,
+   const char __user *ubuf, size_t count, loff_t *ppos)
+{
+   int ret;
+   struct scsi_target *starget =
+   (struct scsi_target *)file->f_inode->i_private;
+   struct sdebug_target_info *targetip =
+   (struct sdebug_target_info *)starget->hostdata;
+
+   if (targetip) {
+   ret = kstrtobool_from_user(ubuf, count, >reset_fail);
+   return ret < 0 ? ret : count;
+   }
+   return -ENODEV;
+}
+
+static const struct file_operations sdebug_target_reset_fail_fops = {
+   .open   = sdebug_target_reset_fail_open,
+   .read   = seq_read,
+   .write  = sdebug_target_reset_fail_write,
+   .release = single_release,
+};
+
+static int sdebug_target_alloc(struct scsi_target *starget)
+{
+   struct sdebug_target_info *targetip;
+
+   targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
+   if (!targetip)
+   return -ENOMEM;
+
+   targetip->debugfs_entry = debugfs_create_dir(dev_name(>dev),
+   sdebug_debugfs_root);
+   debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, 
starget,
+   _target_reset_fail_fops);
+
+   starget->hostdata = targetip;
+
+   return 0;
+}
+
+static void sdebug_tartget_cleanup_async(void *data, async_cookie_t cookie)
+{
+   struct sdebug_target_info *targetip = data;
+
+   debugfs_remove(targetip->debugfs_entry);
+   kfree(targetip);
+}
+
+static void sdebug_target_destroy(struct scsi_target *starget)
+{
+   struct sdebug_target_info *targetip;
+
+   targetip = (struct sdebug_target_info *)starget->hostdata;
+   if (targetip) {
+   starget->hostdata = NULL;
+   async_schedule(sdebug_tartget_cleanup_async, targetip);
+   }
+}
+
 /* Only do the extra work involved in logical block provisioning if one or
  * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
  * real reads and writes (i.e. not skipping them for speed).
@@ -5634,11 +5717,25 @@ static int scsi_debug_device_reset(struct scsi_cmnd 
*SCpnt)
return SUCCESS;
 }
 
+static int sdebug_fail_target_reset(struct scsi_cmnd *cmnd)
+{
+   struct scsi_target *starget = scsi_target(cmnd->device);
+   struct sdebug_target_info *targetip =
+   (struct sdebug_target_info *)starget->hostdata;
+
+   if (targetip)
+   return targetip->reset_fail;
+
+   return 0;
+}
+
 static int scsi_debug_target_reset(struct scsi_cmnd *SCpnt)
 {
struct scsi_device *sdp = SCpnt->device;
struct sdebug_host_info *sdbg_host = shost_to_sdebug_host(sdp->host);
struct sdebug_dev_info *devip;
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
int k = 0;
 
++num_target_resets;
@@ -5656,6 +5753,12 @@ static int scsi_debug_target_reset(struct scsi_cmnd 
*SCpnt)
sdev_printk(KERN_INFO, sdp,
"%s: %d device(s) found in target\n", __func__, k);
 
+   if (sdebug_fail_target_reset(SCpnt)) {
+   scmd_printk(KERN_INFO, SCpnt, "fail target reset 0x%x\n",
+   opcode);
+   return FAILED;
+   }
+
return SUCCESS;
 }
 

[PATCH v5 07/10] scsi: scsi_debug: Add new error injection abort failed

['Wenchao Hao' via open-iscsi]

Add error injection type 3 to make scsi_debug_abort() return FAILED.
Fail abort command foramt:

  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error type, fixed to 0x3  |
  ++--+---+
  |   2|  s32 | Error count   |
  ||  |  0: this rule will be ignored |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+

Examples:
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "0 -10 0x12" > ${error}
will make the device return FAILED when abort inquiry command 10 times.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 40 +++
 1 file changed, 40 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fe1f7421f617..8a16cb9642a6 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -293,6 +293,8 @@ enum sdebug_err_type {
ERR_FAIL_CMD= 2,/* make specific scsi command's */
/* queuecmd return succeed but */
/* with errors set in scsi_cmnd */
+   ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
+   /* scsi_debug_abort() */
 };
 
 struct sdebug_err_inject {
@@ -970,6 +972,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
list_for_each_entry_rcu(err, >inject_err_list, list) {
switch (err->type) {
case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1031,6 +1034,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
 
switch (inject_type) {
case ERR_TMOUT_CMD:
+   case ERR_ABORT_CMD_FAILED:
if (sscanf(buf, "%d %d %hhx", >type, >cnt,
   >cmd) != 3)
goto out_error;
@@ -5504,9 +5508,39 @@ static void stop_all_queued(void)
mutex_unlock(_host_list_mutex);
 }
 
+static int sdebug_fail_abort(struct scsi_cmnd *cmnd)
+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_ABORT_CMD_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
 static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
 {
bool ok = scsi_debug_abort_cmnd(SCpnt);
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
 
++num_aborts;
 
@@ -5515,6 +5549,12 @@ static int scsi_debug_abort(struct scsi_cmnd *SCpnt)
"%s: command%s found\n", __func__,
ok ? "" : " not");
 
+   if (sdebug_fail_abort(SCpnt)) {
+   scmd_printk(KERN_INFO, SCpnt, "fail abort command 0x%x\n",
+   opcode);
+   return FAILED;
+   }
+
return SUCCESS;
 }
 
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-8-haowenchao2%40huawei.com.

[PATCH v5 08/10] scsi: scsi_debug: Add new error injection reset lun failed

['Wenchao Hao' via open-iscsi]

Add error injection type 3 to make scsi_debug_device_reset() return FAILED.
Fail abort command foramt:

  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error type, fixed to 0x4  |
  ++--+---+
  |   2|  s32 | Error count   |
  ||  |  0: this rule will be ignored |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+

Examples:
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "0 -10 0x12" > ${error}
will make the device return FAILED when try to reset lun with inquiry
command 10 times.
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "0 -10 0xff" > ${error}
will make the device return FAILED when try to reset lun 10 times.

Usually we do not care about what command it is when trying to perform
reset LUN, so 0xff could be applied.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 8a16cb9642a6..db8ab6cad078 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -295,6 +295,8 @@ enum sdebug_err_type {
/* with errors set in scsi_cmnd */
ERR_ABORT_CMD_FAILED= 3,/* control return FAILED from */
/* scsi_debug_abort() */
+   ERR_LUN_RESET_FAILED= 4,/* control return FAILED from */
+   /* 
scsi_debug_device_reseLUN_RESET_FAILEDt() */
 };
 
 struct sdebug_err_inject {
@@ -973,6 +975,7 @@ static int sdebug_error_show(struct seq_file *m, void *p)
switch (err->type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
seq_printf(m, "%d\t%d\t0x%x\n", err->type, err->cnt,
err->cmd);
break;
@@ -1035,6 +1038,7 @@ static ssize_t sdebug_error_write(struct file *file, 
const char __user *ubuf,
switch (inject_type) {
case ERR_TMOUT_CMD:
case ERR_ABORT_CMD_FAILED:
+   case ERR_LUN_RESET_FAILED:
if (sscanf(buf, "%d %d %hhx", >type, >cnt,
   >cmd) != 3)
goto out_error;
@@ -5578,10 +5582,40 @@ static void scsi_debug_stop_all_queued(struct 
scsi_device *sdp)
scsi_debug_stop_all_queued_iter, sdp);
 }
 
+static int sdebug_fail_lun_reset(struct scsi_cmnd *cmnd)
+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_LUN_RESET_FAILED &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
 static int scsi_debug_device_reset(struct scsi_cmnd *SCpnt)
 {
struct scsi_device *sdp = SCpnt->device;
struct sdebug_dev_info *devip = sdp->hostdata;
+   u8 *cmd = SCpnt->cmnd;
+   u8 opcode = cmd[0];
 
++num_dev_resets;
 
@@ -5592,6 +5626,11 @@ static int scsi_debug_device_reset(struct scsi_cmnd 
*SCpnt)
if (devip)
set_bit(SDEBUG_UA_POR, devip->uas_bm);
 
+   if (sdebug_fail_lun_reset(SCpnt)) {
+   scmd_printk(KERN_INFO, SCpnt, "fail lun reset 0x%x\n", opcode);
+   return FAILED;
+   }
+
return SUCCESS;
 }
 
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 

[PATCH v5 01/10] scsi: scsi_debug: create scsi_debug directory in the debugfs filesystem

['Wenchao Hao' via open-iscsi]

Create directory scsi_debug in the root of the debugfs filesystem.
Prepare to add interface for manage error injection.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 9c0af50501f9..35c336271b13 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -862,6 +863,8 @@ static const int device_qfull_result =
 
 static const int condition_met_result = SAM_STAT_CONDITION_MET;
 
+static struct dentry *sdebug_debugfs_root;
+
 
 /* Only do the extra work involved in logical block provisioning if one or
  * more of the lbpu, lbpws or lbpws10 parameters are given and we are doing
@@ -7011,6 +7014,8 @@ static int __init scsi_debug_init(void)
goto driver_unreg;
}
 
+   sdebug_debugfs_root = debugfs_create_dir("scsi_debug", NULL);
+
for (k = 0; k < hosts_to_add; k++) {
if (want_store && k == 0) {
ret = sdebug_add_host_helper(idx);
@@ -7057,6 +7062,7 @@ static void __exit scsi_debug_exit(void)
 
sdebug_erase_all_stores(false);
xa_destroy(per_store_ap);
+   debugfs_remove(sdebug_debugfs_root);
 }
 
 device_initcall(scsi_debug_init);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-2-haowenchao2%40huawei.com.

[PATCH v5 02/10] scsi: scsi_debug: Add interface to manage single device's error inject

['Wenchao Hao' via open-iscsi]

This new facility uses the debugfs pseudo file system which is typically
mounted under the /sys/kernel/debug directory and requires root permissions
to access.

The interface file is found at /sys/kernel/debug/scsi_debug//error
where  identifies the device (logical unit (LU)) to inject errors
on.

For the following description the ${error} environment variable is assumed
to be set to/sys/kernel/debug/scsi_debug/1:0:0:0/error where 1:0:0:0 is a
pseudo device (LU) owned by the scsi_debug driver. Rules are written to
${error} in the normal sysfs fashion (e.g. 'echo "0 -2 0x12" > ${error}').

More than one rule can be active on a device at a time and inactive rules
(i.e. those whose Error count is 0) remain in the rule listing. The
existing rules can be read with 'cat ${error}' with one line output for
each rule.

The interface format is line-by-line, each line is an error injection rule.
Each rule contains integers separated by spaces, the first three columns
correspond to "Error code", "Error count" and "SCSI command", other
columns depend on Error code.

General rule format:
  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error code|
  ||  |  0: timeout SCSI command  |
  ||  |  1: fail queuecommand, make queuecommand return   |
  ||  | given value   |
  ||  |  2: fail command, finish command with SCSI status,|
  ||  | sense key and ASC/ASCQ values |
  ||  |  3: make abort commands for specific command fail |
  ||  |  4: make reset lun for specific command fail  |
  ++--+---+
  |   2|  s32 | Error count   |
  ||  |  0: this rule will be ignored |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+
  |  ...   |  xxx | Error type specific fields|
  ++--+---+
Notes:
- when multiple error inject rules are added for the same SCSI command, the
  one with smaller error code will take effect (and the others will be
  ignored).
- if an same error (i.e. same Error code and SCSI command) is added, the
  older one will be overwritten.
- Currently, the basic types are (u8/u16/u32/u64/s8/s16/s32/s64) and the
  hexadecimal types (x8/x16/x32/x64)
- where a hexadecimal value is expected (e.g. Column 3: SCSI command
  opcode) the "0x" prefix is optional on the value (e.g. the INQUIRY opcode
  can be given as '0x12' or '12')
- when the Error count is negative, reading ${error} will show that value
  incrementing, stopping when it gets to 0

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 206 +-
 1 file changed, 202 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 35c336271b13..ca1e2f4878bc 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -286,6 +286,42 @@ struct sdeb_zone_state {   /* ZBC: per zone state */
sector_t z_wp;
 };
 
+enum sdebug_err_type {
+   ERR_TMOUT_CMD   = 0,/* make specific scsi command timeout */
+   ERR_FAIL_QUEUE_CMD  = 1,/* make specific scsi command's */
+   /* queuecmd return failed */
+   ERR_FAIL_CMD= 2,/* make specific scsi command's */
+   /* queuecmd return succeed but */
+   /* with errors set in scsi_cmnd */
+};
+
+struct sdebug_err_inject {
+   int type;
+   struct list_head list;
+   int cnt;
+   unsigned char cmd;
+   struct rcu_head rcu;
+
+   union {
+   /*
+* For ERR_FAIL_QUEUE_CMD
+*/
+   int queuecmd_ret;
+
+   /*
+* For ERR_FAIL_CMD
+*/
+   struct {
+   unsigned char host_byte;
+   unsigned char driver_byte;
+   unsigned char status_byte;
+   unsigned char sense_key;
+   unsigned 

[PATCH v5 00/10] scsi:scsi_debug: Add error injection for single device

['Wenchao Hao' via open-iscsi]

The original error injection mechanism was based on scsi_host which
could not inject fault for a single SCSI device.

This patchset provides the ability to inject errors for a single
SCSI device. Now we supports inject timeout errors, queuecommand
errors, and hostbyte, driverbyte, statusbyte, and sense data for
specific SCSI Command. Two new error injection is defined to make
abort command or reset LUN failed.

Besides error injection for single device, this patchset add a
new interface to make reset target failed for each scsi_target.

The first two patch add an debugfs interface to add and inquiry single
device's error injection info; the third patch defined how to remove
an injection which has been added. The following 5 patches use the
injection info and generate the related error type. The last one just
Add a new interface to make reset target failed.

V5:
  - Using rcu list to sync between error inject add, remove and check
  - Add module parameter "allow_restart" to control scsi_device's
allow_restart flag

V4:
  - Fix BUG_ON triggered by schedule in atomic context when rmmod scsi_debug
Closes: 
https://lore.kernel.org/oe-lkp/202308031027.5941ce5f-oliver.s...@intel.com

V3:
  - Add two more error types to fail abort command and lun reset
  - Fix memleak when rmmod scsi_debug without clearing errors injected
  - Fix memkeak because did not implement release in sdebug_error_fops
  - Fix possible NULL point access in scsi_debug_slave_destroy
  - Move specific error type's description to each single patch which
implement this error type
  - Add interface to make target reset fail

V2:
  - Using debugfs rather than sysfs attribute interface to manage error

Wenchao Hao (10):
  scsi: scsi_debug: create scsi_debug directory in the debugfs
filesystem
  scsi: scsi_debug: Add interface to manage single device's error inject
  scsi: scsi_debug: Define grammar to remove added error injection
  scsi: scsi_debug: timeout command if the error is injected
  scsi: scsi_debug: Return failed value if the error is injected
  scsi: scsi_debug: set command's result and sense data if the error is
injected
  scsi: scsi_debug: Add new error injection abort failed
  scsi: scsi_debug: Add new error injection reset lun failed
  scsi: scsi_debug: Add debugfs interface to fail target reset
  scsi: scsi_debug: Add param to control sdev's allow_restart

 drivers/scsi/scsi_debug.c | 557 +-
 1 file changed, 552 insertions(+), 5 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-1-haowenchao2%40huawei.com.

[PATCH v5 04/10] scsi: scsi_debug: timeout command if the error is injected

['Wenchao Hao' via open-iscsi]

If a timeout error is injected, return 0 from scsi_debug_queuecommand to
make the command timeout.

Timeout SCSI command format:
  ++--+---+
  | Column | Type | Description   |
  ++--+---+
  |   1|  u8  | Error type, fixed to 0x0  |
  ++--+---+
  |   2|  s32 | Error count   |
  ||  |  0: this rule will be ignored |
  ||  |  positive: the rule will always take effect   |
  ||  |  negative: the rule takes effect n times where -n is  |
  ||  |the value given. Ignored after n times |
  ++--+---+
  |   3|  x8  | SCSI command opcode, 0xff for all commands|
  ++--+---+
Examples:
error=/sys/kernel/debug/scsi_debug/0:0:0:1/error
echo "0 -10 0x12" > ${error}
will make the device's inquiry command timeout 10 times.
echo "0 1 0x12" > ${error}
will make the device's inquiry timeout each time it is invoked on this
device.

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6235e828c430..785ad5e5b4a4 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -7731,6 +7731,34 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, 
unsigned int queue_num)
return num_entries;
 }
 
+static int sdebug_timeout_cmd(struct scsi_cmnd *cmnd)
+{
+   struct scsi_device *sdp = cmnd->device;
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
+   struct sdebug_err_inject *err;
+   unsigned char *cmd = cmnd->cmnd;
+   int ret = 0;
+
+   if (devip == NULL)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == ERR_TMOUT_CMD &&
+   (err->cmd == cmd[0] || err->cmd == 0xff)) {
+   ret = !!err->cnt;
+   if (err->cnt < 0)
+   err->cnt++;
+
+   rcu_read_unlock();
+   return ret;
+   }
+   }
+   rcu_read_unlock();
+
+   return 0;
+}
+
 static int scsi_debug_queuecommand(struct Scsi_Host *shost,
   struct scsi_cmnd *scp)
 {
@@ -7789,6 +7817,12 @@ static int scsi_debug_queuecommand(struct Scsi_Host 
*shost,
if (NULL == devip)
goto err_out;
}
+
+   if (sdebug_timeout_cmd(scp)) {
+   scmd_printk(KERN_INFO, scp, "timeout command 0x%x\n", opcode);
+   return 0;
+   }
+
if (unlikely(inject_now && !atomic_read(_inject_pending)))
atomic_set(_inject_pending, 1);
 
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-5-haowenchao2%40huawei.com.

[PATCH v5 03/10] scsi: scsi_debug: Define grammar to remove added error injection

['Wenchao Hao' via open-iscsi]

The grammar to remove error injection is a line with fixed 3 columns
separated by spaces.

First column is fixed to "-". It tells this is a removal operation.
Second column is the error code to match.
Third column is the scsi command to match.

For example the following command would remove timeout injection of
inquiry command.
echo "- 0 0x12" > /sys/kernel/debug/scsi_debug/0:0:0:1/error

Acked-by: Douglas Gilbert 
Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_debug.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index ca1e2f4878bc..6235e828c430 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -930,6 +930,34 @@ static void sdebug_err_add(struct scsi_device *sdev, 
struct sdebug_err_inject *n
spin_unlock(>list_lock);
 }
 
+static int sdebug_err_remove(struct scsi_device *sdev, const char *buf, size_t 
count)
+{
+   struct sdebug_dev_info *devip = (struct sdebug_dev_info 
*)sdev->hostdata;
+   struct sdebug_err_inject *err;
+   int type;
+   unsigned char cmd;
+
+   if (sscanf(buf, "- %d %hhx", , ) != 2) {
+   kfree(buf);
+   return -EINVAL;
+   }
+
+   spin_lock(>list_lock);
+   list_for_each_entry_rcu(err, >inject_err_list, list) {
+   if (err->type == type && err->cmd == cmd) {
+   list_del_rcu(>list);
+   call_rcu(>rcu, sdebug_err_free);
+   spin_unlock(>list_lock);
+   kfree(buf);
+   return count;
+   }
+   }
+   spin_unlock(>list_lock);
+
+   kfree(buf);
+   return -EINVAL;
+}
+
 static int sdebug_error_show(struct seq_file *m, void *p)
 {
struct scsi_device *sdev = (struct scsi_device *)m->private;
@@ -987,6 +1015,9 @@ static ssize_t sdebug_error_write(struct file *file, const 
char __user *ubuf,
return -EFAULT;
}
 
+   if (buf[0] == '-')
+   return sdebug_err_remove(sdev, buf, count);
+
if (sscanf(buf, "%d", _type) != 1) {
kfree(buf);
return -EINVAL;
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20230922092906.2645265-4-haowenchao2%40huawei.com.

Re: [PATCH 0/2] scsi:donot skip lun if inquiry returns PQ=1 for all hosts

['Wenchao Hao' via open-iscsi]

On 2022/12/16 15:12, Christoph Hellwig wrote:
> On Thu, Dec 15, 2022 at 05:09:31PM +0800, Wenchao Hao wrote:
>> In my opinion, if the addressed lun still response the
>> inquiry and other commands, we should not skip it,
>> maybe let the scsi drivers like sd/st/sg to determine
>> how to handle this lun accordint to the PQ value.
>>
>> As discussed in following mail, another drivers would
>> be broken too.
> 
> So why do you force a specific behavior for iSCSI?
> .

For nothing, I want the iscsi_tcp transport do not skip PQ=1 default
as what it did before commit 948e922fc4461 ("scsi: core: map PQ=1,
PDT=other values to SCSI_SCAN_TARGET_PRESENT").

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/5129c84b-38e5-8be4-6411-fcc003776d70%40huawei.com.

[PATCH 2/2] scsi:iscsi_tcp:Do not skip lun inquiry returns PQ=1

['Wenchao Hao' via open-iscsi]

When luns inquiry return PQ=1, do not skip this lun and try to
map these luns to an sg device.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/iscsi_tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 5fb1f364e815..316e2e17c72d 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -941,6 +941,7 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, 
uint16_t cmds_max,
shost->max_id = 0;
shost->max_channel = 0;
shost->max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE;
+   shost->no_skip_pq1 = 1;
 
rc = iscsi_host_get_max_scsi_cmds(shost, cmds_max);
if (rc < 0)
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221214070846.1808300-3-haowenchao%40huawei.com.

[PATCH 0/2] scsi:donot skip lun if inquiry returns PQ=1 for all hosts

['Wenchao Hao' via open-iscsi]

commit 948e922fc4461 ("scsi: core: map PQ=1, PDT=other values to
SCSI_SCAN_TARGET_PRESENT") returns SCSI_SCAN_TARGET_PRESENT if inquiry
returns PQ=1.

According to the SPC, PQ=1 means the addressed logical unit having the
indicated device type is not accessible, it does not mean the addressed
logical unit is invalid. We still can map this lun to an sg device.

In some conditions, we do not want to skip these devices, for example
with iSCSI:

When iSCSI initiator logged in target, the target attached none valid
lun but lun0. lun0 is not an valid disk, while it would response
inquiry command with PQ=1 and other general scsi commands like probe lun.
The others luns of target is added/removed dynamicly.

We want the lun0 to be mapped to an sg device in initiator, so we can
probe luns of target based on lun0.

In first patch, I add an interface to control if to skip luns return
PQ=1 for inquiry.

In second patch, make iscsi_tcp do not skip luns return PQ=1 as default,
since I do not have iscsi_tcp environment, so here just modified the
iscsi_tcp.

Wenchao Hao (2):
  scsi:core:Add sysfs interface to control if skip lun with PQ=1
  scsi:iscsi_tcp:Do not skip lun inquiry returns PQ=1

 drivers/scsi/iscsi_tcp.c  |  1 +
 drivers/scsi/scsi_scan.c  |  9 ++---
 drivers/scsi/scsi_sysfs.c | 29 +
 include/scsi/scsi_host.h  |  3 +++
 4 files changed, 39 insertions(+), 3 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221214070846.1808300-1-haowenchao%40huawei.com.

[PATCH 1/2] scsi:core:Add sysfs interface to control if skip lun with PQ=1

['Wenchao Hao' via open-iscsi]

commit 948e922fc4461 ("scsi: core: map PQ=1, PDT=other values to
SCSI_SCAN_TARGET_PRESENT") returns SCSI_SCAN_TARGET_PRESENT if inquiry
returns PQ=1.

According to the SPC, PQ=1 means the addressed logical unit having the
indicated device type is not accessible, it does not mean the addressed
logical unit is invalid. We still can map this lun to an sg device.

In some conditions, we do not want to skip these devices, for example
with iSCSI:

When iSCSI initiator logged in target, the target attached none valid
lun but lun0. lun0 is not an valid disk, while it would response
inquiry command with PQ=1 and other general scsi commands like probe lun.
The others luns of target is added/removed dynamicly.

We want the lun0 to be mapped to an sg device in initiator, so we can
probe luns of target based on lun0.

I add an sysfs interface named no_skip_pq1 in each Scsi_Host to
control if to skip lun which return PQ=1 for inquiry.

The default behavior is not changed, which means we would still skip
add lun if inquiry returns PQ=1. We can set host's no_skip_pq1  in
specific drivers or via sysfs.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_scan.c  |  9 ++---
 drivers/scsi/scsi_sysfs.c | 29 +
 include/scsi/scsi_host.h  |  3 +++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 920b145f80b7..bd4faaabee8c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1233,10 +1233,13 @@ static int scsi_probe_and_add_lun(struct scsi_target 
*starget,
 * that no LUN is present, so don't add sdev in these cases.
 * Two specific examples are:
 * 1) NetApp targets: return PQ=1, PDT=0x1f
-* 2) IBM/2145 targets: return PQ=1, PDT=0
-* 3) USB UFI: returns PDT=0x1f, with the PQ bits being "reserved"
+* 2) USB UFI: returns PDT=0x1f, with the PQ bits being "reserved"
 *in the UFI 1.0 spec (we cannot rely on reserved bits).
 *
+* Targets set PQ=1 would be skipped if shost->no_skip_pq1 is not set
+* For example:
+* 1) IBM/2145 targets: return PQ=1, PDT=0
+*
 * References:
 * 1) SCSI SPC-3, pp. 145-146
 * PQ=1: "A peripheral device having the specified peripheral
@@ -1248,7 +1251,7 @@ static int scsi_probe_and_add_lun(struct scsi_target 
*starget,
 * PDT=00h Direct-access device (floppy)
 * PDT=1Fh none (no FDD connected to the requested logical unit)
 */
-   if (((result[0] >> 5) == 1 ||
+   if result[0] >> 5) == 1 && !shost->no_skip_pq1) ||
(starget->pdt_1f_for_no_lun && (result[0] & 0x1f) == 0x1f)) &&
!scsi_is_wlun(lun)) {
SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index f2a345cc0f8a..a72466c7e3c4 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -369,6 +369,34 @@ store_shost_eh_deadline(struct device *dev, struct 
device_attribute *attr,
 
 static DEVICE_ATTR(eh_deadline, S_IRUGO | S_IWUSR, show_shost_eh_deadline, 
store_shost_eh_deadline);
 
+static ssize_t
+show_no_skip_pq1(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+   struct Scsi_Host *shost = class_to_shost(dev);
+
+   return sysfs_emit(buf, "%s\n", shost->no_skip_pq1 ? "Y" : "N");
+}
+
+static ssize_t
+store_no_skip_pq1(struct device *dev, struct device_attribute *attr,
+   const char *buf, size_t count)
+{
+   struct Scsi_Host *shost = class_to_shost(dev);
+   int ret = -EINVAL;
+   bool store_val;
+
+   ret = kstrtobool(buf, _val);
+   if (ret)
+   return ret;
+
+   shost->no_skip_pq1 = store_val;
+
+   return count;
+}
+
+static DEVICE_ATTR(no_skip_pq1, S_IRUGO | S_IWUSR, show_no_skip_pq1, 
store_no_skip_pq1);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(can_queue, "%d\n");
@@ -421,6 +449,7 @@ static struct attribute *scsi_sysfs_shost_attrs[] = {
_attr_host_reset.attr,
_attr_eh_deadline.attr,
_attr_nr_hw_queues.attr,
+   _attr_no_skip_pq1.attr,
NULL
 };
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 587cc767bb67..56bb11d9a886 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -659,6 +659,9 @@ struct Scsi_Host {
/* The transport requires the LUN bits NOT to be stored in CDB[1] */
unsigned no_scsi2_lun_in_cdb:1;
 
+   /* Do not skip adding lun if inquiry command returns PQ == 1 */
+   unsigned no_skip_pq1:1;
+
/*
 * Optional work queue to be utilized by the transport
 */
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to 

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

['Wenchao Hao' via open-iscsi]

On Wednesday, December 7, 2022 at 1:01:23 AM UTC+8 Wenchao Hao wrote:

> On Tue, Dec 6, 2022 at 1:13 AM Lee Duncan  wrote: 
> > 
> > On 12/4/22 05:02, Wenchao Hao wrote: 
> > > On Thu, Dec 1, 2022 at 3:53 AM Lee Duncan  wrote: 
> > >> 
> > ... 
> > >> 
> > >> Let me start by saying I agree with you now, that there *is* an 
> issue. 
> > >> But your test was flawed. 
> > >> 
> > >> After you log into a target, changing the Node database does nothing. 
> > >> The node database is only referenced with you login using it, e.g. 
> > >> "iscsiadm -m node ... -l". 
> > >> 
> > >> But even if you logged out and then back into the target, thereby 
> using 
> > >> the updated Node DB entries, it would not have worked. 
> > >> 
> > >> For one thing, "iscsiadm -m session -u" just logs out of all 
> sessions, 
> > >> as far as I can see, based on testing and code inspection. So that is 
> a 
> > >> problem. 
> > >> 
> > >> Note that the iscsi.service systemd service file on SLES does "not" 
> do 
> > >> that. It instead logs of of "manual" and "automatic" session, but 
> only 
> > >> ones that are listed in the Node database. 
> > >> 
> > >> And as you pointed out, any knowledge iscsid has of existing sessions 
> is 
> > >> lost if the daemmon dies or is stopped, then restarted. At that 
> point, 
> > >> the only knowledge is has about each session is what it finds in 
> sysfs. 
> > >> 
> > >> I have done some testing with your new kernel change that adds a 
> > >> "node_startup" sysfs string attribute to session data. I modified 
> > >> open-iscsi to pass in the node startup value, and that's a good 
> start. 
> > >> The next step is adding a "startup" value in the session structure, 
> > >> filling it in from sysfs (or current state), and refusing to logout 
> out 
> > >> of sesions that have this set to "onboot", which all sounds fairly 
> > >> simple. I also want to test with "iscsiadm -m fw -l", which is what I 
> > >> believe is used when booting from software iscsi (i.e. iBFT). 
> > >> 
> > >> Have you already worked on the open-iscsi side of this? No reason for 
> > >> duplicate development. 
> > >> 
> > >> -- 
> > >> Lee Duncan 
> > >> 
> > > 
> > > Sorry I missed this message, I have modified open-iscsi to work 
> > > with this sysfs interface. But I think we do not need this any more 
> > > because the safe logout can avoid disks being removed. 
> > > 
> > > Checking holders and if disk is mounted before logout seems enough, 
> > > so ignore this discussion. 
> > > 
> > > Thank you very much for your reply. 
> > 
> > I have some philosophical issues with using safe_logout. 
> > 
> > It is off by default, which implies to me that it has overhead. If I'm 
> > doing a lot of iscsi session start/stops, I don't want the overhead. 
> > Otherwise, why not just use it all the time. 
> > 
> > Also, it only checks for mounts. What about if some process has the 
> > device open but isn't using it for a filesystem? 
> > 
> > And since it has overhead, I'd rather just use it on root iscsi volumes. 
> > I have not had a single problem report from folks that have ended a 
> > session by accident that is mounted on. Since ending your root volume 
> > iscsi session is fatal, I _would_ like to proactively avoid that 
> > possibility. So I want to only set this attribute on iscsi root volumes, 
> > which means it'd have to be a per-node (or per-session) attribute, not a 
> > global one. 
> > 
> > Lastly, I can imagine a time when I want to override safe_logout, say if 
> > some process is stuck. So it'd be nice to have a "--force" option to end 
> > a session even if safe_logout is set. 
> > 
> > But as I said, these objections are philosophical/theoretical. 
> > 
> > And for the record, I like the idea of tracking the "start mode" of 
> > sessions. Right now, if I list the iscsi sessions, I can't tell which 
> > ones where started from firmware, which were started in the initrd, and 
> > which were just manually started. So tracking (and being able to 
> > display) the startup mode would only be a good thing IMHO. 
> > -- 
> > Lee D 
> > 
> >


My previous reply was in a mess format, here is the formatted one: 

The original purpose of my patch is to record session's startup_mode
in the kernel, so iscsiadm/iscsid can refer to it before logout session,
this can fix both the two issues.

While the safe_logout mode can solve the first issue in another way by
checking if iscsi disk is in used. But we did not enable safe_logout as
default and it can not cover the sense when iscsi disks in not mounted
nor used by multipath or lvm.

The safe_logout mode can not address the issue which iscsiadm/iscsid
can overwrite session's startup_mode.

Firstly, we need to come to one same conclusion that if it is a bug to
be fixed, then discuss how to fix it.

If we treat this issue as a bug, we have two ways to fix it:

1. Recording session's startup_mode in kernel is a way to fix it
2. Checking if session has been created for this node before 

Re: [PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

On 2022/11/26 9:07, Wenchao Hao wrote:
> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
> for multiple times which should be fixed.
> 
> This patch introduce target_state in iscsi_cls_session to make
> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
> 
> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
> Report unbind session event when the target has been removed"). The issue
> is iscsid died for any reason after it send unbind session to kernel, once
> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
> 
> Now kernel think iscsi_cls_session has already sent an
> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
> would cause userspace unable to logout. Actually the session is in
> invalid state(it's target_id is INVALID), iscsid should not sync this
> session in it's restart.
> 
> So we need to check session's target state during iscsid restart,
> if session is in unbound state, do not sync this session and perform
> session teardown. It's reasonable because once a session is unbound, we
> can not recover it any more(mainly because it's target id is INVALID)
> 
> V7:
> - Define target state to string map and refer this map directly
> - Cleanup __iscsi_unbind_session, drop check for session's
>   target_id == ISCSI_MAX_TARGET since it can be handled by target_state
> 
> V6:
> - Set target state to ALLOCATED in iscsi_add_session
> - Rename state BOUND to SCANNED
> - Make iscsi_session_target_state_name() more efficient
> 
> V5:
> - Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
>   target has been allocated but not scanned yet. We should
>   sync this session and scan this session when iscsid restarted.
> 
> V4:
> - Move debug print out of spinlock critical section
> 
> V3:
> - Make target bind state to a state kind rather than a bool.
> 
> V2:
> - Using target_unbound rather than state to indicate session has been
>   unbound
> 
> Signed-off-by: Wenchao Hao 
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 47 ++---
>  include/scsi/scsi_transport_iscsi.h |  9 ++
>  2 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index cd3db9684e52..812578c20fe5 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1676,6 +1676,13 @@ static const char *iscsi_session_state_name(int state)
>   return name;
>  }
>  
> +static char *iscsi_session_target_state_name[] = {
> + [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
> + [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
> + [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
> + [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
> +};
> +
>  int iscsi_session_chkready(struct iscsi_cls_session *session)
>  {
>   int err;
> @@ -1785,9 +1792,13 @@ static int iscsi_user_scan_session(struct device *dev, 
> void *data)
>   if ((scan_data->channel == SCAN_WILD_CARD ||
>scan_data->channel == 0) &&
>   (scan_data->id == SCAN_WILD_CARD ||
> -  scan_data->id == id))
> +  scan_data->id == id)) {
>   scsi_scan_target(>dev, 0, id,
>scan_data->lun, scan_data->rescan);
> + spin_lock_irqsave(>lock, flags);
> + session->target_state = ISCSI_SESSION_TARGET_SCANNED;
> + spin_unlock_irqrestore(>lock, flags);
> + }
>   }
>  
>  user_scan_exit:
> @@ -1960,31 +1971,41 @@ static void __iscsi_unbind_session(struct work_struct 
> *work)
>   struct iscsi_cls_host *ihost = shost->shost_data;
>   unsigned long flags;
>   unsigned int target_id;
> + bool remove_target = true;
>  
>   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>   /* Prevent new scans and make sure scanning is not in progress */
>   mutex_lock(>mutex);
>   spin_lock_irqsave(>lock, flags);
> - if (session->target_id == ISCSI_MAX_TARGET) {
> + if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
> + remove_target = false;
> + } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
> - goto unbind_session_exit;
> + ISCSI_DBG_TRANS_SESSION(session,
> + "Skipping target unbinding: Session is 
> unbound/unbinding.\n");
> + return;
>   }
>  
> + session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
>   target_id = session->target_id;
>   session->target_id = ISCSI_MAX_TARGET;
>   spin_unlock_irqrestore(>lock, flags);
>   mutex_unlock(>mutex);
>  
> - scsi_remove_target(>dev);
> + if (remove_target)
> + scsi_remove_target(>dev);
>  
>   if 

[PATCH v7] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
for multiple times which should be fixed.

This patch introduce target_state in iscsi_cls_session to make
sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.

But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
Report unbind session event when the target has been removed"). The issue
is iscsid died for any reason after it send unbind session to kernel, once
iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.

Now kernel think iscsi_cls_session has already sent an
ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
would cause userspace unable to logout. Actually the session is in
invalid state(it's target_id is INVALID), iscsid should not sync this
session in it's restart.

So we need to check session's target state during iscsid restart,
if session is in unbound state, do not sync this session and perform
session teardown. It's reasonable because once a session is unbound, we
can not recover it any more(mainly because it's target id is INVALID)

V7:
- Define target state to string map and refer this map directly
- Cleanup __iscsi_unbind_session, drop check for session's
  target_id == ISCSI_MAX_TARGET since it can be handled by target_state

V6:
- Set target state to ALLOCATED in iscsi_add_session
- Rename state BOUND to SCANNED
- Make iscsi_session_target_state_name() more efficient

V5:
- Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
  target has been allocated but not scanned yet. We should
  sync this session and scan this session when iscsid restarted.

V4:
- Move debug print out of spinlock critical section

V3:
- Make target bind state to a state kind rather than a bool.

V2:
- Using target_unbound rather than state to indicate session has been
  unbound

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 47 ++---
 include/scsi/scsi_transport_iscsi.h |  9 ++
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..812578c20fe5 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,13 @@ static const char *iscsi_session_state_name(int state)
return name;
 }
 
+static char *iscsi_session_target_state_name[] = {
+   [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
+   [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
+   [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
+   [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
+};
+
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
int err;
@@ -1785,9 +1792,13 @@ static int iscsi_user_scan_session(struct device *dev, 
void *data)
if ((scan_data->channel == SCAN_WILD_CARD ||
 scan_data->channel == 0) &&
(scan_data->id == SCAN_WILD_CARD ||
-scan_data->id == id))
+scan_data->id == id)) {
scsi_scan_target(>dev, 0, id,
 scan_data->lun, scan_data->rescan);
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_SCANNED;
+   spin_unlock_irqrestore(>lock, flags);
+   }
}
 
 user_scan_exit:
@@ -1960,31 +1971,41 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;
+   bool remove_target = true;
 
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
-   if (session->target_id == ISCSI_MAX_TARGET) {
+   if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+   remove_target = false;
+   } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
-   goto unbind_session_exit;
+   ISCSI_DBG_TRANS_SESSION(session,
+   "Skipping target unbinding: Session is 
unbound/unbinding.\n");
+   return;
}
 
+   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
 
-   scsi_remove_target(>dev);
+   if (remove_target)
+   scsi_remove_target(>dev);
 
if (session->ida_used)
ida_free(_sess_ida, target_id);
 
-unbind_session_exit:
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, 

Re: [EXT] Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

['Wenchao Hao' via open-iscsi]

On Thursday, November 24, 2022 at 6:06:09 PM UTC+8 Uli wrote:

> >>> "'Lee Duncan' via open-iscsi"  schrieb am 
> 23.11.2022 um 17:47 in Nachricht 
> <0f7258d5-ff8e-fa4e...@suse.com>: 
> > On 11/22/22 20:41, Wenchao Hao wrote: 
>
> ... 
> > Again, I don't believe that's correct. I will test it. 
> ... 
> Maybe a session capture (via serial line or so) to show real facts would 
> be helpful for the discussion.


Sorry, I can not understand this, could you describe more detail?
 

>
> Personally I think that information the kernel needs to continue working 
> (e.g. the mount table) should be in the kernel. 
> Maybe user-land tools can manage the info there (in the kernel, via API), 
> but the primary source should be the kernel. 
>
> Regards, 
> Ulrich 
>
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2d0439ba-7fb7-47ef-b52c-a866dc0c86e1n%40googlegroups.com.

Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

On 2022/11/23 2:15, Mike Christie wrote:
> On 11/22/22 11:29 AM, Wenchao Hao wrote:
>> On Wed, Nov 23, 2022 at 1:04 AM Mike Christie
>>  wrote:
>>>
>>> On 11/21/22 8:17 AM, Wenchao Hao wrote:
 And the function looks like following after change:

 static void __iscsi_unbind_session(struct work_struct *work)
 {
   struct iscsi_cls_session *session =
   container_of(work, struct iscsi_cls_session,
unbind_work);
   struct Scsi_Host *shost = iscsi_session_to_shost(session);
   struct iscsi_cls_host *ihost = shost->shost_data;
   unsigned long flags;
   unsigned int target_id;

   ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");

   /* Prevent new scans and make sure scanning is not in progress */
   mutex_lock(>mutex);
   spin_lock_irqsave(>lock, flags);
   if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
>>>
>>> What was the reason for not checking for ALLOCATED and freeing the ida
>>> in that case?
>>>
>>
>> target_state would be in "ALLOCATED" state if iscsid died after add
>> session successfully.
>> When iscsid restarted, if the session's target_state is "ALLOCATED",
>> it should scan
>> the session and the target_state would switch to "SCANNED".
>>
>> So I think we would not call in __iscsi_unbind_session() with
>> session's target_state
>> is ALLOCATED.
> 
> Makes sense for the normal case.
> 
> The only issue is when __iscsi_unbind_session is called via
> iscsi_remove_session for the cases where userspace didn't do
> the  UNBIND event. Some tools don't do unbind or open-iscsi
> sometimes doesn't if the session is down. We will leak the ida,
> so you need some code to handle that.
> 
> .

Sorry, I did not take this condition in consideration. I would change
the __iscsi_unbind_session as following:

1. do not check if target_id is ISCSI_MAX_TARGET
2. define remove_target and default set to true, if target_state is ALLOCATED, 
then set
   it to false and continue the unbind flow; else if target_state not SCANNED, 
just return.
3. set target_state to ISCSI_SESSION_TARGET_UNBOUND after is sent to avoid 
potential race condition.

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..9264c75ad9ea 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1960,31 +1960,40 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;
+   bool remove_target = true;
 
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
-   if (session->target_id == ISCSI_MAX_TARGET) {
+   if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+   remove_target = false;
+   } else if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
-   goto unbind_session_exit;
+   ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: 
Session is unbound/unbinding.\n");
+   return;
}
 
+   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
 
-   scsi_remove_target(>dev);
+   if (remove_target)
+   scsi_remove_target(>dev);
 
if (session->ida_used)
ida_free(_sess_ida, target_id);
 
-unbind_session_exit:
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
+
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+   spin_unlock_irqrestore(>lock, flags);
 }

And the function would be:

static void __iscsi_unbind_session(struct work_struct *work)
{
struct iscsi_cls_session *session =
container_of(work, struct iscsi_cls_session,
 unbind_work);
struct Scsi_Host *shost = iscsi_session_to_shost(session);
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;
bool remove_target = true;

ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");

/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
remove_target = false;
} else if 

[PATCH v3 2/2] scsi: donot increase scsi_device's iorequest_cnt if dispatch failed

['Wenchao Hao' via open-iscsi]

If scsi_dispatch_cmd() failed, the scsi command did not send to disks,
so it would never done from LLDs.

scsi scsi_queue_rq() would return BLK_STS_RESOURCE if
scsi_dispatch_cmd() failed, the related request would be requeued, and
the timeout of this request would not fired any more, so no one
would increase iodone_cnt which matches with this increase of
iorequest_cnt.

Signed-off-by: Wenchao Hao 
Reviewed-by: Mike Christie 
---
 drivers/scsi/scsi_lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ec890865abae..a29d87e57430 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1464,8 +1464,6 @@ static int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
struct Scsi_Host *host = cmd->device->host;
int rtn = 0;
 
-   atomic_inc(>device->iorequest_cnt);
-
/* check if the device is still usable */
if (unlikely(cmd->device->sdev_state == SDEV_DEL)) {
/* in SDEV_DEL we error all commands. DID_NO_CONNECT
@@ -1764,6 +1762,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx 
*hctx,
goto out_dec_host_busy;
}
 
+   atomic_inc(>device->iorequest_cnt);
return BLK_STS_OK;
 
 out_dec_host_busy:
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221123122137.150776-3-haowenchao%40huawei.com.

[PATCH v3 1/2] scsi: increase scsi device's iodone_cnt in scsi_timeout()

['Wenchao Hao' via open-iscsi]

If an scsi command time out and going to be aborted, we should
increase the iodone_cnt of the related scsi device, or the
iodone_cnt would be less than iorequest_cnt

Increase iodone_cnt in scsi_timeout() would not cause double
accounting issue, briefly analysed as following:

 - we add the iodone_cnt when BLK_EH_DONE would be returned in
   scsi_timeout(), so the related scsi command's timeout event
   would not happened

 - if the abort succeed and do not retry, the command would be done
   with scsi_finish_command() which would not increase iodone_cnt;

 - if the abort succeed and retry the command, it would be requeue,
   a scsi_dispatch_cmd() would be called and iorequest_cnt would be
   increased again

 - if the abort failed, the error handler successfully recover the
   device, do not retry this command, the command would be done
   with scsi_finish_command() which would not increase iodone_cnt;

 - if the abort failed, the error handler successfully recover the
   device, and retry this command, the iorequest_cnt would be
   increased again

Signed-off-by: Wenchao Hao 
Reviewed-by: Mike Christie 
---
 drivers/scsi/scsi_error.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index be2a70c5ac6d..613d5aeb1e3c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -354,6 +354,7 @@ enum blk_eh_timer_return scsi_timeout(struct request *req)
 */
if (test_and_set_bit(SCMD_STATE_COMPLETE, >state))
return BLK_EH_DONE;
+   atomic_inc(>device->iodone_cnt);
if (scsi_abort_command(scmd) != SUCCESS) {
set_host_byte(scmd, DID_TIME_OUT);
scsi_eh_scmd_add(scmd);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221123122137.150776-2-haowenchao%40huawei.com.

[PATCH v3 0/2] Fix scsi device's iodone_cnt mismatch with iorequest_cnt

['Wenchao Hao' via open-iscsi]

Following scenario would make scsi_device's iodone_cnt mismatch with
iorequest_cnt even if there is no request on this device any more.
   
1. request timeout happened. If we do not retry the timeouted command,
   this command would be finished in scsi_finish_command() which would
   not increase the iodone_cnt; if the timeouted command is retried,
   another increasement for iorequest_cnt would be performed, the
   command might add iorequest_cnt for multiple times but iodone_cnt
   only once. Increase iodone_cnt in scsi_timeout() can handle this
   scenario.

2. scsi_dispatch_cmd() failed, while the iorequest_cnt has already been
   increased. If scsi_dispatch_cmd() failed, the request would be
   requeued, then another iorequest_cnt would be added. So we should not
   increase iorequest_cnt if dispatch command failed

V3:
- Rebase to solve conflicts caused by context when apply patch

V2:
- Add description about why we can add iodone_cnt in scsi_timeout()
- Do not increase iorequest_cnt if dispatch command failed

Wenchao Hao (2):
  scsi: increase scsi device's iodone_cnt in scsi_timeout()
  scsi: donot increase scsi_device's iorequest_cnt if dispatch failed

 drivers/scsi/scsi_error.c | 1 +
 drivers/scsi/scsi_lib.c   | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221123122137.150776-1-haowenchao%40huawei.com.

Re: [PATCH] scsi:iscsi: Record session's startup mode in kernel

['Wenchao Hao' via open-iscsi]

On 2022/11/23 4:00, Lee Duncan wrote:
> On 11/22/22 13:30, Wenchao Hao wrote:
>> There are 3 iscsi session's startup mode which are onboot, manual and
>> automatic. We can boot from iSCSI disks with help of dracut's service
>> in initrd, which would set node's startup mode to onboot, then create
>> iSCSI sessions.
>>
>> While the configure of onboot mode is recorded in file of initrd stage
>> and would be lost when switch to rootfs. Even if we update the startup
>> mode to onboot by hand after switch to rootfs, it is possible that the
>> configure would be covered by another discovery command.
>>
>> root would be mounted on iSCSI disks when boot from iSCSI disks, if the
>> sessions is logged out, the related disks would be removed, which would
>> cause the whole system halt.
>>
>> So we need record session's start up mode in kernel and check this
>> mode before logout this session.
>>
>> Signed-off-by: Wenchao Hao 
>> ---
>>   drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
>>   drivers/scsi/be2iscsi/be_iscsi.c | 1 +
>>   drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 +
>>   drivers/scsi/cxgbi/libcxgbi.c    | 1 +
>>   drivers/scsi/iscsi_tcp.c | 1 +
>>   drivers/scsi/libiscsi.c  | 5 +
>>   drivers/scsi/qedi/qedi_iscsi.c   | 1 +
>>   drivers/scsi/qla4xxx/ql4_os.c    | 1 +
>>   drivers/scsi/scsi_transport_iscsi.c  | 4 
>>   include/scsi/iscsi_if.h  | 1 +
>>   include/scsi/libiscsi.h  | 1 +
>>   11 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
>> b/drivers/infiniband/ulp/iser/iscsi_iser.c
>> index 620ae5b2d80d..778c023673ea 100644
>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
>> @@ -947,6 +947,7 @@ static umode_t iser_attr_is_visible(int param_type, int 
>> param)
>>   case ISCSI_PARAM_IFACE_NAME:
>>   case ISCSI_PARAM_INITIATOR_NAME:
>>   case ISCSI_PARAM_DISCOVERY_SESS:
>> +    case ISCSI_PARAM_NODE_STARTUP:
>>   return S_IRUGO;
>>   default:
>>   return 0;
>> diff --git a/drivers/scsi/be2iscsi/be_iscsi.c 
>> b/drivers/scsi/be2iscsi/be_iscsi.c
>> index 8aeaddc93b16..a21a4d9ab8b8 100644
>> --- a/drivers/scsi/be2iscsi/be_iscsi.c
>> +++ b/drivers/scsi/be2iscsi/be_iscsi.c
>> @@ -1401,6 +1401,7 @@ umode_t beiscsi_attr_is_visible(int param_type, int 
>> param)
>>   case ISCSI_PARAM_LU_RESET_TMO:
>>   case ISCSI_PARAM_IFACE_NAME:
>>   case ISCSI_PARAM_INITIATOR_NAME:
>> +    case ISCSI_PARAM_NODE_STARTUP:
>>   return S_IRUGO;
>>   default:
>>   return 0;
>> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c 
>> b/drivers/scsi/bnx2i/bnx2i_iscsi.c
>> index a3c800e04a2e..d1fb06d8a92e 100644
>> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
>> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
>> @@ -2237,6 +2237,7 @@ static umode_t bnx2i_attr_is_visible(int param_type, 
>> int param)
>>   case ISCSI_PARAM_BOOT_ROOT:
>>   case ISCSI_PARAM_BOOT_NIC:
>>   case ISCSI_PARAM_BOOT_TARGET:
>> +    case ISCSI_PARAM_NODE_STARTUP:
>>   return S_IRUGO;
>>   default:
>>   return 0;
>> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
>> index af281e271f88..111b2ac78964 100644
>> --- a/drivers/scsi/cxgbi/libcxgbi.c
>> +++ b/drivers/scsi/cxgbi/libcxgbi.c
>> @@ -3063,6 +3063,7 @@ umode_t cxgbi_attr_is_visible(int param_type, int 
>> param)
>>   case ISCSI_PARAM_TGT_RESET_TMO:
>>   case ISCSI_PARAM_IFACE_NAME:
>>   case ISCSI_PARAM_INITIATOR_NAME:
>> +    case ISCSI_PARAM_NODE_STARTUP:
>>   return S_IRUGO;
>>   default:
>>   return 0;
>> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
>> index 5fb1f364e815..47a73fb3e4b0 100644
>> --- a/drivers/scsi/iscsi_tcp.c
>> +++ b/drivers/scsi/iscsi_tcp.c
>> @@ -1036,6 +1036,7 @@ static umode_t iscsi_sw_tcp_attr_is_visible(int 
>> param_type, int param)
>>   case ISCSI_PARAM_TGT_RESET_TMO:
>>   case ISCSI_PARAM_IFACE_NAME:
>>   case ISCSI_PARAM_INITIATOR_NAME:
>> +    case ISCSI_PARAM_NODE_STARTUP:
>>   return S_IRUGO;
>>   default:
>>   return 0;
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index d95f4bcdeb2e..1f2b0a9a029e 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -3576,6 +3576,8 @@ int iscsi_set_param(struct iscsi_cls_conn *cls_conn,
>>   break;
>>   case ISCSI_PARAM_LOCAL_IPADDR:
>>   return iscsi_switch_str_param(>local_ipaddr, buf);
>> +    case ISCSI_PARAM_NODE_STARTUP:
>> +    return iscsi_switch_str_param(>node_startup, buf);
>>   default:
>>   return -ENOSYS;
>>   }
>> @@ -3712,6 +3714,9 @@ int iscsi_session_get_param(struct iscsi_cls_session 
>> *cls_session,
>>   

[PATCH] scsi:iscsi: Record session's startup mode in kernel

['Wenchao Hao' via open-iscsi]

There are 3 iscsi session's startup mode which are onboot, manual and
automatic. We can boot from iSCSI disks with help of dracut's service
in initrd, which would set node's startup mode to onboot, then create
iSCSI sessions.

While the configure of onboot mode is recorded in file of initrd stage
and would be lost when switch to rootfs. Even if we update the startup
mode to onboot by hand after switch to rootfs, it is possible that the
configure would be covered by another discovery command.

root would be mounted on iSCSI disks when boot from iSCSI disks, if the
sessions is logged out, the related disks would be removed, which would
cause the whole system halt.

So we need record session's start up mode in kernel and check this
mode before logout this session.

Signed-off-by: Wenchao Hao 
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 1 +
 drivers/scsi/be2iscsi/be_iscsi.c | 1 +
 drivers/scsi/bnx2i/bnx2i_iscsi.c | 1 +
 drivers/scsi/cxgbi/libcxgbi.c| 1 +
 drivers/scsi/iscsi_tcp.c | 1 +
 drivers/scsi/libiscsi.c  | 5 +
 drivers/scsi/qedi/qedi_iscsi.c   | 1 +
 drivers/scsi/qla4xxx/ql4_os.c| 1 +
 drivers/scsi/scsi_transport_iscsi.c  | 4 
 include/scsi/iscsi_if.h  | 1 +
 include/scsi/libiscsi.h  | 1 +
 11 files changed, 18 insertions(+)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c 
b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 620ae5b2d80d..778c023673ea 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -947,6 +947,7 @@ static umode_t iser_attr_is_visible(int param_type, int 
param)
case ISCSI_PARAM_IFACE_NAME:
case ISCSI_PARAM_INITIATOR_NAME:
case ISCSI_PARAM_DISCOVERY_SESS:
+   case ISCSI_PARAM_NODE_STARTUP:
return S_IRUGO;
default:
return 0;
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 8aeaddc93b16..a21a4d9ab8b8 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -1401,6 +1401,7 @@ umode_t beiscsi_attr_is_visible(int param_type, int param)
case ISCSI_PARAM_LU_RESET_TMO:
case ISCSI_PARAM_IFACE_NAME:
case ISCSI_PARAM_INITIATOR_NAME:
+   case ISCSI_PARAM_NODE_STARTUP:
return S_IRUGO;
default:
return 0;
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index a3c800e04a2e..d1fb06d8a92e 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -2237,6 +2237,7 @@ static umode_t bnx2i_attr_is_visible(int param_type, int 
param)
case ISCSI_PARAM_BOOT_ROOT:
case ISCSI_PARAM_BOOT_NIC:
case ISCSI_PARAM_BOOT_TARGET:
+   case ISCSI_PARAM_NODE_STARTUP:
return S_IRUGO;
default:
return 0;
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index af281e271f88..111b2ac78964 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -3063,6 +3063,7 @@ umode_t cxgbi_attr_is_visible(int param_type, int param)
case ISCSI_PARAM_TGT_RESET_TMO:
case ISCSI_PARAM_IFACE_NAME:
case ISCSI_PARAM_INITIATOR_NAME:
+   case ISCSI_PARAM_NODE_STARTUP:
return S_IRUGO;
default:
return 0;
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 5fb1f364e815..47a73fb3e4b0 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -1036,6 +1036,7 @@ static umode_t iscsi_sw_tcp_attr_is_visible(int 
param_type, int param)
case ISCSI_PARAM_TGT_RESET_TMO:
case ISCSI_PARAM_IFACE_NAME:
case ISCSI_PARAM_INITIATOR_NAME:
+   case ISCSI_PARAM_NODE_STARTUP:
return S_IRUGO;
default:
return 0;
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d95f4bcdeb2e..1f2b0a9a029e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3576,6 +3576,8 @@ int iscsi_set_param(struct iscsi_cls_conn *cls_conn,
break;
case ISCSI_PARAM_LOCAL_IPADDR:
return iscsi_switch_str_param(>local_ipaddr, buf);
+   case ISCSI_PARAM_NODE_STARTUP:
+   return iscsi_switch_str_param(>node_startup, buf);
default:
return -ENOSYS;
}
@@ -3712,6 +3714,9 @@ int iscsi_session_get_param(struct iscsi_cls_session 
*cls_session,
else
len = sysfs_emit(buf, "\n");
break;
+   case ISCSI_PARAM_NODE_STARTUP:
+   len = 

[PATCH] scsi:iscsi: rename iscsi_set_param to iscsi_if_set_param

['Wenchao Hao' via open-iscsi]

There are two iscsi_set_param() functions individually defined
in libiscsi.c and scsi_transport_iscsi.c which is confused.

So rename the one in scsi_transport_iscsi.c to iscsi_if_set_param().

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..c3fe5ecfee59 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2988,7 +2988,7 @@ iscsi_if_destroy_conn(struct iscsi_transport *transport, 
struct iscsi_uevent *ev
 }
 
 static int
-iscsi_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
 {
char *data = (char*)ev + sizeof(*ev);
struct iscsi_cls_conn *conn;
@@ -3941,7 +3941,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr 
*nlh, uint32_t *group)
err = -EINVAL;
break;
case ISCSI_UEVENT_SET_PARAM:
-   err = iscsi_set_param(transport, ev);
+   err = iscsi_if_set_param(transport, ev);
break;
case ISCSI_UEVENT_CREATE_CONN:
case ISCSI_UEVENT_DESTROY_CONN:
-- 
2.35.3

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20221122181105.4123935-1-haowenchao%40huawei.com.

Re: [PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

On 2022/11/9 11:47, Mike Christie wrote:
> On 11/7/22 7:44 PM, Wenchao Hao wrote:
>> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
>> for multiple times which should be fixed.
>>  
>> +static char *iscsi_session_target_state_names[] = {
>> +"UNBOUND",
>> +"ALLOCATED",
>> +"SCANNED",
>> +"UNBINDING",
>> +};
> 
> I think maybe Lee meant you to do something like:
> 
> static int iscsi_target_state_to_name[] = {
>   [ISCSI_SESSION_TARGET_UNBOUND] = "UNBOUND",
>   [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
>   .
> 
> 

Define array as following and remove previous helper function:

static char *iscsi_session_target_state_name[] = {
   [ISCSI_SESSION_TARGET_UNBOUND]   = "UNBOUND",
   [ISCSI_SESSION_TARGET_ALLOCATED] = "ALLOCATED",
   [ISCSI_SESSION_TARGET_SCANNED]   = "SCANNED",
   [ISCSI_SESSION_TARGET_UNBINDING] = "UNBINDING",
};

Reference the array directly:

static ssize_t
show_priv_session_target_state(struct device *dev, struct device_attribute 
*attr,
   char *buf)
{
   struct iscsi_cls_session *session = iscsi_dev_to_session(dev->parent);
   return sysfs_emit(buf, "%s\n",
   iscsi_session_target_state_name[session->target_state]);
}

>> +spin_lock_irqsave(>lock, flags);
>> +if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
>> +spin_unlock_irqrestore(>lock, flags);
>> +if (session->ida_used)
>> +ida_free(_sess_ida, session->target_id);
>> +ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: 
>> allocated\n");
> 
> Could you change the error message to "Skipping target unbinding: Session not 
> yet scanned.\n"
> 
>> +goto unbind_session_exit;
>> +}
> 
> Just add a newline/return here.

Actually we should skip unbind this session if call into 
__iscsi_unbind_session() with target state
is ALLOCATED. So I removed the check, and check only one condition in 
__iscsi_unbind_session(): if the
target state is SCANNED.

> 
> I think you want to move both state checks to after the we take the host lock 
> and
> session lock after the line above. You don't have to take the lock multiple 
> times
> and we can drop the target_id == ISCSI_MAX_TARGET since it would then rely on 
> the
> state checks (I left out the ISCSI_DBG_TRANS_SESSION because I'm lazy):
> 
>   bool remove_target = false;
> .
> 
> 
I think it's not necessary to add a flag remove_target, here is my changes for 
function __iscsi_unbind_session:

@@ -1966,23 +1977,28 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
-   if (session->target_id == ISCSI_MAX_TARGET) {
+   if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
-   goto unbind_session_exit;
+   ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: 
Session is %s.\n",
+   
iscsi_session_target_state_name[session->target_state]);
+   return;
}
-
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
+   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
 
scsi_remove_target(>dev);
 
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+   spin_unlock_irqrestore(>lock, flags);
+
if (session->ida_used)
ida_free(_sess_ida, target_id);
 
-unbind_session_exit:
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
 }

And the function looks like following after change:

static void __iscsi_unbind_session(struct work_struct *work)
{
struct iscsi_cls_session *session =
container_of(work, struct iscsi_cls_session,
 unbind_work);
struct Scsi_Host *shost = iscsi_session_to_shost(session);
struct iscsi_cls_host *ihost = shost->shost_data;
unsigned long flags;
unsigned int target_id;

ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");

/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>mutex);
ISCSI_DBG_TRANS_SESSION(session, "Skipping target unbinding: 
Session is %s.\n",

iscsi_session_target_state_name[session->target_state]);

Re: Could not logout of all requested sessions reported error (9 - internal error)

['Wenchao Hao' via open-iscsi]

Would the discussion in this issue is helpful for you?
On Thursday, November 3, 2022 at 9:56:43 PM UTC+8 Andinet Gebre wrote:

> 
>
> I am able to discover and login into the Target from the iscsi client and 
> CHAP is also configured to authenticate to/from the ISCSI Initiator client. 
> I am getting the following error when trying logging out from the target to 
> check if the CHAP config is working as expected while log back in,
>
> [root@ltolx2020 ~]# iscsiadm --mode node --target 
> iqn.1992-08.com.redhat:sn.120f265e82be345ecb111d039ea331262:vs.14 --portal 
> 10.85.64.270 --logout Logging out of session [sid: 1, target: 
> iqn.1992-08.com.redhat:sn.120f265e82be345ecb111d039ea331262:vs.14, portal: 
> 10.85.64.270 ,3260] iscsiadm: Could not logout of [sid: 1, target: 
> iqn.1992-08.com.redhat:sn.120f265e82be345ecb111d039ea331262:vs.14, portal: 
> 10.85.64.270,3260]. iscsiadm: initiator reported error (9 - internal error) 
> iscsiadm: Could not logout of all requested sessions
>

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/282ce94a-6a01-4504-b1fe-eb476aa97718n%40googlegroups.com.

[PATCH v6] scsi:iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
for multiple times which should be fixed.

This patch introduce target_state in iscsi_cls_session to make
sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.

But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
Report unbind session event when the target has been removed"). The issue
is iscsid died for any reason after it send unbind session to kernel, once
iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.

Now kernel think iscsi_cls_session has already sent an
ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
would cause userspace unable to logout. Actually the session is in
invalid state(it's target_id is INVALID), iscsid should not sync this
session in it's restart.

So we need to check session's target state during iscsid restart,
if session is in unbound state, do not sync this session and perform
session teardown. It's reasonable because once a session is unbound, we
can not recover it any more(mainly because it's target id is INVALID)

V6:
- Set target state to ALLOCATED in iscsi_add_session
- Rename state BOUND to SCANNED
- Make iscsi_session_target_state_name() more efficient

V5:
- Add ISCSI_SESSION_TARGET_ALLOCATED to indicate the session's
  target has been allocated but not scanned yet. We should
  sync this session and scan this session when iscsid restarted.

V4:
- Move debug print out of spinlock critical section

V3:
- Make target bind state to a state kind rather than a bool.

V2:
- Using target_unbound rather than state to indicate session has been
  unbound

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 57 -
 include/scsi/scsi_transport_iscsi.h |  9 +
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index cd3db9684e52..4bbd1aa4a609 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,21 @@ static const char *iscsi_session_state_name(int state)
return name;
 }
 
+static char *iscsi_session_target_state_names[] = {
+   "UNBOUND",
+   "ALLOCATED",
+   "SCANNED",
+   "UNBINDING",
+};
+
+static const char *iscsi_session_target_state_name(int state)
+{
+   if (state > ISCSI_SESSION_TARGET_MAX)
+   return NULL;
+
+   return iscsi_session_target_state_names[state];
+}
+
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
int err;
@@ -1785,9 +1800,13 @@ static int iscsi_user_scan_session(struct device *dev, 
void *data)
if ((scan_data->channel == SCAN_WILD_CARD ||
 scan_data->channel == 0) &&
(scan_data->id == SCAN_WILD_CARD ||
-scan_data->id == id))
+scan_data->id == id)) {
scsi_scan_target(>dev, 0, id,
 scan_data->lun, scan_data->rescan);
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_SCANNED;
+   spin_unlock_irqrestore(>lock, flags);
+   }
}
 
 user_scan_exit:
@@ -1961,6 +1980,21 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
unsigned long flags;
unsigned int target_id;
 
+   spin_lock_irqsave(>lock, flags);
+   if (session->target_state == ISCSI_SESSION_TARGET_ALLOCATED) {
+   spin_unlock_irqrestore(>lock, flags);
+   if (session->ida_used)
+   ida_free(_sess_ida, session->target_id);
+   ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: 
allocated\n");
+   goto unbind_session_exit;
+   }
+   if (session->target_state != ISCSI_SESSION_TARGET_SCANNED) {
+   spin_unlock_irqrestore(>lock, flags);
+   ISCSI_DBG_TRANS_SESSION(session, "Donot unbind sesison: 
unbinding or unbound\n");
+   return;
+   }
+   spin_unlock_irqrestore(>lock, flags);
+
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
/* Prevent new scans and make sure scanning is not in progress */
@@ -1972,6 +2006,7 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
goto unbind_session_exit;
}
 
+   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
target_id = session->target_id;
session->target_id = ISCSI_MAX_TARGET;
spin_unlock_irqrestore(>lock, flags);
@@ -1983,6 +2018,10 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
ida_free(_sess_ida, target_id);
 
 unbind_session_exit:
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+   spin_unlock_irqrestore(>lock, flags);
+
iscsi_session_event(session, 

[PATCH v3] scsi: iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
for multiple times which should be fixed.

This patch introduce target_state in iscsi_cls_session to make
sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.

But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
Report unbind session event when the target has been removed"). The issue
is iscsid died for any reason after it send unbind session to kernel, once
iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.

Now kernel think iscsi_cls_session has already sent an
ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
would cause userspace unable to logout. Actually the session is in
invalid state(it's target_id is INVALID), iscsid should not sync this
session in it's restart.

So we need to check session's target state during iscsid restart,
if session is in unbound state, do not sync this session and perform
session teardown. It's reasonable because once a session is unbound, we
can not recover it any more(mainly because it's target id is INVALID)

Changes from V2:
- Make target bind state to a state kind rather than a bool.

Changes from V1:
- Using target_unbound rather than state to indicate session has been
  unbound

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 49 +
 include/scsi/scsi_transport_iscsi.h |  7 +
 2 files changed, 56 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 5d21f07456c6..43d48ddfea07 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1676,6 +1676,29 @@ static const char *iscsi_session_state_name(int state)
return name;
 }
 
+static struct {
+   int value;
+   char *name;
+} iscsi_session_target_state_names[] = {
+   { ISCSI_SESSION_TARGET_UNBOUND, "UNBOUND" },
+   { ISCSI_SESSION_TARGET_BOUND,   "BOUND" },
+   { ISCSI_SESSION_TARGET_UNBINDING,   "UNBINDING" },
+};
+
+static const char *iscsi_session_target_state_name(int state)
+{
+   int i;
+   char *name = NULL;
+
+   for (i = 0; i < ARRAY_SIZE(iscsi_session_target_state_names); i++) {
+   if (iscsi_session_target_state_names[i].value == state) {
+   name = iscsi_session_target_state_names[i].name;
+   break;
+   }
+   }
+   return name;
+}
+
 int iscsi_session_chkready(struct iscsi_cls_session *session)
 {
int err;
@@ -1899,6 +1922,7 @@ static void __iscsi_unblock_session(struct work_struct 
*work)
cancel_delayed_work_sync(>recovery_work);
spin_lock_irqsave(>lock, flags);
session->state = ISCSI_SESSION_LOGGED_IN;
+   session->target_state = ISCSI_SESSION_TARGET_BOUND;
spin_unlock_irqrestore(>lock, flags);
/* start IO */
scsi_target_unblock(>dev, SDEV_RUNNING);
@@ -1961,6 +1985,15 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
unsigned long flags;
unsigned int target_id;
 
+   spin_lock_irqsave(>lock, flags);
+   if (session->target_state != ISCSI_SESSION_TARGET_BOUND) {
+   ISCSI_DBG_TRANS_SESSION(session, "Abort unbind sesison\n");
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+   session->target_state = ISCSI_SESSION_TARGET_UNBINDING;
+   spin_unlock_irqrestore(>lock, flags);
+
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
/* Prevent new scans and make sure scanning is not in progress */
@@ -1984,6 +2017,9 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
 
 unbind_session_exit:
iscsi_session_event(session, ISCSI_KEVENT_UNBIND_SESSION);
+   spin_lock_irqsave(>lock, flags);
+   session->target_state = ISCSI_SESSION_TARGET_UNBOUND;
+   spin_unlock_irqrestore(>lock, flags);
ISCSI_DBG_TRANS_SESSION(session, "Completed target removal\n");
 }
 
@@ -4324,6 +4360,16 @@ iscsi_session_attr(def_taskmgmt_tmo, 
ISCSI_PARAM_DEF_TASKMGMT_TMO, 0);
 iscsi_session_attr(discovery_parent_idx, ISCSI_PARAM_DISCOVERY_PARENT_IDX, 0);
 iscsi_session_attr(discovery_parent_type, ISCSI_PARAM_DISCOVERY_PARENT_TYPE, 
0);
 
+static ssize_t
+show_priv_session_target_state(struct device *dev, struct device_attribute 
*attr,
+   char *buf)
+{
+   struct iscsi_cls_session *session = iscsi_dev_to_session(dev->parent);
+   return sysfs_emit(buf, "%s\n",
+   iscsi_session_target_state_name(session->target_state));
+}
+static ISCSI_CLASS_ATTR(priv_sess, target_state, S_IRUGO,
+   show_priv_session_target_state, NULL);
 static ssize_t
 show_priv_session_state(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -4426,6 +4472,7 @@ static struct attribute *iscsi_session_attrs[] = {

Re: [PATCH v2] scsi: iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

On 2022/4/21 0:28, Mike Christie wrote:
> On 4/17/22 7:06 PM, Wenchao Hao wrote:
>> I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
>> for multiple times which should be fixed.
>>
>> This patch introduce target_unbound in iscsi_cls_session to make
>> sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.
>>
>> But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
>> Report unbind session event when the target has been removed"). The issue
>> is iscsid died for any reason after it send unbind session to kernel, once
>> iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.
>>
>> Now kernel think iscsi_cls_session has already sent an
>> ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
>> would cause userspace unable to logout. Actually the session is in
>> invalid state(it's target_id is INVALID), iscsid should not sync this
>> session in it's restart.
>>
>> So we need to check session's target unbound state during iscsid restart,
>> if session is in unbound state, do not sync this session and perform
>> session teardown. It's reasonable because once a session is unbound, we
>> can not recover it any more(mainly because it's target id is INVALID)
>>
>> Changes from V1:
>> - Using target_unbound rather than state to indicate session has been
>>   unbound
>>
>> Signed-off-by: Wenchao Hao 
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 21 +
>>  include/scsi/scsi_transport_iscsi.h |  1 +
>>  2 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 2c0dd64159b0..43ba31e595b4 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -1958,6 +1958,14 @@ static void __iscsi_unbind_session(struct work_struct 
>> *work)
>>  
>>  ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>  
>> +spin_lock_irqsave(>lock, flags);
>> +if (session->target_unbound) {
>> +spin_unlock_irqrestore(>lock, flags);
>> +return;
>> +}
>> +session->target_unbound = 1;
> 
> Shoot, sorry, I think I gave you a bad review comment when I said we
> could do a bool or state kind or variable.
> 
> If we set unbound here and iscsid was restarting at this point then
> iscsid really only knows the target removal process is starting up. It
> doesn't know that the target is not yet removed. We could be doing sync
> caches and/or still tearing down scsi_devices/LUNs.
> 
> For the comments I gave you on the userspace PR parts, would it be
> easier if this was a state type of value? Above you would set it to
> REMOVING. When scsi_remove_target is done then we can set it to
> REMOVED. That combined with the session and conn states we can detect
> how far we got in the session removal process if iscsid dies in the
> middle of it.
> 
> What do you think?
> 

I thought about setting this bool to true after ISCSI_KEVENT_UNBIND_SESSION has
been sent in __iscsi_unbind_session(), it's not a good way too, the sync session
and unbind target would run concurrency.

If we need make sure iscsid call in session_conn_shutdown() after kernel's
scsi_remove_target() has finished, we must make it a state type.

We need think about how to set the initial value of this state. Since we only
cares about the removing state, the easiest way is setting it to INITED when 
allocing
session. When iscsid restart and found it's INITED, still sync this session.

Based on your REMOVING and REMOVED state, state is set to REMOVING at beginning
of __iscsi_unbind_session() and set to REMOVED after scsi_remove_target() done.
When iscsid restart and found this state is REMOVING, it do nothing, just 
waiting
for ISCSI_KEVENT_UNBIND_SESSION event. If the state is REMOVED, it should start
shutting down(both check session and conn state as you mentioned in my PR).

> 
>> +spin_unlock_irqrestore(>lock, flags);
>> +
>>  /* Prevent new scans and make sure scanning is not in progress */
>>  mutex_lock(>mutex);
>>  spin_lock_irqsave(>lock, flags)
> 
> ...
> 
>> diff --git a/include/scsi/scsi_transport_iscsi.h 
>> b/include/scsi/scsi_transport_iscsi.h
>> index 9acb8422f680..877632c25e56 100644
>> --- a/include/scsi/scsi_transport_iscsi.h
>> +++ b/include/scsi/scsi_transport_iscsi.h
>> @@ -256,6 +256,7 @@ struct iscsi_cls_session {
>>  struct workqueue_struct *workq;
>>  
>>  unsigned int target_id;
>> +int target_unbound;   /* make sure unbind session only once */
> 
> 
> We don't need the comment since the code using this is so simple
> and the name of the variable tells us what it's for.
> 
> 
>>  bool ida_used;
>>  
>>  /*
> 
> .

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web 

[PATCH v2] scsi: iscsi: Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

I found an issue that kernel would send ISCSI_KEVENT_UNBIND_SESSION
for multiple times which should be fixed.

This patch introduce target_unbound in iscsi_cls_session to make
sure session would send only one ISCSI_KEVENT_UNBIND_SESSION.

But this would break issue fixed in commit 13e60d3ba287 ("scsi: iscsi:
Report unbind session event when the target has been removed"). The issue
is iscsid died for any reason after it send unbind session to kernel, once
iscsid restart again, it loss kernel's ISCSI_KEVENT_UNBIND_SESSION event.

Now kernel think iscsi_cls_session has already sent an
ISCSI_KEVENT_UNBIND_SESSION event and would not send it any more. Which
would cause userspace unable to logout. Actually the session is in
invalid state(it's target_id is INVALID), iscsid should not sync this
session in it's restart.

So we need to check session's target unbound state during iscsid restart,
if session is in unbound state, do not sync this session and perform
session teardown. It's reasonable because once a session is unbound, we
can not recover it any more(mainly because it's target id is INVALID)

Changes from V1:
- Using target_unbound rather than state to indicate session has been
  unbound

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 21 +
 include/scsi/scsi_transport_iscsi.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 2c0dd64159b0..43ba31e595b4 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1958,6 +1958,14 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
 
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
+   spin_lock_irqsave(>lock, flags);
+   if (session->target_unbound) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+   session->target_unbound = 1;
+   spin_unlock_irqrestore(>lock, flags);
+
/* Prevent new scans and make sure scanning is not in progress */
mutex_lock(>mutex);
spin_lock_irqsave(>lock, flags);
@@ -2058,6 +2066,7 @@ int iscsi_add_session(struct iscsi_cls_session *session, 
unsigned int target_id)
session->target_id = target_id;
 
dev_set_name(>dev, "session%u", session->sid);
+   session->target_unbound = 0;
err = device_add(>dev);
if (err) {
iscsi_cls_session_printk(KERN_ERR, session,
@@ -4319,6 +4328,15 @@ iscsi_session_attr(def_taskmgmt_tmo, 
ISCSI_PARAM_DEF_TASKMGMT_TMO, 0);
 iscsi_session_attr(discovery_parent_idx, ISCSI_PARAM_DISCOVERY_PARENT_IDX, 0);
 iscsi_session_attr(discovery_parent_type, ISCSI_PARAM_DISCOVERY_PARENT_TYPE, 
0);
 
+static ssize_t
+show_priv_session_target_unbound(struct device *dev, struct device_attribute 
*attr,
+   char *buf)
+{
+   struct iscsi_cls_session *session = iscsi_dev_to_session(dev->parent);
+   return sysfs_emit(buf, "%d\n", session->target_unbound);
+}
+static ISCSI_CLASS_ATTR(priv_sess, target_unbound, S_IRUGO,
+   show_priv_session_target_unbound, NULL);
 static ssize_t
 show_priv_session_state(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -4422,6 +4440,7 @@ static struct attribute *iscsi_session_attrs[] = {
_attr_priv_sess_recovery_tmo.attr,
_attr_priv_sess_state.attr,
_attr_priv_sess_creator.attr,
+   _attr_priv_sess_target_unbound.attr,
_attr_sess_chap_out_idx.attr,
_attr_sess_chap_in_idx.attr,
_attr_priv_sess_target_id.attr,
@@ -4534,6 +4553,8 @@ static umode_t iscsi_session_attr_is_visible(struct 
kobject *kobj,
return S_IRUGO | S_IWUSR;
else if (attr == _attr_priv_sess_state.attr)
return S_IRUGO;
+   else if (attr == _attr_priv_sess_target_unbound.attr)
+   return S_IRUGO;
else if (attr == _attr_priv_sess_creator.attr)
return S_IRUGO;
else if (attr == _attr_priv_sess_target_id.attr)
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 9acb8422f680..877632c25e56 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -256,6 +256,7 @@ struct iscsi_cls_session {
struct workqueue_struct *workq;
 
unsigned int target_id;
+   int target_unbound;   /* make sure unbind session only once */
bool ida_used;
 
/*
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220418000627.474784-1-haowenchao%40huawei.com.

Re: [PATCH 2/2] iscsi: set session to FREE state after unbind session in remove session

['Wenchao Hao' via open-iscsi]

On 2022/4/14 23:30, Mike Christie wrote:
> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>> __iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
>> would overwrite the ISCSI_SESSION_FREE state.
>>
>> Signed-off-by: Wenchao Hao 
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 26 --
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 97a9fee02efa..d8dd9279cea8 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session 
>> *session)
>>  if (!cancel_work_sync(>block_work))
>>  cancel_delayed_work_sync(>recovery_work);
>>  cancel_work_sync(>unblock_work);
>> +
>> +scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
>> +/*
>> + * qla4xxx can perform it's own scans when it runs in kernel only
>> + * mode. Make sure to flush those scans.
>> + */
>> +flush_work(>scan_work);
>> +
>> +/*
>> + * flush running unbind operations
>> + * if unbind work did not queued, call __iscsi_unbind_session
>> + * directly to perform target remove
> 
> We probably don't need the flush_work test because we are going to
> normally call __iscsi_unbind_session.
> 

I think we still need calling flush_work here. The introduce of flush_work
is to make sure sysfs objects are removed in an correct order. There is a
very low probability that __iscsi_unbind_session() triggered by queue_work()
has not been finished, and iscsi_remove_session() is called. So we need
flush_work() to make sure __iscsi_unbind_session() has done if it has been
activated by queue_work().

> If the unbind work had already run, which is the normal case, then
> flush_work returns false and we end up calling __iscsi_unbind_session
> like before. That function then checks if the target is really unbound.
> So the extra check doesn't normally buy us anything with your patches
> because in patch 1 you fixed it so __iscsi_unbind_session doesn't send
> the extra event.
> 
> 
>> + */
>> +if (!flush_work(>unbind_work))
>> +__iscsi_unbind_session(>unbind_work);
>> +
>>  /*
>>   * If we are blocked let commands flow again. The lld or iscsi
>>   * layer should set up the queuecommand to fail commands.
>> @@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session 
>> *session)
>>  session->state = ISCSI_SESSION_FREE;
>>  spin_unlock_irqrestore(>lock, flags);
>>  
>> -scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
>> -/*
>> - * qla4xxx can perform it's own scans when it runs in kernel only
>> - * mode. Make sure to flush those scans.
>> - */
>> -flush_work(>scan_work);
>> -/* flush running unbind operations */
>> -flush_work(>unbind_work);
>> -__iscsi_unbind_session(>unbind_work);
>> -
>>  /* hw iscsi may not have removed all connections from session */
>>  err = device_for_each_child(>dev, NULL,
>>  iscsi_iter_destroy_conn_fn);
> 
> .

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f587206a-4479-1748-9211-086d79249b95%40huawei.com.

Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event

['Wenchao Hao' via open-iscsi]

On 2022/4/14 23:22, Mike Christie wrote:
> On 4/13/22 8:49 PM, Wenchao Hao wrote:
>> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
>> If session is in UNBOUND state, do not perform unbind operations anymore,
>> else unbind session and set session to UNBOUND state.
>>
> 
> I don't think we want this to be a state because you can have a session
> with no target or it could be partially deleted and it could be in the
> logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of
> the target/device removal operation, and we lose the session then we could
> go through recovery and the state will go from failed to logged in, and
> your new unbound state will have been overwritten.
> 
> I think it might be better to have a new sysfs file, target_state, for
> this where you could have values like scanning, bound, unbinding, and
> unbound, or just a sysfs file, target_bound, that is bool.
> 

Thanks for your review, I would modify and send another patch.

>> Reference:https://github.com/open-iscsi/open-iscsi/issues/338
>>
> 
> You should add a description of the problem in the commit, because that
> link might be gone one day.
> 
> 
>> Signed-off-by: Wenchao Hao 
>> ---
>>  drivers/scsi/scsi_transport_iscsi.c | 19 +--
>>  include/scsi/scsi_transport_iscsi.h |  1 +
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
>> b/drivers/scsi/scsi_transport_iscsi.c
>> index 27951ea05dd4..97a9fee02efa 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -1656,6 +1656,7 @@ static struct {
>>  { ISCSI_SESSION_LOGGED_IN,  "LOGGED_IN" },
>>  { ISCSI_SESSION_FAILED, "FAILED" },
>>  { ISCSI_SESSION_FREE,   "FREE" },
>> +{ ISCSI_SESSION_UNBOUND,"UNBOUND" },
>>  };
>>  
>>  static const char *iscsi_session_state_name(int state)
>> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session 
>> *session)
>>  case ISCSI_SESSION_FREE:
>>  err = DID_TRANSPORT_FAILFAST << 16;
>>  break;
>> +case ISCSI_SESSION_UNBOUND:
>> +err = DID_NO_CONNECT << 16;
>> +break;
>>  default:
>>  err = DID_NO_CONNECT << 16;
>>  break;
>> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
>>  
>>  spin_lock_irqsave(>lock, flags);
>>  while (session->state != ISCSI_SESSION_LOGGED_IN) {
>> -if (session->state == ISCSI_SESSION_FREE) {
>> +if ((session->state == ISCSI_SESSION_FREE) ||
>> +(session->state == ISCSI_SESSION_UNBOUND)) {
>>  ret = FAST_IO_FAIL;
>>  break;
>>  }
>> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct 
>> work_struct *work)
>>  break;
>>  case ISCSI_SESSION_LOGGED_IN:
>>  case ISCSI_SESSION_FREE:
>> +case ISCSI_SESSION_UNBOUND:
>>  /* we raced with the unblock's flush */
>>  spin_unlock_irqrestore(>lock, flags);
>>  return;
>> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct 
>> *work)
>>  unsigned long flags;
>>  unsigned int target_id;
>>  
>> +spin_lock_irqsave(>lock, flags);
>> +if (session->state == ISCSI_SESSION_UNBOUND) {
>> +spin_unlock_irqrestore(>lock, flags);
>> +return;
>> +}
>> +session->state = ISCSI_SESSION_UNBOUND;
>> +spin_unlock_irqrestore(>lock, flags);
>> +
>>  ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>>  
>>  /* Prevent new scans and make sure scanning is not in progress */
>> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev, 
>> \
>>  struct iscsi_cls_session *session = \
>>  iscsi_dev_to_session(dev->parent);  \
>>  if ((session->state == ISCSI_SESSION_FREE) ||   \
>> -(session->state == ISCSI_SESSION_FAILED))   \
>> +(session->state == ISCSI_SESSION_FAILED) || \
>> +(session->state == ISCSI_SESSION_UNBOUND))  \
>>  return -EBUSY;  \
>>  if (strncmp(buf, "off", 3) == 0) {  \
>>  session->field = -1;\
>> diff --git a/include/scsi/scsi_transport_iscsi.h 
>> b/include/scsi/scsi_transport_iscsi.h
>> index 38e4a67f5922..80149643cbcd 100644
>> --- a/include/scsi/scsi_transport_iscsi.h
>> +++ b/include/scsi/scsi_transport_iscsi.h
>> @@ -232,6 +232,7 @@ enum {
>>  ISCSI_SESSION_LOGGED_IN,
>>  ISCSI_SESSION_FAILED,
>>  ISCSI_SESSION_FREE,
>> +ISCSI_SESSION_UNBOUND,
>>  };
>>  
>>  #define ISCSI_MAX_TARGET -1
> 
> .

-- 
You received this message because you are subscribed to the Google Groups 

[PATCH 2/2] iscsi: set session to FREE state after unbind session in remove session

['Wenchao Hao' via open-iscsi]

__iscsi_unbind_session() set session state to ISCSI_SESSION_UNBOUND, which
would overwrite the ISCSI_SESSION_FREE state.

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 97a9fee02efa..d8dd9279cea8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2173,6 +2173,22 @@ void iscsi_remove_session(struct iscsi_cls_session 
*session)
if (!cancel_work_sync(>block_work))
cancel_delayed_work_sync(>recovery_work);
cancel_work_sync(>unblock_work);
+
+   scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
+   /*
+* qla4xxx can perform it's own scans when it runs in kernel only
+* mode. Make sure to flush those scans.
+*/
+   flush_work(>scan_work);
+
+   /*
+* flush running unbind operations
+* if unbind work did not queued, call __iscsi_unbind_session
+* directly to perform target remove
+*/
+   if (!flush_work(>unbind_work))
+   __iscsi_unbind_session(>unbind_work);
+
/*
 * If we are blocked let commands flow again. The lld or iscsi
 * layer should set up the queuecommand to fail commands.
@@ -2183,16 +2199,6 @@ void iscsi_remove_session(struct iscsi_cls_session 
*session)
session->state = ISCSI_SESSION_FREE;
spin_unlock_irqrestore(>lock, flags);
 
-   scsi_target_unblock(>dev, SDEV_TRANSPORT_OFFLINE);
-   /*
-* qla4xxx can perform it's own scans when it runs in kernel only
-* mode. Make sure to flush those scans.
-*/
-   flush_work(>scan_work);
-   /* flush running unbind operations */
-   flush_work(>unbind_work);
-   __iscsi_unbind_session(>unbind_work);
-
/* hw iscsi may not have removed all connections from session */
err = device_for_each_child(>dev, NULL,
iscsi_iter_destroy_conn_fn);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220414014947.4168447-3-haowenchao%40huawei.com.

[PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind event

['Wenchao Hao' via open-iscsi]

Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
If session is in UNBOUND state, do not perform unbind operations anymore,
else unbind session and set session to UNBOUND state.

Reference:https://github.com/open-iscsi/open-iscsi/issues/338

Signed-off-by: Wenchao Hao 
---
 drivers/scsi/scsi_transport_iscsi.c | 19 +--
 include/scsi/scsi_transport_iscsi.h |  1 +
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 27951ea05dd4..97a9fee02efa 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1656,6 +1656,7 @@ static struct {
{ ISCSI_SESSION_LOGGED_IN,  "LOGGED_IN" },
{ ISCSI_SESSION_FAILED, "FAILED" },
{ ISCSI_SESSION_FREE,   "FREE" },
+   { ISCSI_SESSION_UNBOUND,"UNBOUND" },
 };
 
 static const char *iscsi_session_state_name(int state)
@@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session 
*session)
case ISCSI_SESSION_FREE:
err = DID_TRANSPORT_FAILFAST << 16;
break;
+   case ISCSI_SESSION_UNBOUND:
+   err = DID_NO_CONNECT << 16;
+   break;
default:
err = DID_NO_CONNECT << 16;
break;
@@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
 
spin_lock_irqsave(>lock, flags);
while (session->state != ISCSI_SESSION_LOGGED_IN) {
-   if (session->state == ISCSI_SESSION_FREE) {
+   if ((session->state == ISCSI_SESSION_FREE) ||
+   (session->state == ISCSI_SESSION_UNBOUND)) {
ret = FAST_IO_FAIL;
break;
}
@@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct 
*work)
break;
case ISCSI_SESSION_LOGGED_IN:
case ISCSI_SESSION_FREE:
+   case ISCSI_SESSION_UNBOUND:
/* we raced with the unblock's flush */
spin_unlock_irqrestore(>lock, flags);
return;
@@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct 
*work)
unsigned long flags;
unsigned int target_id;
 
+   spin_lock_irqsave(>lock, flags);
+   if (session->state == ISCSI_SESSION_UNBOUND) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+   session->state = ISCSI_SESSION_UNBOUND;
+   spin_unlock_irqrestore(>lock, flags);
+
ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
 
/* Prevent new scans and make sure scanning is not in progress */
@@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev,
\
struct iscsi_cls_session *session = \
iscsi_dev_to_session(dev->parent);  \
if ((session->state == ISCSI_SESSION_FREE) ||   \
-   (session->state == ISCSI_SESSION_FAILED))   \
+   (session->state == ISCSI_SESSION_FAILED) || \
+   (session->state == ISCSI_SESSION_UNBOUND))  \
return -EBUSY;  \
if (strncmp(buf, "off", 3) == 0) {  \
session->field = -1;\
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 38e4a67f5922..80149643cbcd 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -232,6 +232,7 @@ enum {
ISCSI_SESSION_LOGGED_IN,
ISCSI_SESSION_FAILED,
ISCSI_SESSION_FREE,
+   ISCSI_SESSION_UNBOUND,
 };
 
 #define ISCSI_MAX_TARGET -1
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220414014947.4168447-2-haowenchao%40huawei.com.

[PATCH 0/2] Fix multiple iscsi session unbind event sent to userspace

['Wenchao Hao' via open-iscsi]

kernel would send ISCSI_KEVENT_UNBIND_SESSION twice to userspace, for
open-iscsi, this would trigger iscsi_stop twice. We should fix this issue.

Here introduced a new session state ISCSI_SESSION_UNBOUND to address it.
Once session state is ISCSI_KEVENT_UNBIND_SESSION, it means
__iscsi_unbind_session() has been called for this session and do not need
to execute any more.

Reference:https://github.com/open-iscsi/open-iscsi/issues/338

Wenchao Hao (2):
  scsi: iscsi: introduce session UNBOUND state to avoid multiple unbind
event
  iscsi: set session to FREE state after unbind session in remove
session

 drivers/scsi/scsi_transport_iscsi.c | 45 +
 include/scsi/scsi_transport_iscsi.h |  1 +
 2 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220414014947.4168447-1-haowenchao%40huawei.com.

[PATCH] scsi:libiscsi: remove unnecessary memset in iscsi_conn_setup

['Wenchao Hao' via open-iscsi]

iscsi_cls_conn is alloced by kzalloc(), the whole iscsi_cls_conn is
zero filled already including the dd_data. So it is unnecessary to
call memset again.

Signed-off-by: Wenchao Hao 
Reviewed-by: Wu Bo 
Reviewed-by: Lee Duncan 
---
 drivers/scsi/libiscsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d09926e6c8a8..cf4211c6500d 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3045,7 +3045,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
if (!cls_conn)
return NULL;
conn = cls_conn->dd_data;
-   memset(conn, 0, sizeof(*conn) + dd_size);
 
conn->dd_data = cls_conn->dd_data + sizeof(*conn);
conn->session = session;
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220317150116.194140-1-haowenchao%40huawei.com.

Re: [PATCH] scsi:libiscsi: remove unnecessary memset in iscsi_conn_setup

['Wenchao Hao' via open-iscsi]

cc open-iscsi@googlegroups.com linux-s...@vger.kernel.org

On 2022/3/17 6:09, Wenchao Hao wrote:

iscsi_cls_conn is alloced by kzalloc(), the whole iscsi_cls_conn is
zero filled already including the dd_data. So it is unnecessary to
call memset again.

Signed-off-by: Wenchao Hao 
Reviewed-by: Wu Bo 
---
  drivers/scsi/libiscsi.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index d09926e6c8a8..cf4211c6500d 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3045,7 +3045,6 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
if (!cls_conn)
return NULL;
conn = cls_conn->dd_data;
-   memset(conn, 0, sizeof(*conn) + dd_size);
  
  	conn->dd_data = cls_conn->dd_data + sizeof(*conn);

conn->session = session;



--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/44860f67-e626-411e-5ee6-9055ea2d5723%40huawei.com.

Re: [PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly

['Wenchao Hao' via open-iscsi]

On 2022/3/10 9:57, Wenchao Hao wrote:

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

The origin implement do device setup in iscsi_create_conn() which
bind the alloc/init and add in one function; do device teardown in
iscsi_destroy_conn() which bind remove and free in one function.

This implement makes it impossible to initialize resources of device
before add it to sysfs during setup.

So this patchset splict both the setup and teradown of iscsi_cls_conn to
2 steps.

For setup flow, we should call iscsi_alloc_conn() and initialize some
resources, then call iscsi_add_conn().

For teradown flow, we should call iscsi_remove_conn() to remove device
and free resources which related to iscsi_cls_conn, then call
iscsi_put_conn() to free iscsi_cls_conn.



Friendly ping...


V2 -> V3:
   * Fix some bugs and optimization the code implement.

V1 -> V2:
   * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
   * change the teardown flow of iscsi_cls_conn

Wenchao Hao (3):
   scsi: iscsi: Add helper functions to manage iscsi_cls_conn
   scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
   scsi:libiscsi: teradown iscsi_cls_conn gracefully

  drivers/scsi/libiscsi.c | 23 +---
  drivers/scsi/scsi_transport_iscsi.c | 90 +++--
  include/scsi/scsi_transport_iscsi.h |  5 +-
  3 files changed, 66 insertions(+), 52 deletions(-)



--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/069ed5a6-0cba-195f-bab3-391514c55a97%40huawei.com.

[PATCH v3 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized

['Wenchao Hao' via open-iscsi]

iscsi_create_conn() would expose iscsi_cls_conn to sysfs, while the
initialization of iscsi_conn's dd_data is not ready now. When userspace
try to access an attribute such as connect's address, it might cause
a NULL pointer dereference.

So we should add iscsi_cls_conn to sysfs until it has been initialized.
And remove iscsi_create_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/libiscsi.c | 13 -
 drivers/scsi/scsi_transport_iscsi.c | 76 -
 include/scsi/scsi_transport_iscsi.h |  2 -
 3 files changed, 11 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..38e68b449c08 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3040,8 +3040,9 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
struct iscsi_conn *conn;
struct iscsi_cls_conn *cls_conn;
char *data;
+   int err;
 
-   cls_conn = iscsi_create_conn(cls_session, sizeof(*conn) + dd_size,
+   cls_conn = iscsi_alloc_conn(cls_session, sizeof(*conn) + dd_size,
 conn_idx);
if (!cls_conn)
return NULL;
@@ -3078,13 +3079,21 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
goto login_task_data_alloc_fail;
conn->login_task->data = conn->data = data;
 
+   err = iscsi_add_conn(cls_conn);
+   if (err)
+   goto login_task_add_dev_fail;
+
return cls_conn;
 
+login_task_add_dev_fail:
+   free_pages((unsigned long) conn->data,
+  get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+
 login_task_data_alloc_fail:
kfifo_in(>cmdpool.queue, (void*)>login_task,
sizeof(void*));
 login_task_alloc_fail:
-   iscsi_destroy_conn(cls_conn);
+   iscsi_put_conn(cls_conn);
return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_setup);
diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 65117ed5626e..fc33000a6af9 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2437,82 +2437,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_remove_conn);
 
-/**
- * iscsi_create_conn - create iscsi class connection
- * @session: iscsi cls session
- * @dd_size: private driver data size
- * @cid: connection id
- *
- * This can be called from a LLD or iscsi_transport. The connection
- * is child of the session so cid must be unique for all connections
- * on the session.
- *
- * Since we do not support MCS, cid will normally be zero. In some cases
- * for software iscsi we could be trying to preallocate a connection struct
- * in which case there could be two connection structs and cid would be
- * non-zero.
- */
-struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
-{
-   struct iscsi_transport *transport = session->transport;
-   struct iscsi_cls_conn *conn;
-   unsigned long flags;
-   int err;
-
-   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
-   if (!conn)
-   return NULL;
-   if (dd_size)
-   conn->dd_data = [1];
-
-   mutex_init(>ep_mutex);
-   INIT_LIST_HEAD(>conn_list);
-   INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
-   conn->transport = transport;
-   conn->cid = cid;
-   conn->state = ISCSI_CONN_DOWN;
-
-   /* this is released in the dev's release function */
-   if (!get_device(>dev))
-   goto free_conn;
-
-   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
-   conn->dev.parent = >dev;
-   conn->dev.release = iscsi_conn_release;
-   err = device_register(>dev);
-   if (err) {
-   iscsi_cls_session_printk(KERN_ERR, session, "could not "
-"register connection's dev\n");
-   goto release_parent_ref;
-   }
-   err = transport_register_device(>dev);
-   if (err) {
-   iscsi_cls_session_printk(KERN_ERR, session, "could not "
-"register transport's dev\n");
-   goto release_conn_ref;
-   }
-
-   spin_lock_irqsave(, flags);
-   list_add(>conn_list, );
-   spin_unlock_irqrestore(, flags);
-
-   ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
-   return conn;
-
-release_conn_ref:
-   device_unregister(>dev);
-   put_device(>dev);
-   return NULL;
-release_parent_ref:
-   put_device(>dev);
-free_conn:
-   kfree(conn);
-   return NULL;
-}
-
-EXPORT_SYMBOL_GPL(iscsi_create_conn);
-
 /**
  * iscsi_destroy_conn - destroy iscsi class connection
  * @conn: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 

[PATCH v3 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs correctly

['Wenchao Hao' via open-iscsi]

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

The origin implement do device setup in iscsi_create_conn() which
bind the alloc/init and add in one function; do device teardown in
iscsi_destroy_conn() which bind remove and free in one function.

This implement makes it impossible to initialize resources of device
before add it to sysfs during setup.

So this patchset splict both the setup and teradown of iscsi_cls_conn to
2 steps.

For setup flow, we should call iscsi_alloc_conn() and initialize some
resources, then call iscsi_add_conn().

For teradown flow, we should call iscsi_remove_conn() to remove device
and free resources which related to iscsi_cls_conn, then call
iscsi_put_conn() to free iscsi_cls_conn.

V2 -> V3:
  * Fix some bugs and optimization the code implement.

V1 -> V2:
  * add two more iscsi_free_conn() and iscsi_remove_conn() than V1
  * change the teardown flow of iscsi_cls_conn

Wenchao Hao (3):
  scsi: iscsi: Add helper functions to manage iscsi_cls_conn
  scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
  scsi:libiscsi: teradown iscsi_cls_conn gracefully

 drivers/scsi/libiscsi.c | 23 +---
 drivers/scsi/scsi_transport_iscsi.c | 90 +++--
 include/scsi/scsi_transport_iscsi.h |  5 +-
 3 files changed, 66 insertions(+), 52 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220310015759.3296841-1-haowenchao%40huawei.com.

[PATCH v3 3/3] scsi:libiscsi: teradown iscsi_cls_conn gracefully

['Wenchao Hao' via open-iscsi]

commit 1b8d0300a3e9 ("scsi: libiscsi: Fix UAF in
iscsi_conn_get_param()/iscsi_conn_teardown()") fixed an UAF in
iscsi_conn_get_param() and introduced 2 tmp_xxx varibles, the
implement looks ugly.

We can fix this UAF with the help of device_del() gracefully.
Call iscsi_remove_conn() at the beginning of iscsi_conn_teardown would
make userspace unable to see iscsi_cls_conn any more, then we can free
memory safely.
And remove iscsi_destroy_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/libiscsi.c | 10 +-
 drivers/scsi/scsi_transport_iscsi.c | 27 +--
 include/scsi/scsi_transport_iscsi.h |  1 -
 3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 38e68b449c08..da583a39b307 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3109,8 +3109,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_session *session = conn->session;
-   char *tmp_persistent_address = conn->persistent_address;
-   char *tmp_local_ipaddr = conn->local_ipaddr;
+
+   iscsi_remove_conn(cls_conn);
 
del_timer_sync(>transport_timer);
 
@@ -3132,6 +3132,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
spin_lock_bh(>frwd_lock);
free_pages((unsigned long) conn->data,
   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+   kfree(conn->persistent_address);
+   kfree(conn->local_ipaddr);
/* regular RX path uses back_lock */
spin_lock_bh(>back_lock);
kfifo_in(>cmdpool.queue, (void*)>login_task,
@@ -3142,9 +3144,7 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
spin_unlock_bh(>frwd_lock);
mutex_unlock(>eh_mutex);
 
-   iscsi_destroy_conn(cls_conn);
-   kfree(tmp_persistent_address);
-   kfree(tmp_local_ipaddr);
+   iscsi_put_conn(cls_conn);
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_teardown);
 
diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index fc33000a6af9..d0f996d14782 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2165,7 +2165,11 @@ static int iscsi_iter_destroy_conn_fn(struct device 
*dev, void *data)
 {
if (!iscsi_is_conn_dev(dev))
return 0;
-   return iscsi_destroy_conn(iscsi_dev_to_conn(dev));
+
+   iscsi_remove_conn(iscsi_dev_to_conn(dev));
+   iscsi_put_conn(iscsi_dev_to_conn(dev));
+
+   return 0;
 }
 
 void iscsi_remove_session(struct iscsi_cls_session *session)
@@ -2437,27 +2441,6 @@ void iscsi_remove_conn(struct iscsi_cls_conn *conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_remove_conn);
 
-/**
- * iscsi_destroy_conn - destroy iscsi class connection
- * @conn: iscsi cls session
- *
- * This can be called from a LLD or iscsi_transport.
- */
-int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
-{
-   unsigned long flags;
-
-   spin_lock_irqsave(, flags);
-   list_del(>conn_list);
-   spin_unlock_irqrestore(, flags);
-
-   transport_unregister_device(>dev);
-   ISCSI_DBG_TRANS_CONN(conn, "Completing conn destruction\n");
-   device_unregister(>dev);
-   return 0;
-}
-EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
-
 void iscsi_put_conn(struct iscsi_cls_conn *conn)
 {
put_device(>dev);
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 4af6768e8195..aedbc4488149 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -447,7 +447,6 @@ extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_get_conn(struct iscsi_cls_conn *conn);
-extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
 extern void iscsi_unblock_session(struct iscsi_cls_session *session);
 extern void iscsi_block_session(struct iscsi_cls_session *session);
 extern int iscsi_scan_finished(struct Scsi_Host *shost, unsigned long time);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220310015759.3296841-4-haowenchao%40huawei.com.

[PATCH v3 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn

['Wenchao Hao' via open-iscsi]

iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn
iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs.
iscsi_remove_conn(): remove iscsi_cls_conn from sysfs

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/scsi_transport_iscsi.c | 97 +
 include/scsi/scsi_transport_iscsi.h |  4 ++
 2 files changed, 101 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..65117ed5626e 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2340,6 +2340,103 @@ void iscsi_free_session(struct iscsi_cls_session 
*session)
 }
 EXPORT_SYMBOL_GPL(iscsi_free_session);
 
+/**
+ * iscsi_alloc_conn - alloc iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ */
+struct iscsi_cls_conn *
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+   struct iscsi_transport *transport = session->transport;
+   struct iscsi_cls_conn *conn;
+
+   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
+   if (!conn)
+   return NULL;
+   if (dd_size)
+   conn->dd_data = [1];
+
+   mutex_init(>ep_mutex);
+   INIT_LIST_HEAD(>conn_list);
+   INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
+   conn->transport = transport;
+   conn->cid = cid;
+   conn->state = ISCSI_CONN_DOWN;
+
+   /* this is released in the dev's release function */
+   if (!get_device(>dev))
+   goto free_conn;
+
+   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
+   device_initialize(>dev);
+   conn->dev.parent = >dev;
+   conn->dev.release = iscsi_conn_release;
+
+   return conn;
+
+free_conn:
+   kfree(conn);
+   return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+   int err;
+   unsigned long flags;
+   struct iscsi_cls_session *session = 
iscsi_dev_to_session(conn->dev.parent);
+
+   err = device_add(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register connection's 
dev\n");
+   return err;
+   }
+   err = transport_register_device(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register transport's 
dev\n");
+   device_del(>dev);
+   return err;
+   }
+
+   spin_lock_irqsave(, flags);
+   list_add(>conn_list, );
+   spin_unlock_irqrestore(, flags);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iscsi_add_conn);
+
+/**
+ * iscsi_remove_conn - remove iscsi class connection from sysfs
+ * @conn: iscsi cls connection
+ *
+ * this would remove iscsi_cls_conn from sysfs, and wait for previous
+ * read/write of iscsi_cls_conn's attributes in sysfs finishing
+ */
+void iscsi_remove_conn(struct iscsi_cls_conn *conn)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(, flags);
+   list_del(>conn_list);
+   spin_unlock_irqrestore(, flags);
+
+   transport_unregister_device(>dev);
+   device_del(>dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_remove_conn);
+
 /**
  * iscsi_create_conn - create iscsi class connection
  * @session: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..ae686addde0c 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,10 @@ extern struct iscsi_cls_session 
*iscsi_create_session(struct Scsi_Host *shost,
unsigned int target_id);
 extern void iscsi_remove_session(struct iscsi_cls_session *session);
 extern void iscsi_free_session(struct iscsi_cls_session *session);
+extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
+   int dd_size, uint32_t cid);
+extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
+extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
 extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
int dd_size, uint32_t cid);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 

Re: [PATCH v2 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn

['Wenchao Hao' via open-iscsi]

On 2022/3/9 1:19, Mike Christie wrote:

On 3/8/22 9:09 PM, Wenchao Hao wrote:

iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn
iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs.
iscsi_remove_conn(): remove iscsi_cls_conn from sysfs
iscsi_free_conn(): free iscsi_cls_conn

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
  drivers/scsi/scsi_transport_iscsi.c | 107 
  include/scsi/scsi_transport_iscsi.h |   5 ++
  2 files changed, 112 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..8e97c6f88359 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2340,6 +2340,113 @@ void iscsi_free_session(struct iscsi_cls_session 
*session)
  }
  EXPORT_SYMBOL_GPL(iscsi_free_session);
  
+/**

+ * iscsi_alloc_conn - alloc iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ *
+ * This can be called from a LLD or iscsi_transport. The connection
+ * is child of the session so cid must be unique for all connections
+ * on the session.
+ *
+ * Since we do not support MCS, cid will normally be zero. In some cases
+ * for software iscsi we could be trying to preallocate a connection struct
+ * in which case there could be two connection structs and cid would be
+ * non-zero.


Is that with the upstream iscsi tools or your version? I don't think the comment
is needed or is needed somewhere else.

If this happens then they will have the same sysfs/device name so when we do the
device_add it will spit an error about duplicate names.



+ */
+struct iscsi_cls_conn *
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+   struct iscsi_transport *transport = session->transport;
+   struct iscsi_cls_conn *conn;
+
+   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
+   if (!conn)
+   return NULL;
+   if (dd_size)
+   conn->dd_data = [1];
+
+   mutex_init(>ep_mutex);
+   INIT_LIST_HEAD(>conn_list);
+   INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
+   conn->transport = transport;
+   conn->cid = cid;
+   conn->state = ISCSI_CONN_DOWN;
+
+   /* this is released in the dev's release function */
+   if (!get_device(>dev))
+   goto free_conn;
+
+   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
+   device_initialize(>dev);
+   conn->dev.parent = >dev;
+   conn->dev.release = iscsi_conn_release;
+
+   return conn;
+
+free_conn:
+   kfree(conn);
+   return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+   int err;
+   unsigned long flags;
+   struct iscsi_cls_session *session = 
iscsi_dev_to_session(conn->dev.parent);
+
+   err = device_add(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register connection's 
dev\n");
+   put_device(>dev);


I would call iscsi_free_conn. instead of put_device.



Sorry I noticed it but forget to remove it. Here should not call 
put_device() or iscsi_free_conn(). If iscsi_add_conn() failed, we shoule 
not call any put operation which might cause resource free.



+   return err;
+   }
+   err = transport_register_device(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register transport's 
dev\n");
+   device_del(>dev);
+   put_device(>dev);



Is for the get_device(>dev) in iscsi_alloc_conn? If so you don't need 
to
do it because when the last put is done on the conn->dev, it will call
iscsi_conn_release which does the put on the session when it does 
"put_device(parent).

Or did you mean to call put_device on the conn->dev?



As above, we shouldn't call put_device() here.


I would do device_el(>dev) then do a goto free_conn at the bottom which
does iscsi_free_conn. The place above should do the goto as well.




--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/d7a0405f-f0df-1db0-e95e-562db1ef064f%40huawei.com.

[PATCH v2 2/3] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized

['Wenchao Hao' via open-iscsi]

iscsi_create_conn() would expose iscsi_cls_conn to sysfs, while the
initialization of iscsi_conn's dd_data is not ready now. When userspace
try to access an attribute such as connect's address, it might cause
a NULL pointer dereference.

So we should add iscsi_cls_conn to sysfs until it has been initialized.

Remove iscsi_create_conn() by hand since it is not used now.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/libiscsi.c | 13 -
 drivers/scsi/scsi_transport_iscsi.c | 76 -
 include/scsi/scsi_transport_iscsi.h |  2 -
 3 files changed, 11 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..43f903bce0b8 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3040,8 +3040,9 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
struct iscsi_conn *conn;
struct iscsi_cls_conn *cls_conn;
char *data;
+   int err;
 
-   cls_conn = iscsi_create_conn(cls_session, sizeof(*conn) + dd_size,
+   cls_conn = iscsi_alloc_conn(cls_session, sizeof(*conn) + dd_size,
 conn_idx);
if (!cls_conn)
return NULL;
@@ -3078,13 +3079,21 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
goto login_task_data_alloc_fail;
conn->login_task->data = conn->data = data;
 
+   err = iscsi_add_conn(cls_conn);
+   if (err)
+   goto login_task_add_dev_fail;
+
return cls_conn;
 
+login_task_add_dev_fail:
+   free_pages((unsigned long) conn->data,
+  get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+
 login_task_data_alloc_fail:
kfifo_in(>cmdpool.queue, (void*)>login_task,
sizeof(void*));
 login_task_alloc_fail:
-   iscsi_destroy_conn(cls_conn);
+   iscsi_free_conn(cls_conn);
return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_setup);
diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 8e97c6f88359..ca724eed4f4d 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2447,82 +2447,6 @@ void iscsi_free_conn(struct iscsi_cls_conn *conn)
 }
 EXPORT_SYMBOL_GPL(iscsi_free_conn);
 
-/**
- * iscsi_create_conn - create iscsi class connection
- * @session: iscsi cls session
- * @dd_size: private driver data size
- * @cid: connection id
- *
- * This can be called from a LLD or iscsi_transport. The connection
- * is child of the session so cid must be unique for all connections
- * on the session.
- *
- * Since we do not support MCS, cid will normally be zero. In some cases
- * for software iscsi we could be trying to preallocate a connection struct
- * in which case there could be two connection structs and cid would be
- * non-zero.
- */
-struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
-{
-   struct iscsi_transport *transport = session->transport;
-   struct iscsi_cls_conn *conn;
-   unsigned long flags;
-   int err;
-
-   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
-   if (!conn)
-   return NULL;
-   if (dd_size)
-   conn->dd_data = [1];
-
-   mutex_init(>ep_mutex);
-   INIT_LIST_HEAD(>conn_list);
-   INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
-   conn->transport = transport;
-   conn->cid = cid;
-   conn->state = ISCSI_CONN_DOWN;
-
-   /* this is released in the dev's release function */
-   if (!get_device(>dev))
-   goto free_conn;
-
-   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
-   conn->dev.parent = >dev;
-   conn->dev.release = iscsi_conn_release;
-   err = device_register(>dev);
-   if (err) {
-   iscsi_cls_session_printk(KERN_ERR, session, "could not "
-"register connection's dev\n");
-   goto release_parent_ref;
-   }
-   err = transport_register_device(>dev);
-   if (err) {
-   iscsi_cls_session_printk(KERN_ERR, session, "could not "
-"register transport's dev\n");
-   goto release_conn_ref;
-   }
-
-   spin_lock_irqsave(, flags);
-   list_add(>conn_list, );
-   spin_unlock_irqrestore(, flags);
-
-   ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
-   return conn;
-
-release_conn_ref:
-   device_unregister(>dev);
-   put_device(>dev);
-   return NULL;
-release_parent_ref:
-   put_device(>dev);
-free_conn:
-   kfree(conn);
-   return NULL;
-}
-
-EXPORT_SYMBOL_GPL(iscsi_create_conn);
-
 /**
  * iscsi_destroy_conn - destroy iscsi class connection
  * @conn: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 346f65bc3861..505764942f5e 

[PATCH v2 3/3] scsi:libiscsi: teardown iscsi_cls_conn gracefully

['Wenchao Hao' via open-iscsi]

commit 1b8d0300a3e9 ("scsi: libiscsi: Fix UAF in
iscsi_conn_get_param()/iscsi_conn_teardown()") fixed an UAF in
iscsi_conn_get_param() and introduced 2 tmp_xxx varibles, the
implement looks ugly.

We can fix this UAF with the help of device_del() gracefully.
Call iscsi_remove_conn() at the beginning of iscsi_conn_teardown would
make userspace unable to see iscsi_cls_conn any more, then we can free
memory safely.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/libiscsi.c | 8 
 drivers/scsi/scsi_transport_iscsi.c | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 43f903bce0b8..0d57521751dd 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3109,8 +3109,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
 {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_session *session = conn->session;
-   char *tmp_persistent_address = conn->persistent_address;
-   char *tmp_local_ipaddr = conn->local_ipaddr;
+
+   iscsi_remove_conn(cls_conn);
 
del_timer_sync(>transport_timer);
 
@@ -3132,6 +3132,8 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
spin_lock_bh(>frwd_lock);
free_pages((unsigned long) conn->data,
   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+   kfree(conn->persistent_address);
+   kfree(conn->local_ipaddr);
/* regular RX path uses back_lock */
spin_lock_bh(>back_lock);
kfifo_in(>cmdpool.queue, (void*)>login_task,
@@ -3143,8 +3145,6 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn)
mutex_unlock(>eh_mutex);
 
iscsi_destroy_conn(cls_conn);
-   kfree(tmp_persistent_address);
-   kfree(tmp_local_ipaddr);
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_teardown);
 
diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index ca724eed4f4d..7b4d998708e7 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2165,6 +2165,7 @@ static int iscsi_iter_destroy_conn_fn(struct device *dev, 
void *data)
 {
if (!iscsi_is_conn_dev(dev))
return 0;
+   iscsi_remove_conn(iscsi_dev_to_conn(dev));
return iscsi_destroy_conn(iscsi_dev_to_conn(dev));
 }
 
@@ -2463,7 +2464,7 @@ int iscsi_destroy_conn(struct iscsi_cls_conn *conn)
 
transport_unregister_device(>dev);
ISCSI_DBG_TRANS_CONN(conn, "Completing conn destruction\n");
-   device_unregister(>dev);
+   iscsi_free_conn(conn);
return 0;
 }
 EXPORT_SYMBOL_GPL(iscsi_destroy_conn);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220309030916.2932316-4-haowenchao%40huawei.com.

[PATCH v2 0/3] scsi:iscsi: handle iscsi_cls_conn device with sysfs

['Wenchao Hao' via open-iscsi]

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

iscsi_create_conn() expose iscsi_cls_conn to sysfs while the related
resources are not initialized. So we should delay the calling of
device_add() until these resources has been initialized.

This patchset solve this issue by correct the add and teardown flow
of iscsi_cls_conn.

---
V2:
  add two more iscsi_free_conn() and iscsi_remove_conn() than V1
  change the teardown flow of iscsi_cls_conn

Wenchao Hao (3):
  scsi: iscsi: Add helper functions to manage iscsi_cls_conn
  scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized
  scsi:libiscsi: teardown iscsi_cls_conn gracefully

 drivers/scsi/libiscsi.c | 21 ++---
 drivers/scsi/scsi_transport_iscsi.c | 72 +
 include/scsi/scsi_transport_iscsi.h |  5 +-
 3 files changed, 71 insertions(+), 27 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220309030916.2932316-1-haowenchao%40huawei.com.

[PATCH v2 1/3] scsi: iscsi: Add helper functions to manage iscsi_cls_conn

['Wenchao Hao' via open-iscsi]

iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn
iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs.
iscsi_remove_conn(): remove iscsi_cls_conn from sysfs
iscsi_free_conn(): free iscsi_cls_conn

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/scsi_transport_iscsi.c | 107 
 include/scsi/scsi_transport_iscsi.h |   5 ++
 2 files changed, 112 insertions(+)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..8e97c6f88359 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2340,6 +2340,113 @@ void iscsi_free_session(struct iscsi_cls_session 
*session)
 }
 EXPORT_SYMBOL_GPL(iscsi_free_session);
 
+/**
+ * iscsi_alloc_conn - alloc iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ *
+ * This can be called from a LLD or iscsi_transport. The connection
+ * is child of the session so cid must be unique for all connections
+ * on the session.
+ *
+ * Since we do not support MCS, cid will normally be zero. In some cases
+ * for software iscsi we could be trying to preallocate a connection struct
+ * in which case there could be two connection structs and cid would be
+ * non-zero.
+ */
+struct iscsi_cls_conn *
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+   struct iscsi_transport *transport = session->transport;
+   struct iscsi_cls_conn *conn;
+
+   conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
+   if (!conn)
+   return NULL;
+   if (dd_size)
+   conn->dd_data = [1];
+
+   mutex_init(>ep_mutex);
+   INIT_LIST_HEAD(>conn_list);
+   INIT_WORK(>cleanup_work, iscsi_cleanup_conn_work_fn);
+   conn->transport = transport;
+   conn->cid = cid;
+   conn->state = ISCSI_CONN_DOWN;
+
+   /* this is released in the dev's release function */
+   if (!get_device(>dev))
+   goto free_conn;
+
+   dev_set_name(>dev, "connection%d:%u", session->sid, cid);
+   device_initialize(>dev);
+   conn->dev.parent = >dev;
+   conn->dev.release = iscsi_conn_release;
+
+   return conn;
+
+free_conn:
+   kfree(conn);
+   return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+   int err;
+   unsigned long flags;
+   struct iscsi_cls_session *session = 
iscsi_dev_to_session(conn->dev.parent);
+
+   err = device_add(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register connection's 
dev\n");
+   put_device(>dev);
+   return err;
+   }
+   err = transport_register_device(>dev);
+   if (err) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"could not register transport's 
dev\n");
+   device_del(>dev);
+   put_device(>dev);
+   return err;
+   }
+
+   spin_lock_irqsave(, flags);
+   list_add(>conn_list, );
+   spin_unlock_irqrestore(, flags);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iscsi_add_conn);
+
+/**
+ * iscsi_remove_conn - remove iscsi class connection from sysfs
+ * @conn: iscsi cls connection
+ *
+ * this would remove iscsi_cls_conn from sysfs, and wait for previous
+ * read/write of iscsi_cls_conn's attributes in sysfs finishing
+ */
+void iscsi_remove_conn(struct iscsi_cls_conn *conn)
+{
+   device_del(>dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_remove_conn);
+
+void iscsi_free_conn(struct iscsi_cls_conn *conn)
+{
+   put_device(>dev);
+}
+EXPORT_SYMBOL_GPL(iscsi_free_conn);
+
 /**
  * iscsi_create_conn - create iscsi class connection
  * @session: iscsi cls session
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..346f65bc3861 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,11 @@ extern struct iscsi_cls_session 
*iscsi_create_session(struct Scsi_Host *shost,
unsigned int target_id);
 extern void iscsi_remove_session(struct iscsi_cls_session *session);
 extern void iscsi_free_session(struct iscsi_cls_session *session);
+extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
+   int dd_size, uint32_t cid);
+extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
+extern void iscsi_remove_conn(struct iscsi_cls_conn *conn);
+extern void iscsi_free_conn(struct iscsi_cls_conn *conn);
 

Re: [PATCH 1/2] iscsi_tcp: Fix NULL pointer dereference in iscsi_sw_tcp_conn_get_param()

['Wenchao Hao' via open-iscsi]

On 2022/3/3 23:03, Mike Christie wrote:

On 3/3/22 8:56 PM, Wenchao Hao wrote:

kernel might crash in iscsi_sw_tcp_conn_get_param() because it dereference
an invalid address.

The initialization of iscsi_conn's dd_data is after device_register() of
struct iscsi_cls_conn, so iscsi_conn's dd_data might not initialized when
iscsi_sw_tcp_conn_get_param() is called.



We are actually doing sysfs/device addition wrong.

We should be doing the 2 step setup where in step 1 we alloc/init.
When everything is allocated and initialized, then we should do
device_add which exposes us to sysfs. On the teardown side, we are
then supposed to do 2 steps where the remove function does device_del
which waits until sysfs accesses are completed. We can then tear
the structs down and free them and call device_put.



I reviewed the teardown flow of iscsi_cls_conn, it has already written 
as what you saied.




The exposure to NL would be similar where it goes into the wrapper
around device_add. However, see my comments on the other patch where
I don't think we can hit the bug you mention because every nl cmd
that calls into the drivers is done under the rx_queue_mutex.

I think we should separate the iscsi_create_conn function like we
do for sessions. This is going to be a little more involved because
you need to also convert iscsi_tcp_conn_setup and the drivers since
we can call into the drivers for the get_conn_param callout.
.



I hesitated about when should we call device_add(). I think there are 
two places to call it.


The first one is in iscsi_conn_setup(), after some initialization of 
conn, it keeps same with previous's implement and need not to change 
drivers' code. What's more, the change can fix iscsi_tcp's NULL pointer 
access.  While this change can not make sure the LLDs related sources 
are already initialized when iscsi_cls_conn is exposed to sysfs. It 
means LLDs' callback are still responsible to check if the resources are 
accessible.


Another one is in create_conn callback for each driver's 
iscsi_transport.  This need us to change each driver's code.


I send 2 patches which make changes in iscsi_conn_setup(), it's ok with 
iscsi_tcp, would you help to review them?


--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/c2891c08-5809-1f56-8783-357e6df1bc1a%40huawei.com.

[PATCH 1/2] scsi: iscsi: Add helper functions to alloc and add iscsi_cls_conn

['Wenchao Hao' via open-iscsi]

iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
not expose it to userspace.
iscsi_add_conn() would expose it to userspace.

LLDs should split the alloc and register to 2 steps.

And simplify iscsi_create_conn() with these helper functions.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/scsi_transport_iscsi.c | 85 +++--
 include/scsi/scsi_transport_iscsi.h |  3 +
 2 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..092d4429bb1d 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2341,7 +2341,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
 EXPORT_SYMBOL_GPL(iscsi_free_session);
 
 /**
- * iscsi_create_conn - create iscsi class connection
+ * iscsi_alloc_conn - alloc iscsi class connection
  * @session: iscsi cls session
  * @dd_size: private driver data size
  * @cid: connection id
@@ -2356,12 +2356,10 @@ EXPORT_SYMBOL_GPL(iscsi_free_session);
  * non-zero.
  */
 struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
 {
struct iscsi_transport *transport = session->transport;
struct iscsi_cls_conn *conn;
-   unsigned long flags;
-   int err;
 
conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
if (!conn)
@@ -2383,35 +2381,90 @@ iscsi_create_conn(struct iscsi_cls_session *session, 
int dd_size, uint32_t cid)
dev_set_name(>dev, "connection%d:%u", session->sid, cid);
conn->dev.parent = >dev;
conn->dev.release = iscsi_conn_release;
+
+   return conn;
+
+free_conn:
+   kfree(conn);
+   return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+   int err;
+   unsigned long flags;
+   struct iscsi_cls_session *session = 
iscsi_dev_to_session(conn->dev.parent);
+
err = device_register(>dev);
if (err) {
iscsi_cls_session_printk(KERN_ERR, session, "could not "
 "register connection's dev\n");
-   goto release_parent_ref;
+   put_device(>dev);
+   return err;
}
err = transport_register_device(>dev);
if (err) {
iscsi_cls_session_printk(KERN_ERR, session, "could not "
 "register transport's dev\n");
-   goto release_conn_ref;
+   device_unregister(>dev);
+   put_device(>dev);
+   return err;
}
 
spin_lock_irqsave(, flags);
list_add(>conn_list, );
spin_unlock_irqrestore(, flags);
 
+   return 0;
+}
+EXPORT_SYMBOL_GPL(iscsi_add_conn);
+
+/**
+ * iscsi_create_conn - create iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ *
+ * This can be called from a LLD or iscsi_transport. The connection
+ * is child of the session so cid must be unique for all connections
+ * on the session.
+ *
+ * Since we do not support MCS, cid will normally be zero. In some cases
+ * for software iscsi we could be trying to preallocate a connection struct
+ * in which case there could be two connection structs and cid would be
+ * non-zero.
+ *
+ * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
+ * means attributes of iscsi_cls_conn are accessible to userspace. So the
+ * caller must make sure everything related these sysfs attributes are
+ * already initialized.
+ */
+struct iscsi_cls_conn *
+iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+   struct iscsi_cls_conn *conn;
+   int err;
+
+   conn = iscsi_alloc_conn(session, dd_size, cid);
+   if (!conn)
+   return NULL;
+
+   err = iscsi_add_conn(conn);
+   if (err) {
+   kfree(conn);
+   return NULL;
+   }
+
ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
return conn;
-
-release_conn_ref:
-   device_unregister(>dev);
-   put_device(>dev);
-   return NULL;
-release_parent_ref:
-   put_device(>dev);
-free_conn:
-   kfree(conn);
-   return NULL;
 }
 
 EXPORT_SYMBOL_GPL(iscsi_create_conn);
diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..fd9ce99c2186 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,9 @@ extern struct iscsi_cls_session 

[PATCH 2/2] scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized

['Wenchao Hao' via open-iscsi]

iscsi_create_conn() would expose iscsi_cls_conn to sysfs, while the
initialization of iscsi_conn's dd_data is not ready now. When userspace
try to access an attribute such as connect's address, it might cause
a NULL pointer dereference.

So we should add iscsi_cls_conn to sysfs until it has been initialized.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/libiscsi.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 059dae8909ee..1cf25f4acb71 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3040,8 +3040,9 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
struct iscsi_conn *conn;
struct iscsi_cls_conn *cls_conn;
char *data;
+   int err;
 
-   cls_conn = iscsi_create_conn(cls_session, sizeof(*conn) + dd_size,
+   cls_conn = iscsi_alloc_conn(cls_session, sizeof(*conn) + dd_size,
 conn_idx);
if (!cls_conn)
return NULL;
@@ -3078,13 +3079,21 @@ iscsi_conn_setup(struct iscsi_cls_session *cls_session, 
int dd_size,
goto login_task_data_alloc_fail;
conn->login_task->data = conn->data = data;
 
+   err = iscsi_add_conn(cls_conn);
+   if (err)
+   goto login_task_add_dev_fail;
+
return cls_conn;
 
+login_task_add_dev_fail:
+   free_pages((unsigned long) conn->data,
+  get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
+
 login_task_data_alloc_fail:
kfifo_in(>cmdpool.queue, (void*)>login_task,
sizeof(void*));
 login_task_alloc_fail:
-   iscsi_destroy_conn(cls_conn);
+   kfree(cls_conn);
return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_conn_setup);
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220308005654.2281343-3-haowenchao%40huawei.com.

[PATCH 0/2]scsi:libiscsi: Add iscsi_cls_conn device to sysfs correctly

['Wenchao Hao' via open-iscsi]

We found a NULL pointer dereference in iscsi_sw_tcp_conn_get_param(),
the root reason is we did sysfs addition wrong.

iscsi_create_conn() expose iscsi_cls_conn to sysfs while the related
resources are not initialized. So we should delay the calling of
device_add() until these resources has been initialized.

This patchset solve this issue by changing iscsi_conn_setup() and works 
well for iscsi_tcp.

Wenchao Hao (2):
  scsi: iscsi: Add helper functions to alloc and add iscsi_cls_conn
  scsi:libiscsi: Add iscsi_cls_conn to sysfs after been initialized

 drivers/scsi/libiscsi.c | 13 -
 drivers/scsi/scsi_transport_iscsi.c | 85 +++--
 include/scsi/scsi_transport_iscsi.h |  3 +
 3 files changed, 83 insertions(+), 18 deletions(-)

-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220308005654.2281343-1-haowenchao%40huawei.com.

Re: [PATCH 1/2] iscsi_tcp: Fix NULL pointer dereference in iscsi_sw_tcp_conn_get_param()

['Wenchao Hao' via open-iscsi]

On 2022/3/3 23:03, Mike Christie wrote:

On 3/3/22 8:56 PM, Wenchao Hao wrote:

kernel might crash in iscsi_sw_tcp_conn_get_param() because it dereference
an invalid address.

The initialization of iscsi_conn's dd_data is after device_register() of
struct iscsi_cls_conn, so iscsi_conn's dd_data might not initialized when
iscsi_sw_tcp_conn_get_param() is called.

Following stack would be reported and kernel would panic.

[449311.812887] Unable to handle kernel NULL pointer dereference at virtual 
address 0008
[449311.812893] Mem abort info:
[449311.812895]   ESR = 0x9604
[449311.812899]   Exception class = DABT (current EL), IL = 32 bits
[449311.812901]   SET = 0, FnV = 0
[449311.812903]   EA = 0, S1PTW = 0
[449311.812905] Data abort info:
[449311.812907]   ISV = 0, ISS = 0x0004
[449311.812909]   CM = 0, WnR = 0
[449311.812915] user pgtable: 4k pages, 48-bit VAs, pgdp = e26e7ace
[449311.812918] [0008] pgd=
[449311.812925] Internal error: Oops: 9604 [#1] SMP
[449311.814974] Process iscsiadm (pid: 8286, stack limit = 0x800010f78000)
[449311.815570] CPU: 0 PID: 8286 Comm: iscsiadm Kdump: loaded Tainted: GB   
W 4.19.90-vhulk2201.1.0.h1021.kasan.eulerosv2r10.aarch64 #1
[449311.816584] sd 1:0:0:1: [sdg] Attached SCSI disk
[449311.816695] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[449311.817677] pstate: 4045 (nZcv daif +PAN -UAO)
[449311.818121] pc : iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
[449311.818688] lr : iscsi_sw_tcp_conn_get_param+0xe8/0x300 [iscsi_tcp]
[449311.819244] sp : 800010f7f890
[449311.819542] x29: 800010f7f890 x28: 8000cb1bea38
[449311.820025] x27: 800010911010 x26: 228887a4
[449311.820500] x25: 89200d98 x24: 800010911000
[449311.820973] x23:  x22: 8000cb1bea28
[449311.821458] x21: 0015 x20: 200081afa000
[449311.821934] x19: 121eff20 x18: 
[449311.822414] x17:  x16: 200080618220
[449311.822891] x15:  x14: 
[449311.823413] x13:  x12: 
[449311.823897] x11: 10001ab4f41f x10: 10001ab4f41f
[449311.824373] x9 :  x8 : 8000d5a7a100
[449311.824847] x7 :  x6 : dfff2000
[449311.825329] x5 : 121eff20 x4 : 8000cb1bea30
[449311.825806] x3 : 22911178 x2 : 2000841ff000
[449311.826281] x1 : e0c234eab8420c00 x0 : 8000cb1bea38
[449311.826756] Call trace:
[449311.826987]  iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
[449311.827550]  show_conn_ep_param_ISCSI_PARAM_CONN_ADDRESS+0xe4/0x100 
[scsi_transport_iscsi]
[449311.828304]  dev_attr_show+0x58/0xb0
[449311.828639]  sysfs_kf_seq_show+0x124/0x210
[449311.829014]  kernfs_seq_show+0x8c/0xa0
[449311.829362]  seq_read+0x188/0x8a0
[449311.829667]  kernfs_fop_read+0x250/0x398
[449311.830024]  __vfs_read+0xe0/0x350
[449311.830339]  vfs_read+0xbc/0x1c0
[449311.830635]  ksys_read+0xdc/0x1b8
[449311.830941]  __arm64_sys_read+0x50/0x60
[449311.831295]  el0_svc_common+0xc8/0x320
[449311.831642]  el0_svc_handler+0xf8/0x160
[449311.831998]  el0_svc+0x10/0x218
[449311.832292] Code: f94006d7 910022e0 940007bb aa1c03e0 (f94006f9)

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
  drivers/scsi/iscsi_tcp.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 1bc37593c88f..14db224486be 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -741,11 +741,16 @@ static int iscsi_sw_tcp_conn_get_param(struct 
iscsi_cls_conn *cls_conn,
  {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
-   struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+   struct iscsi_sw_tcp_conn *tcp_sw_conn;
struct sockaddr_in6 addr;
struct socket *sock;
int rc;
  
+	if (!tcp_conn)

+   return -ENOTCONN;
+
+   tcp_sw_conn = tcp_conn->dd_data;
+
switch(param) {
case ISCSI_PARAM_CONN_PORT:
case ISCSI_PARAM_CONN_ADDRESS:


We are actually doing sysfs/device addition wrong.

We should be doing the 2 step setup where in step 1 we alloc/init.
When everything is allocated and initialized, then we should do
device_add which exposes us to sysfs. On the teardown side, we are
then supposed to do 2 steps where the remove function does device_del
which waits until sysfs accesses are completed. We can then tear
the structs down and free them and call device_put.



I agree with this, and I would try to split device_add() from 
iscsi_create_conn().


What's more I would do some check between sysfs files add/remove and 
kernel object initialize/release to make a micro-refactoring



The exposure to NL would be similar where it goes into the wrapper
around device_add. However, see my comments on the other patch where
I 

Re: [PATCH 2/2] iscsi_tcp: Check if tcp_conn is valid in

['Wenchao Hao' via open-iscsi]

On 2022/3/3 22:59, Mike Christie wrote:

On 3/3/22 8:56 PM, Wenchao Hao wrote:

iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist,
it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup()
would found this iscsi_cls_conn and call the set_param callback which is
iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not
been initialized.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
  drivers/scsi/iscsi_tcp.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 14db224486be..a42449df6156 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct 
iscsi_cls_conn *cls_conn,
  {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
-   struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+   struct iscsi_sw_tcp_conn *tcp_sw_conn;
  
  	switch(param) {

case ISCSI_PARAM_HDRDGST_EN:
iscsi_set_param(cls_conn, param, buf, buflen);
break;
case ISCSI_PARAM_DATADGST_EN:
+   if (!tcp_conn || !tcp_conn->dd_data)
+   return -ENOTCONN;
+
+   tcp_sw_conn = tcp_conn->dd_data;
iscsi_set_param(cls_conn, param, buf, buflen);
tcp_sw_conn->sendpage = conn->datadgst_en ?
sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;


Is this something you hit or from code review?



It's from code review. I reviewed the code because the panic mentioned 
in my first patch. The issue seems would not happen, so just ignore it.



We have those state checks:

if ((conn->state == ISCSI_CONN_BOUND) ||
 (conn->state == ISCSI_CONN_UP)) {
err = transport->set_param(conn, ev->u.set_param.param,

so we don't call set_param until after we have bound the
connection which will be after ISCSI_UEVENT_CREATE_CONN has returned.

Also for this specific bug iscsi_if_recv_msg is called with the
rx_queue_mutex, so set_param can only be called after the
ISCSI_UEVENT_CREATE_CONN cmd has returned.
.



--
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/f6055c4a-b32f-8025-f096-f6abda03e2d4%40huawei.com.

[PATCH 2/2] iscsi_tcp: Check if tcp_conn is valid in

['Wenchao Hao' via open-iscsi]

iscsi_create_conn() would add newly alloced iscsi_cls_conn to connlist,
it means when userspace sends ISCSI_UEVENT_SET_PARAM, iscsi_conn_lookup()
would found this iscsi_cls_conn and call the set_param callback which is
iscsi_sw_tcp_conn_set_param(). While the iscsi_conn's dd_data might not
been initialized.

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/iscsi_tcp.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 14db224486be..a42449df6156 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -716,13 +716,17 @@ static int iscsi_sw_tcp_conn_set_param(struct 
iscsi_cls_conn *cls_conn,
 {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
-   struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+   struct iscsi_sw_tcp_conn *tcp_sw_conn;
 
switch(param) {
case ISCSI_PARAM_HDRDGST_EN:
iscsi_set_param(cls_conn, param, buf, buflen);
break;
case ISCSI_PARAM_DATADGST_EN:
+   if (!tcp_conn || !tcp_conn->dd_data)
+   return -ENOTCONN;
+
+   tcp_sw_conn = tcp_conn->dd_data;
iscsi_set_param(cls_conn, param, buf, buflen);
tcp_sw_conn->sendpage = conn->datadgst_en ?
sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220304025608.1874516-2-haowenchao%40huawei.com.

[PATCH 1/2] iscsi_tcp: Fix NULL pointer dereference in iscsi_sw_tcp_conn_get_param()

['Wenchao Hao' via open-iscsi]

kernel might crash in iscsi_sw_tcp_conn_get_param() because it dereference
an invalid address.

The initialization of iscsi_conn's dd_data is after device_register() of
struct iscsi_cls_conn, so iscsi_conn's dd_data might not initialized when
iscsi_sw_tcp_conn_get_param() is called.

Following stack would be reported and kernel would panic.

[449311.812887] Unable to handle kernel NULL pointer dereference at virtual 
address 0008
[449311.812893] Mem abort info:
[449311.812895]   ESR = 0x9604
[449311.812899]   Exception class = DABT (current EL), IL = 32 bits
[449311.812901]   SET = 0, FnV = 0
[449311.812903]   EA = 0, S1PTW = 0
[449311.812905] Data abort info:
[449311.812907]   ISV = 0, ISS = 0x0004
[449311.812909]   CM = 0, WnR = 0
[449311.812915] user pgtable: 4k pages, 48-bit VAs, pgdp = e26e7ace
[449311.812918] [0008] pgd=
[449311.812925] Internal error: Oops: 9604 [#1] SMP
[449311.814974] Process iscsiadm (pid: 8286, stack limit = 0x800010f78000)
[449311.815570] CPU: 0 PID: 8286 Comm: iscsiadm Kdump: loaded Tainted: GB   
W 4.19.90-vhulk2201.1.0.h1021.kasan.eulerosv2r10.aarch64 #1
[449311.816584] sd 1:0:0:1: [sdg] Attached SCSI disk
[449311.816695] Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
[449311.817677] pstate: 4045 (nZcv daif +PAN -UAO)
[449311.818121] pc : iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
[449311.818688] lr : iscsi_sw_tcp_conn_get_param+0xe8/0x300 [iscsi_tcp]
[449311.819244] sp : 800010f7f890
[449311.819542] x29: 800010f7f890 x28: 8000cb1bea38
[449311.820025] x27: 800010911010 x26: 228887a4
[449311.820500] x25: 89200d98 x24: 800010911000
[449311.820973] x23:  x22: 8000cb1bea28
[449311.821458] x21: 0015 x20: 200081afa000
[449311.821934] x19: 121eff20 x18: 
[449311.822414] x17:  x16: 200080618220
[449311.822891] x15:  x14: 
[449311.823413] x13:  x12: 
[449311.823897] x11: 10001ab4f41f x10: 10001ab4f41f
[449311.824373] x9 :  x8 : 8000d5a7a100
[449311.824847] x7 :  x6 : dfff2000
[449311.825329] x5 : 121eff20 x4 : 8000cb1bea30
[449311.825806] x3 : 22911178 x2 : 2000841ff000
[449311.826281] x1 : e0c234eab8420c00 x0 : 8000cb1bea38
[449311.826756] Call trace:
[449311.826987]  iscsi_sw_tcp_conn_get_param+0xec/0x300 [iscsi_tcp]
[449311.827550]  show_conn_ep_param_ISCSI_PARAM_CONN_ADDRESS+0xe4/0x100 
[scsi_transport_iscsi]
[449311.828304]  dev_attr_show+0x58/0xb0
[449311.828639]  sysfs_kf_seq_show+0x124/0x210
[449311.829014]  kernfs_seq_show+0x8c/0xa0
[449311.829362]  seq_read+0x188/0x8a0
[449311.829667]  kernfs_fop_read+0x250/0x398
[449311.830024]  __vfs_read+0xe0/0x350
[449311.830339]  vfs_read+0xbc/0x1c0
[449311.830635]  ksys_read+0xdc/0x1b8
[449311.830941]  __arm64_sys_read+0x50/0x60
[449311.831295]  el0_svc_common+0xc8/0x320
[449311.831642]  el0_svc_handler+0xf8/0x160
[449311.831998]  el0_svc+0x10/0x218
[449311.832292] Code: f94006d7 910022e0 940007bb aa1c03e0 (f94006f9)

Signed-off-by: Wenchao Hao 
Signed-off-by: Wu Bo 
---
 drivers/scsi/iscsi_tcp.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 1bc37593c88f..14db224486be 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -741,11 +741,16 @@ static int iscsi_sw_tcp_conn_get_param(struct 
iscsi_cls_conn *cls_conn,
 {
struct iscsi_conn *conn = cls_conn->dd_data;
struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
-   struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
+   struct iscsi_sw_tcp_conn *tcp_sw_conn;
struct sockaddr_in6 addr;
struct socket *sock;
int rc;
 
+   if (!tcp_conn)
+   return -ENOTCONN;
+
+   tcp_sw_conn = tcp_conn->dd_data;
+
switch(param) {
case ISCSI_PARAM_CONN_PORT:
case ISCSI_PARAM_CONN_ADDRESS:
-- 
2.32.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20220304025608.1874516-1-haowenchao%40huawei.com.