Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for IP_LOCAL_PORT_RANGE socket option. #81483

Merged

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Nov 17, 2024

Add support for IP_LOCAL_PORT_RANGE socket option. The option supports both IPv4 and IPv6 sockets although the type is IPPROTO_IP.
The option can be used to enforce the ephemeral port number selection to be in certain range.

bool "Allow clamping down the global local port range for net_context"
depends on NET_UDP || NET_TCP
help
Set or get the per-context default local port range. This
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Set or get the per-context default local port range. This
Set or get the per-context default local port range. This

* then we use the range. Also make sure that upper is
* greater than lower.
*/
if (upper == 0 || lower == 0 || upper < lower) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If upper is supposed to be greater than lower, then we should check for upper <= lower here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we would not be able to have a range of one port number. That would probably not be very practical but I suppose it could be done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in that case, if we want to allow single-number ranges, then we have some inconsistencies in the docs. The net context documentation says:

		 * The lower bound has to be less than the upper bound when
		 * both bounds are not zero

Also the comment just above the check says:

			 * then we use the range. Also make sure that upper is
			 * greater than lower.

And finally, when verifying range in check_used_port() we check:

		if (upper != 0 && lower != 0 && lower < upper) {

All those assume that lower and upper bounds should not be equal.

To be fair, either approach works for me (although if one wanted to use a specific port, he could simply bind to it), just need to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I think we need to stick with what the doc says as it is copied from Linux. I will fix the checks.

} while (check_used_port(context, NULL, net_context_get_proto(context),
htons(local_port), addr, false, false) == -EEXIST);
htons(local_port), addr, false, false, false) == -EEXIST);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit tricky, but what if someone sets a short range and all of the ports within the range get used - we may spin here indefinitely then? Maybe we should add some retry counter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will figure out some fix for that.

Add support for IP_LOCAL_PORT_RANGE socket option. The option
supports both IPv4 and IPv6 sockets although the type is IPPROTO_IP.

The option can be used to enforce the ephemeral port number selection
to be in certain range.

Signed-off-by: Jukka Rissanen <[email protected]>
Make sure that the IP_LOCAL_PORT_RANGE socket option works
as expected.

Signed-off-by: Jukka Rissanen <[email protected]>
@jukkar jukkar force-pushed the devel/support-port-range-selection branch from 39600f8 to 001c71e Compare November 19, 2024 13:08
@jukkar
Copy link
Member Author

jukkar commented Nov 19, 2024

  • rebased to latest main and fixed the merge conflicts
  • fixes according to comments

@jukkar jukkar requested a review from cfriedt November 21, 2024 06:41
@jukkar
Copy link
Member Author

jukkar commented Nov 22, 2024

@pdgendt / @cfriedt if you have any spare cycles, please take a look. Thanks in advance 🙇

Copy link
Collaborator

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kartben kartben merged commit 5531692 into zephyrproject-rtos:main Nov 22, 2024
24 checks passed
@jukkar jukkar deleted the devel/support-port-range-selection branch November 25, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants