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

[Qiliang Yuan]

+Cc Martin, Mike and the patch's other recipients, and Yu Zhang, whose
patch fixes this.

On Wed, Nov 11, 2026 at 01:58:54PM +0000, sashiko-bot@xxxxxxxxxx wrote:
> 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@xxxxxxxxx>
>
> 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

The race is real, and it is already being fixed. Yu's patch [2] checks
lun->lun_tg_pt_gp again under lun_tg_pt_gp_lock before reattaching the
LUN, which covers target_detach_tg_pt_gp() as well as the concurrent
group change it was written for. This patch doesn't touch that loop.

[2] https://lore.kernel.org/r/20261019033532.73575-1-yuz08559@xxxxxxxxx

Thanks,
Qiliang