Re: [PATCH] scsi: target: alua: Free target port groups with kfree_rcu()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] scsi: target: race condition in core_alua_free_tg_pt_gp() [1]
--

commit cbab3de9aa10ecf910c633ef186a6a1775418e09
Author: Qiliang Yuan <qiliang.yuan@linux.dev>

scsi: target: alua: Free target port groups with kfree_rcu()

This commit replaces synchronize_rcu() and kmem_cache_free() in
core_alua_free_tg_pt_gp() with kfree_rcu() to avoid synchronous RCU waits
when deleting target port groups and backstores.

> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> index 140154d93c430..4ef243ae843bb 100644
> --- a/drivers/target/target_core_alua.c
> +++ b/drivers/target/target_core_alua.c
> @@ -1771,8 +1771,7 @@ void core_alua_free_tg_pt_gp(
>  	}
>  	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
>  
> -	synchronize_rcu();
> -	kmem_cache_free(t10_alua_tg_pt_gp_cache, tg_pt_gp);
> +	kfree_rcu(tg_pt_gp, rcu_head);
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but can the LUN detachment
loop right above this race with target_detach_tg_pt_gp() [1] and
leave a freed struct se_lun on default_tg_pt_gp->tg_pt_gp_lun_list?

drivers/target/target_core_alua.c:core_alua_free_tg_pt_gp() {
    ...
	spin_lock(&tg_pt_gp->tg_pt_gp_lock);
	list_for_each_entry_safe(lun, next,
			&tg_pt_gp->tg_pt_gp_lun_list, lun_tg_pt_gp_link) {
		list_del_init(&lun->lun_tg_pt_gp_link);
		tg_pt_gp->tg_pt_gp_members--;

		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
		...
		spin_lock(&lun->lun_tg_pt_gp_lock);
		if (tg_pt_gp != dev->t10_alua.default_tg_pt_gp) {
			__target_attach_tg_pt_gp(lun,
					dev->t10_alua.default_tg_pt_gp);
		} else
			rcu_assign_pointer(lun->lun_tg_pt_gp, NULL);
		spin_unlock(&lun->lun_tg_pt_gp_lock);

		spin_lock(&tg_pt_gp->tg_pt_gp_lock);
	}
	spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
}

When a non-default target port group is removed through configfs while a
fabric LUN in that group is being unlinked, core_alua_free_tg_pt_gp()
drops tg_pt_gp->tg_pt_gp_lock before taking lun->lun_tg_pt_gp_lock while
lun->lun_tg_pt_gp still points to tg_pt_gp:

CPU0                                   CPU1
core_alua_free_tg_pt_gp(tg_pt_gp)      target_fabric_port_unlink()
  spin_lock(&tg_pt_gp->tg_pt_gp_lock)    core_tpg_remove_lun(lun)
  list_del_init(&lun->lun_tg_pt_gp_link)   target_detach_tg_pt_gp(lun)
  tg_pt_gp->tg_pt_gp_members--
  spin_unlock(&tg_pt_gp->tg_pt_gp_lock)
                                             spin_lock(&lun->lun_tg_pt_gp_lock)
                                             __target_detach_tg_pt_gp(...)
                                             rcu_assign_pointer(
                                               lun->lun_tg_pt_gp, NULL)
                                             spin_unlock(&lun->lun_tg_pt_gp_lock)
  spin_lock(&lun->lun_tg_pt_gp_lock)
  __target_attach_tg_pt_gp(lun,
    default_tg_pt_gp)
  spin_unlock(&lun->lun_tg_pt_gp_lock)
                                       target_fabric_port_release()
                                         ...
                                           kfree(lun)
                                       target_emulate_report_target_port_groups()
                                         /* walks default_tg_pt_gp list */
                                         lun->...  <-- use-after-free

Because core_alua_free_tg_pt_gp() does not recheck lun->lun_tg_pt_gp after
acquiring lun->lun_tg_pt_gp_lock, it re-attaches the detaching LUN to
default_tg_pt_gp after target_detach_tg_pt_gp() has already cleared it
(and decremented tg_pt_gp->tg_pt_gp_members a second time).  When port
teardown subsequently frees the LUN, a dangling entry remains on
default_tg_pt_gp->tg_pt_gp_lun_list.

[1] https://sashiko.dev/#/bug/linux-8e4ef2cb-206e-4a76-8c9e-458e67913f8b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-bug-target-alua-free-gp-v1-1-d1e692ae8694@linux.dev?part=1