sysctl interface to set socket options for TCP endpoints

[tuexen]

Summary

This sysctl interface to set a socket option will be used by an upcoming tool tcpsso, which will allow to set a socket option via the command line.

This depends on some upcoming changes to tcp_ctloutput_set(), which allows it to be sent from a context different from setsockopt(). It is under review in D34164.

[rscheff]

There may be an optimization - if that sysctl is being called multiple times, intially with a buflen 0, then with a suitable allocated buffer, you may want to simply return the buffer size and return early. (although as this sysctl expects a binary blob, sysctl will not be a tool suitable to interface with this).

I see that you can set socket options wildcard style across all currently existing sockets.

Also, I'm not sure when the locking comes into play with these macros - is the INP only locked during that final iteration, or from the initialization of inpi (if at all)?

[glebius]

The fact that you needed to include in_pcb_var.h really hints that the new function belongs to in_pcb.c rather than tcp_subr.c! The only TCP specific part is that you call tcp_ctlooutput_set(). What if we create a new field in protocol methods - pr_ctloutput_set(), that would accept already locked inp. You don't need to implement it for UDP, just leave possibility to easy add this later.

[glebius]

This depends on some upcoming changes to tcp_ctloutput_set(), which allows it to be sent from a context different from setsockopt().

Do you have this already published?

[tuexen]

In D34138#772118, @glebius wrote:

This depends on some upcoming changes to tcp_ctloutput_set(), which allows it to be sent from a context different from setsockopt().

Do you have this already published?

Not yet. It will come in two steps: the first one is a cleanup with no functional change (changing function arguments as we discussed and moving locking out of the set/get functions). Will put that up later today and add you as a reviewer. The second step will deal with the case that the set function might get called not via the socket layer. Will put that up once the first is in tree.

[tuexen]

In D34138#772117, @glebius wrote:

The fact that you needed to include in_pcb_var.h really hints that the new function belongs to in_pcb.c rather than tcp_subr.c! The only TCP specific part is that you call tcp_ctlooutput_set(). What if we create a new field in protocol methods - pr_ctloutput_set(), that would accept already locked inp. You don't need to implement it for UDP, just leave possibility to easy add this later.

Sure, that makes sense. However, the code also refers to V_tcbinfo and adds the sysctl field under net.inet.tcp. Are you suggesting to add the field under net.inet?

[tuexen]

Ensure the socket does not go away if the inp gets unlocked.

[tuexen]

Hold a refcount on the socket, if possible.

[tuexen]

In D34138#772050, @rscheff wrote:

There may be an optimization - if that sysctl is being called multiple times, intially with a buflen 0, then with a suitable allocated buffer, you may want to simply return the buffer size and return early. (although as this sysctl expects a binary blob, sysctl will not be a tool suitable to interface with this).

You call this sysctl and provide the data needed to apply the socket option. This is a single call for a single end point.

I see that you can set socket options wildcard style across all currently existing sockets.

tcpsso can perform such a loop, but not the code in the kernel.

Also, I'm not sure when the locking comes into play with these macros - is the INP only locked during that final iteration, or from the initialization of inpi (if at all)?

The inp is locked in the body of the while loop.

[rscheff]

In D34138#772827, @tuexen wrote:

I see that you can set socket options wildcard style across all currently existing sockets.

tcpsso can perform such a loop, but not the code in the kernel.

The gencnt / inpi combination unique on a system?

[tuexen]

In D34138#772831, @rscheff wrote:

In D34138#772827, @tuexen wrote:

I see that you can set socket options wildcard style across all currently existing sockets.

tcpsso can perform such a loop, but not the code in the kernel.

The gencnt / inpi combination unique on a system?

When you allocate a PCB, it gets a unique (with the protocol) inp_gencnt. See in_pcb.c:660. You can use sockstat -i to get this identifier.

[glebius]

In D34138#772825, @tuexen wrote:

Hold a refcount on the socket, if possible.

Can you please explain why this became necessary?

[tuexen]

In D34138#772912, @glebius wrote:

In D34138#772825, @tuexen wrote:

Hold a refcount on the socket, if possible.

Can you please explain why this became necessary?

Normally tcp_ctloutput_set() is ccalled via the socket layer and therefore the socket can't go away. When calling it not via the socket layer, I want to have the same property. Please note, that the inp wlock is given up sometime during processing, for example when processing a socket option of a level different from IPPROTO_TCP. Are you saying, that the socket can't go away?

[tuexen]

Update for latest change in tcp_ctloutput_set().

[glebius]

I talked to rrs@ yesterday and we think that the code for sysctl_setsockopt()
is not useful for SCTP and we don't see a use case for UDP. So from that
perspective, it can stay in tcp_subr.c.

My rationale is not taking care of SCTP or UDP, but keeping generic PCB code isolated from protocol specific code. Just like back in the past I asked for TLS being a SOL_SOCKET level option, not because I really cared about TLS over local socket.

Indeed creating a pr_ctloutput_set that would be implemented just for TCP is an overkill. But we should leave space for that. If we move the function from tcp_subr.c to in_pcb.c and don't expose in_pcb_var.h to TCP, then we need to expose a few bits of TCP to generic PCB code, namely:

extern struct inpcbinfo V_tcbinfo;
extern int tcp_ctloutput_set();

IMHO, this would be lesser evil than exposing generic bits to protocol specific code. And in future this can be easily translated into pr_ctloutput_set for massive setsockopts on other protocols.

[glebius]

Actually, tcpbinfo is already seen by in_pcb.c via tcp_var.h, so only tcp_ctloutput_set() needs to be exposed.

[tuexen]

Try to make Gleb happy...

[tuexen]

Gleb, does this version make you happy? Or do you have something else in mind? I find passing around the pcbinfo pointer and the function pointer no so elegant, but I have not found a better way. The only option I considered is to use IPPROTO_TCP as arg2 in the sysctl parameters and deduce from that the pcbinfo and function pointer. But then I have a problem with the INP_ALL_ITERATOR macro.

[glebius]

This is even better than what I was trying to describe. A middle ground between making a new protocol method and a hack with exporting single TCP function to inpcb code. Thanks a lot! :)