On Thu, 2017-07-27 at 13:56 -0700, Justin Maggard wrote:
> I ran across a GPF after disabling demo-mode on an iSCSI target. The
> backtrace pointed to target_put_nacl(), which was called from
> transport_free_session().
Can we see actual stack trace, and kernel version please..?
> A look at that function shows a pretty clear problem:
>
> 543 mutex_unlock(&se_tpg->acl_node_mutex);
> 544
> 545 if (se_nacl->dynamic_stop)
> 546 target_put_nacl(se_nacl);
> 547
> 548 target_put_nacl(se_nacl);
> 549 }
>
> So we make two calls to target_put_nacl() when se_nacl->dynamic_stop is true.
Correct.
> This got introduced by 01d4d6735589 (target: Fix multi-session dynamic
> se_node_acl double free OOPs). It looks to me like we can just
> eliminate that first call, but I could easily be missing something.
The double put in transport_free_session() when se_nacl->dynamic_stop is
set is expected behavior, because when a dynamic se_node_acl is
generated it takes the initial reference in target_alloc_node_acl(), and
then a second reference in core_tpg_check_initiator_node_acl():
/*
* When allocating a dynamically generated node_acl, go ahead
* and take the extra kref now before returning to the fabric
* driver caller.
*
* Note this reference will be released at session shutdown
* time within transport_free_session() code.
*/
kref_get(&acl->acl_kref);
acl->dynamic_node_acl = 1;
So for se_node_acl->dynamic_node_acl = 0 in transport_free_session(),
it's just dropping the se_node_acl->nacl_kref obtained by
core_tpg_get_initiator_node_acl(). Eg, it's not dropping the last
reference..
But during the se_node_acl->dynamic_node_acl = 1 free special case,
transport_free_session() is responsible for both target_put_nacl() calls
to actually free se_node_acl.
That is, normally for non dynamic ACLs, it's the configfs callback for
se_node_acl->acl_group rmdir into core_tpg_del_initiator_node_acl() that
drops the reference:
target_put_nacl(acl);
/*
* Wait for last target_put_nacl() to complete in target_complete_nacl()
* for active fabric session transport_deregister_session() callbacks.
*/
wait_for_completion(&acl->acl_free_comp);
But since se_node_acl->dynamic_node_acl = 1 doesn't have a corresponding
configfs entry for user-space to remove, it's left up to
transport_free_session() to set se_node_acl->dynamic_stop and drop both
references to free se_node_acl.
--
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
