Fixing gang ABD child removal race condition

On linux the list debug code has been setting off a failure when
checking that the node->next->prev value is pointing back at the node.
At times this check evaluates to 0xdead. When removing a child from a
gang ABD we must acquire the child's abd_mtx to make sure that the
same ABD is not being added to another gang ABD while it is being
removed from a gang ABD. This fixes a race condition when checking
if an ABDs link is already active and part of another gang ABD before
adding it to a gang.

Added additional debug code for the gang ABD in abd_verify() to make
sure each child ABD has active links. Also check to make sure another
gang ABD is not added to a gang ABD.

Reviewed-by: Serapheim Dimitropoulos <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Ahrens <[email protected]>
Signed-off-by: Brian Atkinson <[email protected]>
Closes #10511 

4 files changed, 20 insertions, 4 deletions

diff --git a/include/os/linux/spl/sys/list.h b/include/os/linux/spl/sys/list.h
index 74b784e93..be38f328f 100644
--- a/include/os/linux/spl/sys/list.h
+++ b/include/os/linux/spl/sys/list.h
@@ -26,6 +26,7 @@
 #define	_SPL_LIST_H
 
 #include <sys/types.h>
+#include <sys/debug.h>
 #include <linux/list.h>
 
 /*
@@ -184,7 +185,8 @@ list_prev(list_t *list, void *object)
 static inline int
 list_link_active(list_node_t *node)
 {
-	return (node->next != LIST_POISON1) && (node->prev != LIST_POISON2);
+	EQUIV(node->next == LIST_POISON1, node->prev == LIST_POISON2);
+	return (node->next != LIST_POISON1);
 }
 
 static inline void
diff --git a/lib/libspl/list.c b/lib/libspl/list.c
index 52327b2bb..0f2f3731b 100644
--- a/lib/libspl/list.c
+++ b/lib/libspl/list.c
@@ -232,6 +232,7 @@ list_link_init(list_node_t *ln)
 int
 list_link_active(list_node_t *ln)
 {
+	EQUIV(ln->next == NULL, ln->prev == NULL);
 	return (ln->next != NULL);
 }
 
diff --git a/module/os/freebsd/spl/list.c b/module/os/freebsd/spl/list.c
index e8db13a5c..21230b2ad 100644
--- a/module/os/freebsd/spl/list.c
+++ b/module/os/freebsd/spl/list.c
@@ -235,6 +235,7 @@ list_link_init(list_node_t *link)
 int
 list_link_active(list_node_t *link)
 {
+	EQUIV(link->list_next == NULL, link->list_prev == NULL);
 	return (link->list_next != NULL);
 }
 
diff --git a/module/zfs/abd.c b/module/zfs/abd.c
index 269f2a7e5..3d1ed63cf 100644
--- a/module/zfs/abd.c
+++ b/module/zfs/abd.c
@@ -142,6 +142,7 @@ abd_verify(abd_t *abd)
 		for (abd_t *cabd = list_head(&ABD_GANG(abd).abd_gang_chain);
 		    cabd != NULL;
 		    cabd = list_next(&ABD_GANG(abd).abd_gang_chain, cabd)) {
+			ASSERT(list_link_active(&cabd->abd_gang_link));
 			abd_verify(cabd);
 		}
 	} else {
@@ -290,10 +291,19 @@ static void
 abd_free_gang_abd(abd_t *abd)
 {
 	ASSERT(abd_is_gang(abd));
-	abd_t *cabd;
+	abd_t *cabd = list_head(&ABD_GANG(abd).abd_gang_chain);
 
-	while ((cabd = list_remove_head(&ABD_GANG(abd).abd_gang_chain))
-	    != NULL) {
+	while (cabd != NULL) {
+		/*
+		 * We must acquire the child ABDs mutex to ensure that if it
+		 * is being added to another gang ABD we will set the link
+		 * as inactive when removing it from this gang ABD and before
+		 * adding it to the other gang ABD.
+		 */
+		mutex_enter(&cabd->abd_mtx);
+		ASSERT(list_link_active(&cabd->abd_gang_link));
+		list_remove(&ABD_GANG(abd).abd_gang_chain, cabd);
+		mutex_exit(&cabd->abd_mtx);
 		abd->abd_size -= cabd->abd_size;
 		if (cabd->abd_flags & ABD_FLAG_GANG_FREE) {
 			if (cabd->abd_flags & ABD_FLAG_OWNER)
@@ -301,6 +311,7 @@ abd_free_gang_abd(abd_t *abd)
 			else
 				abd_put(cabd);
 		}
+		cabd = list_head(&ABD_GANG(abd).abd_gang_chain);
 	}
 	ASSERT0(abd->abd_size);
 	list_destroy(&ABD_GANG(abd).abd_gang_chain);
@@ -373,6 +384,7 @@ void
 abd_gang_add(abd_t *pabd, abd_t *cabd, boolean_t free_on_free)
 {
 	ASSERT(abd_is_gang(pabd));
+	ASSERT(!abd_is_gang(cabd));
 	abd_t *child_abd = NULL;
 
 	/*