Page MenuHomeFreeBSD

tcp: refactor register_tcp_functions_as_names()
ClosedPublic

Authored by tuexen on Jul 10 2024, 7:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Sep 30, 11:14 PM
Unknown Object (File)
Wed, Sep 18, 10:17 AM
Unknown Object (File)
Sep 17 2024, 8:23 PM
Unknown Object (File)
Sep 8 2024, 10:15 PM
Unknown Object (File)
Sep 8 2024, 6:38 PM
Unknown Object (File)
Sep 8 2024, 5:31 AM
Unknown Object (File)
Sep 8 2024, 3:38 AM
Unknown Object (File)
Sep 7 2024, 9:26 PM

Details

Summary

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 just removed without holding a lock in between or checking ref counts.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

cc requested changes to this revision.Jul 11 2024, 2:22 PM
cc added a subscriber: cc.
cc added inline comments.
sys/netinet/tcp_subr.c
1188

This E2BIG returns a new error, please update and specify it in section ERRORS of the man page of TCP_FUNCTIONS(9).

1203

This is critical. It will not free f[0]. Shall be:

	while (--i >= 0)
sys/netinet/tcp_var.h
644

Glancing through TCP_FUNCTIONS(9) and manually counting the number of functions in struct tcp_function_block (which is 19 if my eyes don't cheat), why pick this number 16?

Or this is a readability problem to me that TCP_FUNCTION_NAME_NUM_MAX shall be TCP_FUNCTION_BLOCK_NAME_NUM_MAX , per the usage. That means you can't register more than 16 TCP stacks (TCP function blocks)? Then, again why pick this number 16?

If it is the readability problem, I suggest update the comment as well, something like:
/* Maximum number of TCP stack (TCP function block) names a single TCP function block can use. */

This revision now requires changes to proceed.Jul 11 2024, 2:22 PM

Address two of the three issues raised by cc.

tuexen added inline comments.
sys/netinet/tcp_subr.c
1188

Done.

1203

Fixed. Thanks for catching.

sys/netinet/tcp_var.h
644

A TCP function block has one name and is registered using a number of names or aliases. TCP_FUNCTION_NAME_NUM_MAX is the maximum number of times each TCP function block an be registered under different names.
This does NOT limit the number of TCP function blocks, or the components of the structure.
Example: assume you have a TCP function block blk1 with name "tcp_june2024" and a second one blk2 with "tcp_july2024".
So you might want to register blk1 under the name "tcp_june2024" and "tcp_stable" and register blk2 under the name "tcp_jul2024" and "tcp_latest".
In this case you would use 2 names. So you can't use more than TCP_FUNCTION_NAME_NUM_MAX names. I guess 16 is enough.

Don't include unrelated change.

Over all looks good to me. Thanks for the update.

sys/netinet/tcp_var.h
644

I understand. Thanks for the clarity.

First, it was confusing to me that I was thinking something else like these function pointer names inside the struct tcp_function_block at the first glance, given the word use in the comment. That's why I guessed it's a readability problem to me and I recommended the comment update.

Something like below, with the emphasis on word register:

/* Maximum number of names (TCP function block names) a single TCP function block can register. */

Second, I took some time to think about this magic number 16 per the discussion in our group meeting.

  1. Clearly, both 8 and 16 are large enough. And there won't need a larger than 8 number in the near future, as 8 is already a roundup number and is more than double of the current status quo.
  2. I would like to see (if possible in my life) the reason that someone could possibly update this number to 16 or larger with a persuasive reason.

So I would like to recommend this magic number to 8.

This should address the remaining issue brought up by cc@.

tuexen added inline comments.
sys/netinet/tcp_var.h
644

I reduced the constant to 8 and improved the comment describing this constant.

Thanks for the update! Looks good to me.

This revision is now accepted and ready to land.Jul 12 2024, 3:04 PM