tcp: refactor register_tcp_functions_as_names()

Refactor register_tcp_functions_as_names() such that either all or
no (in error cases) registrations happen atomically (while holding
the tcp_function_lock write lock). Also ensure that the TCP function
block is not already registered.
This avoids situations, where some registrations were performed and
then they were removed without holding a lock in between or checking
ref counts.

Reviewed by:		cc
MFC after:		1 week
Sponsored by:		Netflix, Inc.
Differential Revision:	https://reviews.freebsd.org/D45947

share/man/man9/tcp_functions.9

@@ -23,7 +23,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd June 6, 2024
+.Dd July 13, 2024
 .Dt TCP_FUNCTIONS 9
 .Os
 .Sh NAME
@@ -37,6 +37,7 @@
 .Ft int
 .Fn register_tcp_functions_as_name "struct tcp_function_block *blk" \
 "const char *name" "int wait"
+.Ft int
 .Fn register_tcp_functions_as_names "struct tcp_function_block *blk" \
 "int wait" "const char *names[]" "int *num_names"
 .Ft int
@@ -112,6 +113,7 @@ argument.
 The
 .Fa num_names
 argument provides a pointer to the number of names.
+This number must not exceed TCP_FUNCTION_NAME_NUM_MAX.
 This function will either succeed in registering all of the names in the array,
 or none of the names in the array.
 On failure, the
@@ -328,8 +330,11 @@ must be prepared to wait until all connections have stopped using the
 specified TCP stack.
 .Sh ERRORS
 The
-.Fn register_tcp_functions
-function will fail if:
+.Fn register_tcp_functions ,
+.Fn register_tcp_functions_as_name ,
+and
+.Fn register_tcp_functions_as_names
+functions will fail if:
 .Bl -tag -width Er
 .It Bq Er EINVAL
 Any of the members of the
@@ -338,7 +343,19 @@ argument are set incorrectly.
 .It Bq Er ENOMEM
 The function could not allocate memory for its internal data.
 .It Bq Er EALREADY
-A function block is already registered with the same name.
+The
+.Fa blk
+is already registered or a function block is already registered with the same
+name.
+.El
+Additionally,
+.Fn register_tcp_functions_as_names
+will fail if:
+.Bl -tag -width Er
+.It Bq Er E2BIG
+The number of names pointed to by the
+.Fa num_names
+argument is larger than TCP_FUNCTION_NAME_NUM_MAX.
 .El
 The
 .Fn deregister_tcp_functions

sys/netinet/tcp_subr.c

