[PATCH] mpt2sas: prevent double free on error path

Jörn Engel

author

2013-01-23 21:20 UTC #0 lore raw

PATCH 1/1 mpt2sas: prevent double free on error path

I noticed this one when list_del was called with poisoned list pointers, but the real problem is a double-free (and a use-after-free just before that).

Both _scsih_probe_boot_devices() and _scsih_sas_device_add() put the sas_device onto a list, thereby giving up control. Next they call mpt2sas_transport_port_add() and will list_del and free the object on errors. If some other function already did the list_del and free, it will happen again.

This patch adds reference counting to prevent the double free. One reference count goes to the caller of mpt2sas_transport_port_add(), the second to the list. Whoever removes the object from the list gets to drop one reference count. _scsih_probe_boot_devices() and _scsih_sas_device_add() get a second reference count to ensure the object is not freed while they are still accessing it.

To prevent the double list_del(), I changed the code to list_del_init() and added a list_empty() check before that. Since the list_empty/list_del_init is always called under a lock, this should be safe.

I hate the complexity this patch adds, but see no alternative to it.

mpt2sas0: failure at drivers/scsi/mpt2sas/mpt2sas_transport.c:708/mpt2sas_transport_port_add()!

general protection fault: 0000 [#1] SMP
CPU 9
Pid: 3097, comm: kworker/u:11 Tainted: G        W  O 3.6.10+ #31392.trunk    /0JP31P
RIP: 0010:[<ffffffffa0309744>]  [<ffffffffa0309744>] _scsih_sas_device_remove+0x54/0x90 [mpt2sas]
RSP: 0018:ffff881fed4d7ab0  EFLAGS: 00010046
RAX: dead000000200200 RBX: ffff881ff6a5cd88 RCX: 00000000000010e8
RDX: ffff881ff7dab800 RSI: ffff881ff7daba00 RDI: dead000000100100
RBP: ffff881fed4d7ad0 R08: dead000000200200 R09: ffff880fff802200
R10: ffffffffa0317980 R11: 0000000000000000 R12: ffff881ff7daba00
R13: 0000000000000286 R14: 500605ba006c9d09 R15: ffff881ff7daba00
FS:  0000000000000000(0000) GS:ffff88203fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f8ac89ec458 CR3: 0000001ff4c5c000 CR4: 00000000000407e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/u:11 (pid: 3097, threadinfo ffff881fed4d6000, task ffff881402f3d9c0)
Stack:
 0000000000000401 ffff881ff6a5c6b0 0000000000000401 0000000000000016
 ffff881fed4d7bb0 ffffffffa030f93e 0000000000000000 ffff881ff6a5cd88
 0012000e0f000008 006c9d090002000b 00180009500605ba 0000040100000016
Call Trace:
 [<ffffffffa030f93e>] _scsih_add_device.clone.32+0x2fe/0x420 [mpt2sas]
 [<ffffffffa03126e5>] _scsih_sas_topology_change_event.clone.38+0x285/0x620 [mpt2sas]
 [<ffffffff81078c90>] ? load_balance+0x100/0x7a0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffffa0312d8a>] _firmware_event_work+0x30a/0xfc0 [mpt2sas]
 [<ffffffff810015cc>] ? __switch_to+0x14c/0x410
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffff8105bf40>] process_one_work+0x140/0x500
 [<ffffffff8105d354>] worker_thread+0x194/0x510
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffff8105d1c0>] ? manage_workers+0x320/0x320
 [<ffffffff8106282e>] kthread+0x9e/0xb0
 [<ffffffff815bef44>] kernel_thread_helper+0x4/0x10
 [<ffffffff815b5e5d>] ? retint_restore_args+0x13/0x13
 [<ffffffff81062790>] ? kthread_freezable_should_stop+0x70/0x70
 [<ffffffff815bef40>] ? gs_change+0x13/0x13

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/mpt2sas/mpt2sas_base.h  |    1 +
 drivers/scsi/mpt2sas/mpt2sas_scsih.c |   55 ++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 9 deletions(-)

