| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 07 Feb 2017 19:46:52 -0800
From: "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To: Christoph Hellwig <hch@...radead.org>
Cc: target-devel <target-devel@...r.kernel.org>,
linux-scsi <linux-scsi@...r.kernel.org>,
lkml <linux-kernel@...r.kernel.org>,
Rob Millner <rlm@...erainc.com>
Subject: Re: [PATCH 4/5] target: Fix multi-session dynamic se_node_acl
double free OOPs
On Tue, 2017-02-07 at 15:12 -0800, Christoph Hellwig wrote:
> And the real patch after compile fixing it is here of course:
>
Getting rid of the extra se_node_acl->acl_free_comp seems to make sense
here..
The only potential issue is if returning from configfs syscall
rmdir /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/acls/$INITIATOR/
before se_node_acl memory is released has implications when the
underlying ../$WWN/$TPGT/ is removed immediately after.
In any event, I'll verify this patch with the original test case and use
it instead if everything looks OK.
> diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> index 9ab7090f7c83..96c38f30069d 100644
> --- a/drivers/target/target_core_internal.h
> +++ b/drivers/target/target_core_internal.h
> @@ -152,6 +152,7 @@ void target_qf_do_work(struct work_struct *work);
> bool target_check_wce(struct se_device *dev);
> bool target_check_fua(struct se_device *dev);
> void __target_execute_cmd(struct se_cmd *, bool);
> +void target_put_nacl(struct se_node_acl *);
>
> /* target_core_stat.c */
> void target_stat_setup_dev_default_groups(struct se_device *);
> diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> index d99752c6cd60..08738c87e5b0 100644
> --- a/drivers/target/target_core_tpg.c
> +++ b/drivers/target/target_core_tpg.c
> @@ -197,7 +197,6 @@ static struct se_node_acl *target_alloc_node_acl(struct se_portal_group *tpg,
> INIT_LIST_HEAD(&acl->acl_sess_list);
> INIT_HLIST_HEAD(&acl->lun_entry_hlist);
> kref_init(&acl->acl_kref);
> - init_completion(&acl->acl_free_comp);
> spin_lock_init(&acl->nacl_sess_lock);
> mutex_init(&acl->lun_entry_mutex);
> atomic_set(&acl->acl_pr_ref_count, 0);
> @@ -370,21 +369,6 @@ void core_tpg_del_initiator_node_acl(struct se_node_acl *acl)
> target_shutdown_sessions(acl);
>
> 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);
> -
> - core_tpg_wait_for_nacl_pr_ref(acl);
> - core_free_device_list_for_node(acl, tpg);
> -
> - pr_debug("%s_TPG[%hu] - Deleted ACL with TCQ Depth: %d for %s"
> - " Initiator Node: %s\n", tpg->se_tpg_tfo->get_fabric_name(),
> - tpg->se_tpg_tfo->tpg_get_tag(tpg), acl->queue_depth,
> - tpg->se_tpg_tfo->get_fabric_name(), acl->initiatorname);
> -
> - kfree(acl);
> }
>
> /* core_tpg_set_initiator_node_queue_depth():
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1cadc9eefa21..9ebd86a8ef41 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -453,19 +453,25 @@ ssize_t target_show_dynamic_sessions(struct se_portal_group *se_tpg, char *page)
> }
> EXPORT_SYMBOL(target_show_dynamic_sessions);
>
> -static void target_complete_nacl(struct kref *kref)
> +static void target_destroy_nacl(struct kref *kref)
> {
> struct se_node_acl *nacl = container_of(kref,
> struct se_node_acl, acl_kref);
> + struct se_portal_group *se_tpg = nacl->se_tpg;
>
> - complete(&nacl->acl_free_comp);
> + mutex_lock(&se_tpg->acl_node_mutex);
> + list_del(&nacl->acl_list);
> + mutex_unlock(&se_tpg->acl_node_mutex);
> +
> + core_tpg_wait_for_nacl_pr_ref(nacl);
> + core_free_device_list_for_node(nacl, se_tpg);
> + kfree(nacl);
> }
>
> void target_put_nacl(struct se_node_acl *nacl)
> {
> - kref_put(&nacl->acl_kref, target_complete_nacl);
> + kref_put(&nacl->acl_kref, target_destroy_nacl);
> }
> -EXPORT_SYMBOL(target_put_nacl);
>
> void transport_deregister_session_configfs(struct se_session *se_sess)
> {
> @@ -499,12 +505,40 @@ EXPORT_SYMBOL(transport_deregister_session_configfs);
> void transport_free_session(struct se_session *se_sess)
> {
> struct se_node_acl *se_nacl = se_sess->se_node_acl;
> +
> /*
> * Drop the se_node_acl->nacl_kref obtained from within
> * core_tpg_get_initiator_node_acl().
> */
> if (se_nacl) {
> + struct se_portal_group *se_tpg = se_nacl->se_tpg;
> + const struct target_core_fabric_ops *se_tfo = se_tpg->se_tpg_tfo;
> + unsigned long flags;
> + bool stop = false;
> +
> se_sess->se_node_acl = NULL;
> +
> + /*
> + * Also determine if we need to drop the extra ->cmd_kref if
> + * it had been previously dynamically generated, and
> + * the endpoint is not caching dynamic ACLs.
> + */
> + mutex_lock(&se_tpg->acl_node_mutex);
> + if (se_nacl->dynamic_node_acl &&
> + !se_tfo->tpg_check_demo_mode_cache(se_tpg)) {
> + spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags);
> + if (list_empty(&se_nacl->acl_sess_list))
> + stop = true;
> + spin_unlock_irqrestore(&se_nacl->nacl_sess_lock, flags);
> +
> + if (stop)
> + list_del_init(&se_nacl->acl_list);
> + }
> + mutex_unlock(&se_tpg->acl_node_mutex);
> +
> + if (stop)
> + target_put_nacl(se_nacl);
> +
> target_put_nacl(se_nacl);
> }
> if (se_sess->sess_cmd_map) {
> @@ -518,16 +552,12 @@ EXPORT_SYMBOL(transport_free_session);
> void transport_deregister_session(struct se_session *se_sess)
> {
> struct se_portal_group *se_tpg = se_sess->se_tpg;
> - const struct target_core_fabric_ops *se_tfo;
> - struct se_node_acl *se_nacl;
> unsigned long flags;
> - bool drop_nacl = false;
>
> if (!se_tpg) {
> transport_free_session(se_sess);
> return;
> }
> - se_tfo = se_tpg->se_tpg_tfo;
>
> spin_lock_irqsave(&se_tpg->session_lock, flags);
> list_del(&se_sess->sess_list);
> @@ -535,35 +565,8 @@ void transport_deregister_session(struct se_session *se_sess)
> se_sess->fabric_sess_ptr = NULL;
> spin_unlock_irqrestore(&se_tpg->session_lock, flags);
>
> - /*
> - * Determine if we need to do extra work for this initiator node's
> - * struct se_node_acl if it had been previously dynamically generated.
> - */
> - se_nacl = se_sess->se_node_acl;
> -
> - mutex_lock(&se_tpg->acl_node_mutex);
> - if (se_nacl && se_nacl->dynamic_node_acl) {
> - if (!se_tfo->tpg_check_demo_mode_cache(se_tpg)) {
> - list_del(&se_nacl->acl_list);
> - drop_nacl = true;
> - }
> - }
> - mutex_unlock(&se_tpg->acl_node_mutex);
> -
> - if (drop_nacl) {
> - core_tpg_wait_for_nacl_pr_ref(se_nacl);
> - core_free_device_list_for_node(se_nacl, se_tpg);
> - se_sess->se_node_acl = NULL;
> - kfree(se_nacl);
> - }
> pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",
> se_tpg->se_tpg_tfo->get_fabric_name());
> - /*
> - * If last kref is dropping now for an explicit NodeACL, awake sleeping
> - * ->acl_free_comp caller to wakeup configfs se_node_acl->acl_group
> - * removal context from within transport_free_session() code.
> - */
> -
> transport_free_session(se_sess);
> }
> EXPORT_SYMBOL(transport_deregister_session);
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 43edf82e54ff..edad452c3c25 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -557,7 +557,6 @@ struct se_node_acl {
> struct config_group acl_fabric_stat_group;
> struct list_head acl_list;
> struct list_head acl_sess_list;
> - struct completion acl_free_comp;
> struct kref acl_kref;
> };
>
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 358041bad1da..1c417731b63d 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -125,7 +125,6 @@ void transport_register_session(struct se_portal_group *,
> struct se_node_acl *, struct se_session *, void *);
> ssize_t target_show_dynamic_sessions(struct se_portal_group *, char *);
> void transport_free_session(struct se_session *);
> -void target_put_nacl(struct se_node_acl *);
> void transport_deregister_session_configfs(struct se_session *);
> void transport_deregister_session(struct se_session *);
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists