prov/tcp: Add fi_set_val support for active/passive port range #10020
Conversation
|
It is valid to call bind with a port number over INADDR_ANY. Also could you update fi_tcp.7.md accordingly? |
|
The existing high/low port range applies to listens and accepted sockets. The proposed new range applies to sockets passed to connect. Do we need separate port ranges for sockets configured using accept() versus connect()? A single range makes life easier on the user. The original code is performing this check: If we have a source IP address or source port (or both), then the socket must be bound to that address. The expected case is to have a source IP address only (wildcard port). However, the code supports all options. The changes in this PR break the combination of wildcard IP with a specific port, as bind() no longer gets called. |
|
@shefty We had a discussion prior to the PR about whether to use the existing variables or a new set. The argument for new variables was that firewalls could configure different ranges of allowed ports for incoming connection requests and outgoing connection requests. |
|
I would change the variable names and text to better reflect their use. The difference between "port range high" and "egress port range high" is unclear. Maybe "high value for listening port" or "high value for connected socket port", etc.? |
|
I agree the variable names are not intuitive. Part of the reason is that we want to keep the original variable intact. One possibility is that we deprecate the original variables and recommend using the following new variables: Does that sound good? |
|
A socket that's created using accept() is restricted to the listen/PEP socket range. But such a socket still ends up as an 'EP' to the libfabric user. So, I'm hesitant to refer to PEP vs EP ranges. But I do like the idea of specifying the range using a single variable, rather than setting 2 separate ones, even if parsing the result takes more work. It also opens the possibility of conflicting variables. The current variable suggests that it applies to all sockets. If you want to keep the same behavior and name, at least update the help text to describe what it actually controls. |
|
Putting this PR on hold until we know this can fulfil user requirement. |
chien-intel
force-pushed
the
active_port_range
branch
from
4402fd3
to
8d5834e
Compare
chien-intel
changed the title
|
User requires finer control over TCP port range at domain level. Use fi_set_val to set low/high port range in TCP domain object. More details in commit message. I moved the bind check from xnet_endpoint to xnet_bind_port as it was nested too deep. The check is the same as before with additional condition on high port number to bind. The binding order is: 1 - port number specified in src_address, 2 - port range if defined, 3 - no port |
chien-intel
changed the title
|
Add another commit to use fi_set_val to variables corresponding to FI_TCP_PORT_LOW_RANGE and FI_TCP_PORT_HIGH_RANGE |
There is a need (e.g. firewall rule) to limit TCP ports used for
active connections.
The use case is DOAS clients started by MPI accessing DAOS servers
behind firewall. Since MPI and DAOS may not want to share the same
port restriction, add active port range support under domain object
using fi_set_val.
Define FI_TCP_DOMAIN_ACTIVE_PORT_RANGE to take a
struct xnet_port_range * of low and high port range.
Sample:
struct xnet_port_range range = { .low = 5000, .high = 6000 };
fi_set_val(&domain->fid, FI_TCP_DOMAIN_ACTIVE_PORT_RANGE, &range);
To disable the range, set low and high port range to 0.
If the source address for an active connection contains a port
number, it will be used over a defined port range.
Signed-off-by: Chien Tin Tung <chien.tin.tung@intel.com>
For applicaitons that cannot set environment variables via shell or setenv
API, add fi_set_val support to fabric object to set FI_TCP_PORT_LOW_RANGE
and FI_TCP_PORT_HIGH_RANGE values.
Define FI_TCP_FABRIC_PASSIVE_PORT_RANGE for fabric object taking
struct xnet_port_range * parameter set with desired low and high port range.
Sample:
struct xnet_port_range range = { .low = 9000, .high = 9100 };
fi_set_val(&fabric->fid, FI_TCP_FABRIC_PASSIVE_PORT_RANGE, &range);
Signed-off-by: Chien Tin Tung <chien.tin.tung@intel.com>
Signed-off-by: Chien Tin Tung <chien.tin.tung@intel.com>
chien-intel
force-pushed
the
active_port_range
branch
from
6dafe19
to
8ab97b2
Compare
I agree. We should consider moving more control knobs/capabilities under domain. I think I made changes for all review comments. Let me know if I missed anything. I'm on vacation so further changes will have some delay. |
| @@ -78,6 +78,12 @@ enum { | |||
| FI_OPT_EFA_WRITE_IN_ORDER_ALIGNED_128_BYTES, /* bool */ | |||
| }; | |||
|
|
|||
| enum { | |||
| /* null terminated string, <low port>-<high port> */ | |||
| Use fabric object with struct xnet_port_range * set with desired values. | ||
| Sample: | ||
| struct xnet_port_range range = { .low = 9000, .high = 9100 }; | ||
| fi_set_val(&fabric->fid, FI_TCP_FABRIC_PASSIVE_PORT_RANGE, &range); |
There was a problem hiding this comment.
The structure should be defined in a public header file available to the user. The standard practice for this would be to define prov/tcp/include/rdma/fi_ext_tcp.h, and define a structure there. The structure name should probably be something like fi_tcp_port_range { }. The new header should be installed if the tcp provider is built.
As an example, you can look at the gni provider. See prov/gni/include/fi_ext_gni.h. (I think they should have placed the file under include/rdma.)
There was a problem hiding this comment.
Btw, if the structure is defined as:
struct fi_tcp_port_range {
uint16_t low;
uint16_t high;
};
then you can remove the checks for the port being out of range.
There is a need (e.g. firewall rule) to limit TCP ports used for active connections. Add FI_TCP_EGRESS_PORT_LOW_RANGE and FI_TCP_EGRESS_PORT_HIGH_RANGE environment variables to specify the port range for active connections. If a port is specified for the active connection, it will be used over the specified range. This is the same behavior as passive connection.
I don't understand the check !ofi_is_any_addr(info->src_addr) in the original code. It seems there is a possibility of INADDR_ANY with a port number and continue to call bind. Anyhow, this is my best interpretation.