Message-ID: <d0f77858-4370-aa0c-0ece-32f4dcfe85f0@gmail.com>
Date:   Sun, 21 May 2023 13:28:20 +0200
From:   Grzegorz Uriasz <gorbak25@...il.com>
To:     Dmitry Bogdanov <d.bogdanov@...ro.com>
Cc:     "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, target-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org, dutkahugo@...il.com,
        Grzegorz Uriasz <gorbak25@...il.com>
Subject: Re: [PATCH] scsi: target: Fix data corruption under concurrent target
 configuration

Hi Dmitry,

Thank you for your feedback :)

On 20/05/2023 10:46, Dmitry Bogdanov wrote:
> Hi Grzegorz,
>
> On Sat, May 20, 2023 at 02:26:14AM +0200, Grzegorz Uriasz wrote:
>> This fixes data corruptions arising from concurrent enabling of a target
>> devices. When multiple enable calls are made concurrently then it is
>> possible for the target device to be set up twice which results in a
>> kernel BUG.
>> Introduces a per target device mutex for serializing enable requests.
> Device enable call is already secured by configfs per-file mutex. That
> is actually per device. So Enable procedures are already not executed
> simulteniously.
True, I've checked the code in configfs and indeed there is a per 
file/subsystem mutex.
>
> Look like you wrongly identified the root cause of double list_add.
>
>
> If you have an evidence that dev->dev_flags could have no DF_CONFIGURED
> bit, then it meeans that it (dev_flags) is raced in other
> configuration actions (udev_path, vpd_unit_serial, alias).
> Bits in dev->dev_flags are written not atomically and if you writes to
> enable, alias, udev_path,unit_serial files simulteniously, then some
> bits could be lost.
>
> IHMO the best solution is to make dev_flags changes be atomical.

I've tried that using the following patch:
---
  drivers/target/target_core_configfs.c | 12 ++++++------
  drivers/target/target_core_device.c   |  2 +-
  drivers/target/target_core_iblock.c   |  2 +-
  drivers/target/target_core_pscsi.c    |  4 ++--
  drivers/target/target_core_spc.c      |  8 ++++----
  drivers/target/target_core_tpg.c      |  2 +-
  include/target/target_core_backend.h  |  2 +-
  include/target/target_core_base.h     |  4 ++--
  8 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 74b67c346dfe..bdc06f654aa8 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1621,7 +1621,7 @@ static ssize_t 
