Hi Sebastian,
On Mon, 2013-11-04 at 19:26 +0100, Sebastian Andrzej Siewior wrote:
> I added recently debugobject support for kref and testing how well it
> works. That means code like this:
>
> | p = kmalloc();
> | kref_init(&p->kref);
> | kfree(p);
>
> is considered as invalid because
> |kref_put(&p->kref, cleanup)
> was expected.
>
> With this change I created ramdisk+loopback target. Then I started
> removing it and run into
>
> |/sys/kernel/config/target/loopback/naa.6001405c3214b06a# rmdir tpgt_1
> | ------------[ cut here ]------------
> | WARNING: CPU: 0 PID: 2038 at lib/debugobjects.c:260 debug_print_object+0x94/0xc4()
> | ODEBUG: free active (active state 0) object type: kref hint: core_tpg_check_initiator_node_acl+0x5c/0x220 [target_core_mod]
> | CPU: 0 PID: 2038 Comm: rmdir Not tainted 3.12.0+ #452
> | [<c0014d38>] (unwind_backtrace+0x0/0xf4) from [<c001249c>] (show_stack+0x14/0x1c)
> | [<c001249c>] (show_stack+0x14/0x1c) from [<c0037474>] (warn_slowpath_common+0x64/0x84)
> | [<c0037474>] (warn_slowpath_common+0x64/0x84) from [<c0037528>] (warn_slowpath_fmt+0x30/0x40)
> | [<c0037528>] (warn_slowpath_fmt+0x30/0x40) from [<c022ea9c>] (debug_print_object+0x94/0xc4)
> | [<c022ea9c>] (debug_print_object+0x94/0xc4) from [<c022f3fc>] (__debug_check_no_obj_freed+0x1bc/0x228)
> | [<c022f3fc>] (__debug_check_no_obj_freed+0x1bc/0x228) from [<c00f25b8>] (kfree+0xf8/0x228)
> | [<c00f25b8>] (kfree+0xf8/0x228) from [<bf172634>] (transport_deregister_session+0xfc/0x13c [target_core_mod])
> | [<bf172634>] (transport_deregister_session+0xfc/0x13c [target_core_mod]) from [<bf1bf7f0>] (tcm_loop_drop_nexus+0x3c/0x6c [tcm
> | [<bf1bf7f0>] (tcm_loop_drop_nexus+0x3c/0x6c [tcm_loop]) from [<bf1c002c>] (tcm_loop_drop_naa_tpg+0x18/0x34 [tcm_loop])
> | [<bf1c002c>] (tcm_loop_drop_naa_tpg+0x18/0x34 [tcm_loop]) from [<bf163a70>] (target_fabric_tpg_release+0x24/0x30 [target_core_
> | [<bf163a70>] (target_fabric_tpg_release+0x24/0x30 [target_core_mod]) from [<c015c93c>] (config_item_release+0x5c/0x80)
> | [<c015c93c>] (config_item_release+0x5c/0x80) from [<c015b13c>] (configfs_rmdir+0x254/0x2e4)
> | [<c015b13c>] (configfs_rmdir+0x254/0x2e4) from [<c0105b48>] (vfs_rmdir+0x9c/0x10c)
> | [<c0105b48>] (vfs_rmdir+0x9c/0x10c) from [<c0107ce0>] (do_rmdir+0x14c/0x174)
> | [<c0107ce0>] (do_rmdir+0x14c/0x174) from [<c000e680>] (ret_fast_syscall+0x0/0x48)
> | ---[ end trace 8cbc7c644521ad81 ]---
>
> kref_init() is from core_tpg_check_initiator_node_acl()
> |kref_init(&acl->acl_kref)
>
> I see in __transport_register_session() a get and in target_put_nacl() a
> put. As it can be seen in transport_deregister_session() the memory
> behind that kref is removed before the release function has been called.
>
> Can somebody say if this is a bug or a false positive?
So for the tcm_loop case (eg: using demo-mode se_node_acls) I believe
your hitting the !se_tfo->tpg_check_demo_mode_cache() section in
transport_deregister_session()
This is because the call to target_put_nacl() is used to complete
se_node_acl->acl_free_comp for node acls that may be in the process of
deletion via core_tpg_del_initiator_node_acl(), and when the se_node_acl
is released directly from transport_deregister_session(), the kref_put()
is currently skipped because no matching configfs entry exists to wait
on..
Note that tcm_loop is a special case because no matching
se_node_acl->acl_group will ever exist. However, I expect you'll hit
this same case for other fabrics as well..
How about the following patch..?
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 81e945e..f0b905c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -462,6 +462,7 @@ void transport_deregister_session(struct se_session *se_sess)
spin_unlock_irqrestore(&se_tpg->acl_node_lock, flags);
core_tpg_wait_for_nacl_pr_ref(se_nacl);
core_free_device_list_for_node(se_nacl, se_tpg);
+ target_put_nacl(se_nacl);
se_tfo->tpg_release_fabric_acl(se_tpg, se_nacl);
comp_nacl = false;
>
> I've hit the second one at transport_init_session() / ->sess_kref and
> asking basically the same question :)
>
Ok, this one is exclusively used by core_tpg_del_initiator_node_acl()
for reference counting a forced se_session close, and is only kref_put()
when the se_session is active and forced to shutdown while
se_node_acl->acl_group is being explicitly deleted.
A patch for this one is not as clear given that target_put_session()
should only be called for this special case when se_node_acl->acl_group
is being removed, and not from normal transport_deregister_session()
context.
--nab
--
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
