On 01/06/2016 11:11 PM, Nicholas A. Bellinger wrote:
On Tue, 2016-01-05 at 14:48 +0100, Bart Van Assche wrote:
Target drivers like ib_srpt can call transport_deregister_session()
while core_tpg_del_initiator_node_acl() is processing sess_acl_list.
Avoid that this scenario triggers a use-after-free by postponing
freeing a session object until core_tpg_del_initiator_node_acl() has
finished accessing that session object. Keep the se_tpg and
fabric_sess_ptr member variables as long as the session object
exists.
This patch fixes the following crash:
general protection fault: 0000 [#1] SMP
CPU: 5 PID: 15050 Comm: rmdir Tainted: G E 4.4.0-rc7+ #1
Hardware name: Dell Inc. PowerEdge R430/03XKDV, BIOS 1.0.2 11/17/2014
task: ffff880428349f40 ti: ffff880426584000 task.ti: ffff880426584000
RIP: 0010:[<ffffffff8126ef7d>] [<ffffffff8126ef7d>] __list_del_entry+0x2d/0xd0
RSP: 0018:ffff880426587cd8 EFLAGS: 00010a83
RAX: 6b6b6b6b6b6b6b6b RBX: ffff8804273687c0 RCX: dead000000000200
RDX: 6b6b6b6b6b6b6b6b RSI: ffff88042834ab20 RDI: ffff8804273687c0
RBP: ffff880426587ce8 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: ffff880426587d48
R13: ffff88041fc66a30 R14: 6b6b6b6b6b6b6b2b R15: ffff880427368780
FS: 00007f8b4ad54700(0000) GS:ffff88046e4a0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fec7ed44000 CR3: 0000000075a30000 CR4: 00000000001406e0
Stack:
ffff88041fc66a30 ffff880427368780 ffff880426587d08 ffffffff8126f031
ffff880426587d08 ffff880422106618 ffff880426587d98 ffffffffa04f8390
ffff88045e5271b8 0000000000000000 ffff880426587d48 ffff880400000001
Call Trace:
[<ffffffff8126f031>] list_del+0x11/0x40
[<ffffffffa04f8390>] core_tpg_del_initiator_node_acl+0x110/0x200 [target_core_mod]
[<ffffffffa04ebcaf>] target_fabric_nacl_base_release+0x4f/0x60 [target_core_mod]
[<ffffffffa0482bef>] config_item_release+0x6f/0xc0 [configfs]
[<ffffffffa0482c5b>] config_item_put+0x1b/0x20 [configfs]
[<ffffffffa0481f88>] configfs_rmdir+0x1e8/0x2e0 [configfs]
[<ffffffff81183461>] vfs_rmdir+0x81/0x120
[<ffffffff81187246>] do_rmdir+0x146/0x190
[<ffffffff811872d1>] SyS_rmdir+0x11/0x20
[<ffffffff8151c857>] entry_SYSCALL_64_fastpath+0x12/0x6f
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Andy Grover <agrover@xxxxxxxxxx>
Cc: Sagi Grimberg <sagig@xxxxxxxxxxxx>
---
drivers/target/target_core_transport.c | 27 ++++++++++++++++++++-------
include/target/target_core_base.h | 1 +
2 files changed, 21 insertions(+), 7 deletions(-)
This is unnecessary. The real issue is that ib_srpt is still using it's
own internal srpt_lookup_acl() hackery that ignores shutdown logic in
core_tpg_del_initiator_node_acl() that would prevent se_node_acl from
being located in the first place.
It needs to be converted to use core_tpg_check_initiator_node_acl()
following what everybody else does to handle explicit se_node_acl
deletion, and not add some random logic to avoid a proper conversion.
Hello Nic,
I agree that the ib_srpt driver should use
core_tpg_del_initiator_node_acl() and also that srpt_lookup_acl() should
be removed. I will implement that change before I retest and resubmit
this patch series. But I do not agree that that change would make the
above patch superfluous. In the test I ran demo mode was disabled. This
means that the only effect of
core_tpg_check_initiator_node_acl() in my test would be that it looks up
an ACL in the ACL list. That's exactly what srpt_lookup_acl() does today.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