target_wwn_vpd_unit_serial_store(struct config_item *item,
       * it is doing 'the right thing' wrt a world wide unique
       * VPD Unit Serial Number that OS dependent multipath can depend on.
       */
-    if (dev->dev_flags & DF_FIRMWARE_VPD_UNIT_SERIAL) {
+    if (atomic_read(&dev->dev_flags) & DF_FIRMWARE_VPD_UNIT_SERIAL) {
          pr_err("Underlying SCSI device firmware provided VPD"
              " Unit Serial, ignoring request\n");
          return -EOPNOTSUPP;
@@ -1654,7 +1654,7 @@ static ssize_t 
target_wwn_vpd_unit_serial_store(struct config_item *item,
      snprintf(buf, INQUIRY_VPD_SERIAL_LEN, "%s", page);
      snprintf(dev->t10_wwn.unit_serial, INQUIRY_VPD_SERIAL_LEN,
              "%s", strstrip(buf));
-    dev->dev_flags |= DF_EMULATED_VPD_UNIT_SERIAL;
+    atomic_or(DF_EMULATED_VPD_UNIT_SERIAL, &dev->dev_flags);

      pr_debug("Target_Core_ConfigFS: Set emulated VPD Unit Serial:"
              " %s\n", dev->t10_wwn.unit_serial);
@@ -2263,7 +2263,7 @@ static ssize_t target_dev_alias_show(struct 
config_item *item, char *page)
  {
      struct se_device *dev = to_device(item);

-    if (!(dev->dev_flags & DF_USING_ALIAS))
+    if (!(atomic_read(&dev->dev_flags) & DF_USING_ALIAS))
          return 0;

      return snprintf(page, PAGE_SIZE, "%s\n", dev->dev_alias);
@@ -2289,7 +2289,7 @@ static ssize_t target_dev_alias_store(struct 
config_item *item,
      if (dev->dev_alias[read_bytes - 1] == '\n')
          dev->dev_alias[read_bytes - 1] = '\0';

-    dev->dev_flags |= DF_USING_ALIAS;
+    atomic_or(DF_USING_ALIAS, &dev->dev_flags);

      pr_debug("Target_Core_ConfigFS: %s/%s set alias: %s\n",
          config_item_name(&hba->hba_group.cg_item),
@@ -2303,7 +2303,7 @@ static ssize_t target_dev_udev_path_show(struct 
config_item *item, char *page)
  {
      struct se_device *dev = to_device(item);

-    if (!(dev->dev_flags & DF_USING_UDEV_PATH))
+    if (!(atomic_read(&dev->dev_flags) & DF_USING_UDEV_PATH))
          return 0;

      return snprintf(page, PAGE_SIZE, "%s\n", dev->udev_path);
@@ -2330,7 +2330,7 @@ static ssize_t target_dev_udev_path_store(struct 
config_item *item,
      if (dev->udev_path[read_bytes - 1] == '\n')
          dev->udev_path[read_bytes - 1] = '\0';

-    dev->dev_flags |= DF_USING_UDEV_PATH;
+    atomic_or(DF_USING_UDEV_PATH, &dev->dev_flags);

      pr_debug("Target_Core_ConfigFS: %s/%s set udev_path: %s\n",
          config_item_name(&hba->hba_group.cg_item),
diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 90f3f4926172..5054b647dd0b 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -967,7 +967,7 @@ int target_configure_device(struct se_device *dev)
      hba->dev_count++;
      spin_unlock(&hba->device_lock);

-    dev->dev_flags |= DF_CONFIGURED;
+    atomic_or(DF_CONFIGURED, &dev->dev_flags);

      return 0;

diff --git a/drivers/target/target_core_iblock.c 
b/drivers/target/target_core_iblock.c
index cc838ffd1294..38d0d104661d 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -112,7 +112,7 @@ static int iblock_configure_device(struct se_device 
*dev)
      if (!ib_dev->ibd_readonly)
          mode |= FMODE_WRITE;
      else
-        dev->dev_flags |= DF_READ_ONLY;
+        atomic_or(DF_READ_ONLY, &dev->dev_flags);

      bd = blkdev_get_by_path(ib_dev->ibd_udev_path, mode, ib_dev);
      if (IS_ERR(bd)) {
diff --git a/drivers/target/target_core_pscsi.c 
b/drivers/target/target_core_pscsi.c
index e7425549e39c..36a1ac519f0b 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -201,7 +201,7 @@ pscsi_get_inquiry_vpd_serial(struct scsi_device 
*sdev, struct t10_wwn *wwn)

      snprintf(&wwn->unit_serial[0], INQUIRY_VPD_SERIAL_LEN, "%s", &buf[4]);

-    wwn->t10_dev->dev_flags |= DF_FIRMWARE_VPD_UNIT_SERIAL;
+    atomic_or(DF_FIRMWARE_VPD_UNIT_SERIAL, &wwn->t10_dev->dev_flags);

      kfree(buf);
      return 0;
@@ -450,7 +450,7 @@ static int pscsi_configure_device(struct se_device *dev)
           * For the newer PHV_VIRTUAL_HOST_ID struct scsi_device
           * reference, we enforce that udev_path has been set
           */
-        if (!(dev->dev_flags & DF_USING_UDEV_PATH)) {
+        if (!(atomic_read(&dev->dev_flags) & DF_USING_UDEV_PATH)) {
              pr_err("pSCSI: udev_path attribute has not"
                  " been set before ENABLE=1\n");
              return -EINVAL;
diff --git a/drivers/target/target_core_spc.c 
b/drivers/target/target_core_spc.c
index 89c0d56294cc..d380d08a2df0 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -159,7 +159,7 @@ spc_emulate_evpd_80(struct se_cmd *cmd, unsigned 
char *buf)
      struct se_device *dev = cmd->se_dev;
      u16 len;

-    if (dev->dev_flags & DF_EMULATED_VPD_UNIT_SERIAL) {
+    if (atomic_read(&dev->dev_flags) & DF_EMULATED_VPD_UNIT_SERIAL) {
          len = sprintf(&buf[4], "%s", dev->t10_wwn.unit_serial);
          len++; /* Extra Byte for NULL Terminator */
          buf[3] = len;
@@ -239,7 +239,7 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned 
char *buf)
       * /sys/kernel/config/target/core/$HBA/$DEV/wwn/vpd_unit_serial
       * value in order to return the NAA id.
       */
-    if (!(dev->dev_flags & DF_EMULATED_VPD_UNIT_SERIAL))
+    if (!(atomic_read(&dev->dev_flags) & DF_EMULATED_VPD_UNIT_SERIAL))
          goto check_t10_vend_desc;

      /* CODE SET == Binary */
@@ -267,7 +267,7 @@ spc_emulate_evpd_83(struct se_cmd *cmd, unsigned 
char *buf)
       */
      id_len = 8; /* For Vendor field */

-    if (dev->dev_flags & DF_EMULATED_VPD_UNIT_SERIAL)
+    if (atomic_read(&dev->dev_flags) & DF_EMULATED_VPD_UNIT_SERIAL)
          id_len += sprintf(&buf[off+12], "%s:%s", prod,
                  &dev->t10_wwn.unit_serial[0]);
      buf[off] = 0x2; /* ASCII */
@@ -720,7 +720,7 @@ spc_emulate_evpd_00(struct se_cmd *cmd, unsigned 
char *buf)
       * Registered Extended LUN WWN has been set via ConfigFS
       * during device creation/restart.
       */
-    if (cmd->se_dev->dev_flags & DF_EMULATED_VPD_UNIT_SERIAL) {
+    if (atomic_read(&cmd->se_dev->dev_flags) & 
DF_EMULATED_VPD_UNIT_SERIAL) {
          buf[3] = ARRAY_SIZE(evpd_handlers);
          for (p = 0; p < ARRAY_SIZE(evpd_handlers); ++p)
              buf[p + 4] = evpd_handlers[p].page;
diff --git a/drivers/target/target_core_tpg.c 
b/drivers/target/target_core_tpg.c
index c0e429e5ef31..c88dc36db6de 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -656,7 +656,7 @@ int core_tpg_add_lun(
      list_add_tail(&lun->lun_dev_link, &dev->dev_sep_list);
      spin_unlock(&dev->se_port_lock);

-    if (dev->dev_flags & DF_READ_ONLY)
+    if (atomic_read(&dev->dev_flags) & DF_READ_ONLY)
          lun->lun_access_ro = true;
      else
          lun->lun_access_ro = lun_access_ro;
diff --git a/include/target/target_core_backend.h 
b/include/target/target_core_backend.h
index a3c193df25b3..27c70a69e088 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -122,7 +122,7 @@ bool target_configure_unmap_from_queue(struct 
se_dev_attrib *attrib,

  static inline bool target_dev_configured(struct se_device *se_dev)
  {
-    return !!(se_dev->dev_flags & DF_CONFIGURED);
+    return !!(atomic_read(&se_dev->dev_flags) & DF_CONFIGURED);
  }

  #endif /* TARGET_CORE_BACKEND_H */
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index 5f8e96f1516f..5794b2360c47 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -792,8 +792,8 @@ struct se_device_queue {

  struct se_device {
      /* Used for SAM Task Attribute ordering */
-    u32            dev_cur_ordered_id;
-    u32            dev_flags;
+    u32         dev_cur_ordered_id;
+    atomic_t    dev_flags;
  #define DF_CONFIGURED                0x00000001
  #define DF_FIRMWARE_VPD_UNIT_SERIAL        0x00000002
  #define DF_EMULATED_VPD_UNIT_SERIAL        0x00000004
-- 
2.40.0

And it didn't fix the double add issue, additionally i was able to 
trigger a double free one time:

[  291.075508] list_del corruption. next->prev should be 
ffff8881cd240000, but was ffff88810085a000. (next=ffff88826da7a000)
[  291.075971] ------------[ cut here ]------------
[  291.075972] Kernel BUG at __list_del_entry_valid+0xc3/0xd0 [verbose 
debug info unavailable]
[  291.076304] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[  291.076500] CPU: 7 PID: 19557 Comm: node Not tainted 
6.4.0-rc2hocus-00246-gd3f704310cc7-dirty #7
[  291.076821] RIP: 0010:__list_del_entry_valid+0xc3/0xd0
[  291.077014] Code: 0b 48 89 fe 48 89 c2 48 c7 c7 e8 45 1a 82 e8 a4 73 
c1 ff 0f 0b 48 89 d1 48 c7 c7 38 46 1a 82 48 89 f2 48 89 c6 e8 8d 73 c1 
ff <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
[  291.077698] RSP: 0018:ffffc90001f13d70 EFLAGS: 00010246
[  291.077905] RAX: 000000000000006d RBX: ffff8881cd240018 RCX: 
0000000000000000
[  291.078172] RDX: 0000000000000000 RSI: ffffffff82142371 RDI: 
00000000ffffffff
[  291.078432] RBP: ffffc90001f13d70 R08: 0000000000000000 R09: 
ffffc90001f13be8
[  291.078692] R10: 0000000000000003 R11: ffffffff82b52c48 R12: 
ffff8881cd240000
[  291.078961] R13: ffff8881f8e09030 R14: 0000000000000000 R15: 
0000000000000000
[  291.079231] FS:  00007fbf93fff6c0(0000) GS:ffff88841fdc0000(0000) 
knlGS:0000000000000000
[  291.079532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  291.079746] CR2: 00007fd57b825020 CR3: 000000022af4a000 CR4: 
00000000003506a0
[  291.080009] Call Trace:
[  291.080103]  <TASK>
[  291.080187]  tcmu_destroy_device+0x51/0x120
[  291.080362]  ? config_item_cleanup+0x2d/0xa0
[  291.080537]  target_free_device+0x4d/0x100
[  291.080699]  target_core_dev_release+0x14/0x20
[  291.080869]  config_item_cleanup+0x53/0xa0
[  291.081023]  config_item_put+0x34/0x50
[  291.081181]  configfs_rmdir+0x246/0x370
[  291.081328]  ? may_delete+0x113/0x1d0
[  291.081469]  vfs_rmdir+0x7c/0x1e0
[  291.081593]  do_rmdir+0x12e/0x170
[  291.081719]  __x64_sys_rmdir+0x41/0x60
[  291.081862]  do_syscall_64+0x3e/0x90
[  291.082011]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  291.082210] RIP: 0033:0x7fbfaad52907
[  291.082349] Code: 73 01 c3 48 8b 0d f9 84 0d 00 f7 d8 64 89 01 48 83 
c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 54 00 00 00 0f 
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 84 0d 00 f7 d8 64 89 01 48
[  291.083021] RSP: 002b:00007fbf93ffed18 EFLAGS: 00000297 ORIG_RAX: 
0000000000000054
[  291.083298] RAX: ffffffffffffffda RBX: 000000000679b7e0 RCX: 
00007fbfaad52907
[  291.083563] RDX: 0000000000000001 RSI: 0000000000000081 RDI: 
00007fbf91eb1460
[  291.083828] RBP: 00007fbf93ffee20 R08: 0000000000000000 R09: 
00000000ffffffff
[  291.084099] R10: 0000000000000000 R11: 0000000000000297 R12: 
00007fbf93fff5c0
[  291.084367] R13: 000000000679b7c8 R14: 00007ffd460fbb80 R15: 
000000000679b7c8
[  291.084622]  </TASK>
[  291.084703] ---[ end trace 0000000000000000 ]---
[  291.084832] RIP: 0010:__list_del_entry_valid+0xc3/0xd0
[  291.084975] Code: 0b 48 89 fe 48 89 c2 48 c7 c7 e8 45 1a 82 e8 a4 73 
c1 ff 0f 0b 48 89 d1 48 c7 c7 38 46 1a 82 48 89 f2 48 89 c6 e8 8d 73 c1 
ff <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90
[  291.085449] RSP: 0018:ffffc90001f13d70 EFLAGS: 00010246
[  291.085585] RAX: 000000000000006d RBX: ffff8881cd240018 RCX: 
0000000000000000
[  291.085774] RDX: 0000000000000000 RSI: ffffffff82142371 RDI: 
00000000ffffffff
[  291.085961] RBP: ffffc90001f13d70 R08: 0000000000000000 R09: 
ffffc90001f13be8
[  291.086163] R10: 0000000000000003 R11: ffffffff82b52c48 R12: 
ffff8881cd240000
[  291.086361] R13: ffff8881f8e09030 R14: 0000000000000000 R15: 
0000000000000000
[  291.086552] FS:  00007fbf93fff6c0(0000) GS:ffff88841fdc0000(0000) 
knlGS:0000000000000000
[  291.086766] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  291.086920] CR2: 00007fd57b825020 CR3: 000000022af4a000 CR4: 
00000000003506a0

I'm currently trying to hunt down the root cause behind the data 
corruption. For context those bugs are triggered by my script which is 
concurrently adding/deleting a lot of TCMU devices and exposing them to 
the system using TCM_LOOP.

>
> BR,
>   Dmitry
Best regards,
Grzegorz Uriasz

Powered by blists - more mailing lists