The branch main has been updated by tuexen:
URL:
https://cgit.FreeBSD.org/src/commit/?id=859f0f0d6bf753c16caa0103a541c69fb6bd5e37
commit 859f0f0d6bf753c16caa0103a541c69fb6bd5e37
Author: Michael Tuexen <tue...@freebsd.org>
AuthorDate: 2024-07-13 10:22:25 +0000
Commit: Michael Tuexen <tue...@freebsd.org>
CommitDate: 2024-07-13 10:22:25 +0000
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 | 25 +++++++++++---
sys/netinet/tcp_subr.c | 78 ++++++++++++++++++++++--------------------
sys/netinet/tcp_var.h | 3 ++
3 files changed, 64 insertions(+), 42 deletions(-)
diff --git a/share/man/man9/tcp_functions.9 b/share/man/man9/tcp_functions.9
index 1e0616e03a9f..8ba7f21c978c 100644
--- a/share/man/man9/tcp_functions.9
+++ b/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
diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c
index b4f605534d59..5743d1bbd100 100644
--- a/sys/netinet/tcp_subr.c
+++ b/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);
}
- if (blk->tfb_flags & TCP_FUNC_BEING_REMOVED) {
- *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) {
+ 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);
}
diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
index 8330966c2c3f..1f48297c2b0a 100644
--- a/sys/netinet/tcp_var.h
+++ b/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];