drivers/scsi/mpt2sas/mpt2sas_base.h +1 -0

index 543d8d6..ceb7d41 100644
@@ -367,6 +367,7 @@ struct _sas_device {
 	u16	slot;
 	u8	phy;
 	u8	responding;
+	struct kref kref;
 };
 
 /**

drivers/scsi/mpt2sas/mpt2sas_scsih.c +46 -9

129 lines not on this page · raw message

Signature · 1 line

1.7.10.4

Bjørn Mork

2013-01-24 07:52 UTC #1 lore raw

Quoted 34 lines · Jörn Engel

drivers/scsi/mpt2sas/mpt2sas_scsih.c +13 -1

index c6bdc92..43b3a98 100644
@@ -570,6 +570,18 @@ _scsih_sas_device_find_by_handle(struct MPT2SAS_ADAPTER *ioc, u16 handle)
 	return NULL;
 }
 
+static void free_sas_device(struct kref *kref)
+{
+	struct _sas_device *sas_device = container_of(kref, struct _sas_device,
+			kref);
+	kfree(sas_device);
+}
+
+static void put_sas_device(struct _sas_device *sas_device)
+{
+	kref_put(&sas_device->kref, free_sas_device);
+}
+
 /**
  * _scsih_sas_device_remove - remove sas_device from list.
  * @ioc: per adapter object

@@ -583,14 +595,19 @@ _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc,
     struct _sas_device *sas_device)
 {
 	unsigned long flags;
+	int was_on_list = 0;
 
 	if (!sas_device)
 		return;
 
 	spin_lock_irqsave(&ioc->sas_device_lock, flags);
-	list_del(&sas_device->list);

drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc, · line 612

Quoted 10 lines

drivers/scsi/mpt2sas/mpt2sas_scsih.c +6 -1

-	kfree(sas_device);
+	if (!list_empty(&sas_device->list)) {
+		list_del_init(&sas_device->list);
+		was_on_list = 1;
+	}
 	spin_unlock_irqrestore(&ioc->sas_device_lock, flags);
+	if (was_on_list)
+		put_sas_device(sas_device);
 }

How about the copy of this code in drivers/scsi/mpt3sas/mpt3sas_scsih.c? Is that safe, or does it need fixing as well?

Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2013-01-25 17:12 UTC #2 lore raw

Quoted 3 lines · Bjørn Mork

How about the copy of this code in drivers/scsi/mpt3sas/mpt3sas_scsih.c? Is that safe, or does it need fixing as well?

Well spotted, that appears to suffer from the same ailment. Will cook up a second patch for that.

Are there any other copies of this code around?

Jörn

--
The object-oriented version of 'Spaghetti code' is, of course, 'Lasagna code'. (Too many layers).
-- Roberto Waltman.

Jörn Engel

author

2013-01-25 19:22 UTC #3 lore raw

PATCH v2 1/1 mpt2sas/mpt3sas: prevent double free on error path

Changes since v1:
- Also fixes mpt3sas, thanks to Bjørn Mork.
- Adds a missing put_sas_device to _scsih_probe_boot_devices
- Added a newline
- Moved a kref_get outside the spinlock

I noticed this one when list_del was called with poisoned list pointers, but the real problem is a double-free (and a use-after-free just before that).

Both _scsih_probe_boot_devices() and _scsih_sas_device_add() put the sas_device onto a list, thereby giving up control. Next they call mpt2sas_transport_port_add() and will list_del and free the object on errors. If some other function already did the list_del and free, it will happen again.

This patch adds reference counting to prevent the double free. One reference count goes to the caller of mpt2sas_transport_port_add(), the second to the list. Whoever removes the object from the list gets to drop one reference count. _scsih_probe_boot_devices() and _scsih_sas_device_add() get a second reference count to ensure the object is not freed while they are still accessing it.

To prevent the double list_del(), I changed the code to list_del_init() and added a list_empty() check before that. Since the list_empty/list_del_init is always called under a lock, this should be safe.

I hate the complexity this patch adds, but see no alternative to it.

mpt2sas0: failure at drivers/scsi/mpt2sas/mpt2sas_transport.c:708/mpt2sas_transport_port_add()!

general protection fault: 0000 [#1] SMP
CPU 9
Pid: 3097, comm: kworker/u:11 Tainted: G        W  O 3.6.10+ #31392.trunk    /0JP31P
RIP: 0010:[<ffffffffa0309744>]  [<ffffffffa0309744>] _scsih_sas_device_remove+0x54/0x90 [mpt2sas]
RSP: 0018:ffff881fed4d7ab0  EFLAGS: 00010046
RAX: dead000000200200 RBX: ffff881ff6a5cd88 RCX: 00000000000010e8
RDX: ffff881ff7dab800 RSI: ffff881ff7daba00 RDI: dead000000100100
RBP: ffff881fed4d7ad0 R08: dead000000200200 R09: ffff880fff802200
R10: ffffffffa0317980 R11: 0000000000000000 R12: ffff881ff7daba00
R13: 0000000000000286 R14: 500605ba006c9d09 R15: ffff881ff7daba00
FS:  0000000000000000(0000) GS:ffff88203fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f8ac89ec458 CR3: 0000001ff4c5c000 CR4: 00000000000407e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/u:11 (pid: 3097, threadinfo ffff881fed4d6000, task ffff881402f3d9c0)
Stack:
 0000000000000401 ffff881ff6a5c6b0 0000000000000401 0000000000000016
 ffff881fed4d7bb0 ffffffffa030f93e 0000000000000000 ffff881ff6a5cd88
 0012000e0f000008 006c9d090002000b 00180009500605ba 0000040100000016
Call Trace:
 [<ffffffffa030f93e>] _scsih_add_device.clone.32+0x2fe/0x420 [mpt2sas]
 [<ffffffffa03126e5>] _scsih_sas_topology_change_event.clone.38+0x285/0x620 [mpt2sas]
 [<ffffffff81078c90>] ? load_balance+0x100/0x7a0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffffa0312d8a>] _firmware_event_work+0x30a/0xfc0 [mpt2sas]
 [<ffffffff810015cc>] ? __switch_to+0x14c/0x410
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffff8105bf40>] process_one_work+0x140/0x500
 [<ffffffff8105d354>] worker_thread+0x194/0x510
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffff8105d1c0>] ? manage_workers+0x320/0x320
 [<ffffffff8106282e>] kthread+0x9e/0xb0
 [<ffffffff815bef44>] kernel_thread_helper+0x4/0x10
 [<ffffffff815b5e5d>] ? retint_restore_args+0x13/0x13
 [<ffffffff81062790>] ? kthread_freezable_should_stop+0x70/0x70
 [<ffffffff815bef40>] ? gs_change+0x13/0x13

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/mpt2sas/mpt2sas_base.h  |    1 +
 drivers/scsi/mpt2sas/mpt2sas_scsih.c |   57 ++++++++++++++++++++++++++++------
 drivers/scsi/mpt3sas/mpt3sas_base.h  |    1 +
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |   57 ++++++++++++++++++++++++++++------
 4 files changed, 98 insertions(+), 18 deletions(-)

drivers/scsi/mpt2sas/mpt2sas_base.h +1 -0

index 543d8d6..ceb7d41 100644
@@ -367,6 +367,7 @@ struct _sas_device {
 	u16	slot;
 	u8	phy;
 	u8	responding;
+	struct kref kref;
 };
 
 /**

drivers/scsi/mpt2sas/mpt2sas_scsih.c +48 -9

137 lines not on this page · raw message

drivers/scsi/mpt3sas/mpt3sas_base.h +1 -0

index 994656c..fff7fe7 100644
@@ -293,6 +293,7 @@ struct _sas_device {
 	u8	phy;
 	u8	responding;
 	u8	fast_path;
+	struct kref kref;
 };
 
 /**

drivers/scsi/mpt3sas/mpt3sas_scsih.c +48 -9

137 lines not on this page · raw message

Signature · 6 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jörn Engel

author

2013-01-25 20:42 UTC #4 lore raw

PATCH v3 1/1 mpt2sas/mpt3sas: prevent double free on error path

Changes since v1:
- Adds another missing put_sas_device to _scsih_probe_boot_devices, proving the need for more coffee before sending out patches.

I noticed this one when list_del was called with poisoned list pointers, but the real problem is a double-free (and a use-after-free just before that).

Both _scsih_probe_boot_devices() and _scsih_sas_device_add() put the sas_device onto a list, thereby giving up control. Next they call mpt2sas_transport_port_add() and will list_del and free the object on errors. If some other function already did the list_del and free, it will happen again.

This patch adds reference counting to prevent the double free. One reference count goes to the caller of mpt2sas_transport_port_add(), the second to the list. Whoever removes the object from the list gets to drop one reference count. _scsih_probe_boot_devices() and _scsih_sas_device_add() get a second reference count to ensure the object is not freed while they are still accessing it.

To prevent the double list_del(), I changed the code to list_del_init() and added a list_empty() check before that. Since the list_empty/list_del_init is always called under a lock, this should be safe.

I hate the complexity this patch adds, but see no alternative to it.

mpt2sas0: failure at drivers/scsi/mpt2sas/mpt2sas_transport.c:708/mpt2sas_transport_port_add()!

general protection fault: 0000 [#1] SMP
CPU 9
Pid: 3097, comm: kworker/u:11 Tainted: G        W  O 3.6.10+ #31392.trunk    /0JP31P
RIP: 0010:[<ffffffffa0309744>]  [<ffffffffa0309744>] _scsih_sas_device_remove+0x54/0x90 [mpt2sas]
RSP: 0018:ffff881fed4d7ab0  EFLAGS: 00010046
RAX: dead000000200200 RBX: ffff881ff6a5cd88 RCX: 00000000000010e8
RDX: ffff881ff7dab800 RSI: ffff881ff7daba00 RDI: dead000000100100
RBP: ffff881fed4d7ad0 R08: dead000000200200 R09: ffff880fff802200
R10: ffffffffa0317980 R11: 0000000000000000 R12: ffff881ff7daba00
R13: 0000000000000286 R14: 500605ba006c9d09 R15: ffff881ff7daba00
FS:  0000000000000000(0000) GS:ffff88203fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f8ac89ec458 CR3: 0000001ff4c5c000 CR4: 00000000000407e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kworker/u:11 (pid: 3097, threadinfo ffff881fed4d6000, task ffff881402f3d9c0)
Stack:
 0000000000000401 ffff881ff6a5c6b0 0000000000000401 0000000000000016
 ffff881fed4d7bb0 ffffffffa030f93e 0000000000000000 ffff881ff6a5cd88
 0012000e0f000008 006c9d090002000b 00180009500605ba 0000040100000016
Call Trace:
 [<ffffffffa030f93e>] _scsih_add_device.clone.32+0x2fe/0x420 [mpt2sas]
 [<ffffffffa03126e5>] _scsih_sas_topology_change_event.clone.38+0x285/0x620 [mpt2sas]
 [<ffffffff81078c90>] ? load_balance+0x100/0x7a0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffffa0312d8a>] _firmware_event_work+0x30a/0xfc0 [mpt2sas]
 [<ffffffff810015cc>] ? __switch_to+0x14c/0x410
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffffa0312a80>] ? _scsih_sas_topology_change_event.clone.38+0x620/0x620 [mpt2sas]
 [<ffffffff8105bf40>] process_one_work+0x140/0x500
 [<ffffffff8105d354>] worker_thread+0x194/0x510
 [<ffffffff8106dfdb>] ? finish_task_switch+0x4b/0xf0
 [<ffffffff8105d1c0>] ? manage_workers+0x320/0x320
 [<ffffffff8106282e>] kthread+0x9e/0xb0
 [<ffffffff815bef44>] kernel_thread_helper+0x4/0x10
 [<ffffffff815b5e5d>] ? retint_restore_args+0x13/0x13
 [<ffffffff81062790>] ? kthread_freezable_should_stop+0x70/0x70
 [<ffffffff815bef40>] ? gs_change+0x13/0x13

Signed-off-by Joern Engel <[email protected]>

---
 drivers/scsi/mpt2sas/mpt2sas_base.h  |    1 +
 drivers/scsi/mpt2sas/mpt2sas_scsih.c |   57 ++++++++++++++++++++++++++++------
 drivers/scsi/mpt3sas/mpt3sas_base.h  |    1 +
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |   57 ++++++++++++++++++++++++++++------
 4 files changed, 98 insertions(+), 18 deletions(-)

drivers/scsi/mpt2sas/mpt2sas_base.h +1 -0

index 543d8d6..ceb7d41 100644
@@ -367,6 +367,7 @@ struct _sas_device {
 	u16	slot;
 	u8	phy;
 	u8	responding;
+	struct kref kref;
 };
 
 /**

drivers/scsi/mpt2sas/mpt2sas_scsih.c +48 -9

137 lines not on this page · raw message

drivers/scsi/mpt3sas/mpt3sas_base.h +1 -0

index 994656c..fff7fe7 100644
@@ -293,6 +293,7 @@ struct _sas_device {
 	u8	phy;
 	u8	responding;
 	u8	fast_path;
+	struct kref kref;
 };
 
 /**

drivers/scsi/mpt3sas/mpt3sas_scsih.c +48 -9

137 lines not on this page · raw message

Signature · 1 line

1.7.10.4

[email protected]

2013-01-26 03:52 UTC #5 lore raw

mpt2sas: device_blocked question

Hi,
we encounter a problem that a disk is blocked forever, the key log is:

mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0) sd 2:0:1:0: device_blocked, handle(0x000a)

looking into the driver code, it seems that the device is blocked by function
_scsih_block_io_to_children_attached_directly() in mpt2sas/mpt2sas_scsih.c
with reason_code == MPI2_EVENT_SAS_TOPO_RC_DELAY_NOT_RESPONDING,
what does this reason_code mean please? when should it be returned? and any way to handle it?

Best Regards,
spren
2013-01-26

Reddy, Sreekanth

2013-01-30 06:52 UTC #6 lore raw

RE: mpt2sas: device_blocked question

Hi Spren,

Reason code MPI2_EVENT_SAS_TOPO_RC_DELAY_NOT_RESPONDING mean that "A target device has stopped responding or is missing but the ReportDeviceMissingDelay timer has not expired for this device".

Can you please send me the some more information i.e.

1. For which operation/TestCase this issue has occurred?
2. Which Driver & Firmware version you are using?
3. Can you also please send me the complete log.

Thanks,
Sreekanth.

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: Saturday, January 26, 2013 9:22 AM
To: JörnEngel; BjørnMork; Nandigama, Nagalakshmi; Reddy, Sreekanth; Support; James E.J. Bottomley; DL-MPT Fusion Linux
Cc: [email protected]
Subject: mpt2sas: device_blocked question

Hi,
we encounter a problem that a disk is blocked forever, the key log is:

mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0)
mpt2sas0: TEST_UNIT_READY: handle(0x000a), lun(0)
mpt2sas0: Spinning up disk.... handle(0x000a), lun(0) sd 2:0:1:0: device_blocked, handle(0x000a)

looking into the driver code, it seems that the device is blocked by function
_scsih_block_io_to_children_attached_directly() in mpt2sas/mpt2sas_scsih.c with reason_code == MPI2_EVENT_SAS_TOPO_RC_DELAY_NOT_RESPONDING,
what does this reason_code mean please? when should it be returned? and any way to handle it?

Best Regards,
spren
2013-01-26

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html