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
