[PATCH] scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

[PATCH] scsi: target: fix list corruption in core_alua_free_tg_pt_gp()

[Yu Zhang]

core_alua_free_tg_pt_gp() moves every se_lun off the group being freed
and reattaches it to the default group.  For each lun it removes the
membership link under tg_pt_gp->tg_pt_gp_lock, drops that lock, and only
then takes lun->lun_tg_pt_gp_lock to call __target_attach_tg_pt_gp().

lun->lun_tg_pt_gp still points at the group being freed across that gap.
A concurrent core_alua_store_tg_pt_gp_info() holds lun->lun_tg_pt_gp_lock
across both the detach and the attach (via target_swap_tg_pt_gp()), so it
can move the same lun to a third group and re-add lun_tg_pt_gp_link to
that group's list in the window.  When the free path then runs
__target_attach_tg_pt_gp(), its bare list_add_tail() links the
already-linked node a second time, corrupting both lists; the lun's
group pointer is also overwritten and tg_pt_gp_members is decremented
twice.

Writing a new group id to a LUN's alua_tg_pt_gp attribute while another
process removes the LUN's current group (both via configfs) is enough to
trigger it:

  list_add double add: new=ffff8880055a64e0, prev=ffff8880055a64e0, next=ffff8880055a64e0.
  kernel BUG at lib/list_debug.c:35!
  RIP: 0010:__list_add_valid_or_report+0xdf/0xf0
  Call Trace:
   __target_attach_tg_pt_gp+0x9b/0x190
   core_alua_free_tg_pt_gp+0x22f/0x3a0
   config_item_put+0x89/0xa0
   configfs_rmdir+0x42f/0x5b0
   vfs_rmdir+0x22a/0x4b0

Re-check under lun->lun_tg_pt_gp_lock that the lun still references the
group being freed before reattaching it to the default group.  If a
concurrent store has already moved it, there is nothing to do: that
store holds lun->lun_tg_pt_gp_lock across its own detach and attach, so
the node stays consistently linked in the new group's list.

The store side was already made atomic in commit f9793d649c29 ("scsi:
target: Perform ALUA group changes in one step"); the free path was left
reattaching without revalidating the lun's group.

Fixes: adf653f92f38 ("target: Subsume se_port + t10_alua_tg_pt_gp_member into se_lun")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/target/target_core_alua.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 10250ac..3a085e2 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -1746,16 +1746,23 @@ void core_alua_free_tg_pt_gp(
 
 		spin_unlock(&tg_pt_gp->tg_pt_gp_lock);
 		/*
-		 * If the passed tg_pt_gp does NOT match the default_tg_pt_gp,
-		 * assume we want to re-associate a given tg_pt_gp_mem with
-		 * default_tg_pt_gp.
+		 * tg_pt_gp_lock was dropped above, so a concurrent
+		 * core_alua_store_tg_pt_gp_info() may already have moved this
+		 * lun to another group and re-added lun_tg_pt_gp_link to that
+		 * group's list.  Reattach to the default group only while the
+		 * lun still references the group being freed; otherwise the
+		 * bare list_add_tail() in __target_attach_tg_pt_gp() would add
+		 * the already-linked node a second time and corrupt the list.
 		 */
 		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);
+		if (rcu_dereference_check(lun->lun_tg_pt_gp,
+				lockdep_is_held(&lun->lun_tg_pt_gp_lock)) == tg_pt_gp) {
+			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);
-- 
2.43.0