@@ -1172,9 +1172,9 @@ int
 register_tcp_functions_as_names(struct tcp_function_block *blk, int wait,
     const char *names[], int *num_names)
 {
-	struct tcp_function *n;
+	struct tcp_function *f[TCP_FUNCTION_NAME_NUM_MAX];
 	struct tcp_function_set fs;
-	int error, i;
+	int error, i, num_registered;
 
 	KASSERT(names != NULL, ("%s: Called with NULL name list", __func__));
 	KASSERT(*num_names > 0,
@@ -1182,71 +1182,73 @@ register_tcp_functions_as_names(struct tcp_function_block *blk, int wait,
 	KASSERT(rw_initialized(&tcp_function_lock),
 	    ("%s: called too early", __func__));
 
+	if (*num_names > TCP_FUNCTION_NAME_NUM_MAX) {
+		/* Too many names. */
+		*num_names = 0;
+		return (E2BIG);
+	}
 	if ((blk->tfb_tcp_output == NULL) ||
 	    (blk->tfb_tcp_do_segment == NULL) ||
 	    (blk->tfb_tcp_ctloutput == NULL) ||
 	    (blk->tfb_tcp_handoff_ok == NULL) ||
 	    (strlen(blk->tfb_tcp_block_name) == 0)) {
-		/*
-		 * These functions are required and you
-		 * need a name.
-		 */
+		/* These functions are required and a name is needed. */
 		*num_names = 0;
 		return (EINVAL);
 	}
 
+	for (i = 0; i < *num_names; i++) {
+		f[i] = malloc(sizeof(struct tcp_function), M_TCPFUNCTIONS, wait);
+		if (f[i] == NULL) {
+			while (--i >= 0)
+				free(f[i], M_TCPFUNCTIONS);
+			*num_names = 0;
+			return (ENOMEM);
+		}
+	}
+
+	num_registered = 0;
+	rw_wlock(&tcp_function_lock);
+	if (find_tcp_fb_locked(blk, NULL) != NULL) {
+		/* A TCP function block can only be registered once. */
+		error = EALREADY;
+		goto cleanup;
+	}
 	if (blk->tfb_flags & TCP_FUNC_BEING_REMOVED) {
-		*num_names = 0;
-		return (EINVAL);
+		error = EINVAL;
+		goto cleanup;
 	}
-
 	refcount_init(&blk->tfb_refcnt, 0);
 	blk->tfb_id = atomic_fetchadd_int(&next_tcp_stack_id, 1);
 	for (i = 0; i < *num_names; i++) {
-		n = malloc(sizeof(struct tcp_function), M_TCPFUNCTIONS, wait);
-		if (n == NULL) {
-			error = ENOMEM;
-			goto cleanup;
-		}
-		n->tf_fb = blk;
-
 		(void)strlcpy(fs.function_set_name, names[i],
 		    sizeof(fs.function_set_name));
-		rw_wlock(&tcp_function_lock);
 		if (find_tcp_functions_locked(&fs) != NULL) {
 			/* Duplicate name space not allowed */
-			rw_wunlock(&tcp_function_lock);
-			free(n, M_TCPFUNCTIONS);
 			error = EALREADY;
 			goto cleanup;
 		}
-		(void)strlcpy(n->tf_name, names[i], sizeof(n->tf_name));
-		TAILQ_INSERT_TAIL(&t_functions, n, tf_next);
+		f[i]->tf_fb = blk;
+		(void)strlcpy(f[i]->tf_name, names[i], sizeof(f[i]->tf_name));
+		TAILQ_INSERT_TAIL(&t_functions, f[i], tf_next);
 		tcp_fb_cnt++;
-		rw_wunlock(&tcp_function_lock);
+		num_registered++;
 	}
+	rw_wunlock(&tcp_function_lock);
 	return (0);
 
 cleanup:
-	/*
-	 * Deregister the names we just added. Because registration failed
-	 * for names[i], we don't need to deregister that name.
-	 */
-	*num_names = i;
-	rw_wlock(&tcp_function_lock);
-	while (--i >= 0) {
-		TAILQ_FOREACH(n, &t_functions, tf_next) {
-			if (!strncmp(n->tf_name, names[i],
-			    TCP_FUNCTION_NAME_LEN_MAX)) {
-				TAILQ_REMOVE(&t_functions, n, tf_next);
-				tcp_fb_cnt--;
-				n->tf_fb = NULL;
-				free(n, M_TCPFUNCTIONS);
-				break;
-			}
+	/* Remove the entries just added. */
+	for (i = 0; i < *num_names; i++) {
+		if (i < num_registered) {
+			TAILQ_REMOVE(&t_functions, f[i], tf_next);
+			tcp_fb_cnt--;
 		}
+		f[i]->tf_fb = NULL;
+		free(f[i], M_TCPFUNCTIONS);
 	}
 	rw_wunlock(&tcp_function_lock);
+	*num_names = num_registered;
 	return (error);
 }
 

sys/netinet/tcp_var.h

@@ -634,6 +634,9 @@ struct tcp_function_block {
 	uint8_t	tfb_id;
 };
 
+/* Maximum number of names each TCP function block can be registered with. */
+#define	TCP_FUNCTION_NAME_NUM_MAX	8
+
 struct tcp_function {
 	TAILQ_ENTRY(tcp_function)	tf_next;
 	char				tf_name[TCP_FUNCTION_NAME_LEN_MAX];