On Thu, 2016-01-07 at 11:43 +0100, Bart Van Assche wrote: > 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. Adding a second kref to se_node_acl still seems rather pointless to me. We've already got se_node_acl->acl_kref, and se_node_acl->acl_group configfs shutdown in core_tpg_del_initiator_node_acl() that uses a struct completion to wait until all kref_put() references have dropped.. Why do we need to add a second one for something special about ib_srpt..? > In the test I ran demo mode was disabled. Yes, because ib_srpt has never supported demo_mode. Wouldn't it be nice if someone sent a patch to address that..? > 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. > ... In any event, I've gone ahead and dropped the legacy port_acl_lock + port_acl_list internal lookup non-sense. -- 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
