Make tcp_ctloutput_set() non-static

[tuexen]

Summary

This allows tcp_ctloutput_set() to be called from contexts other than setsockopt(). One example of this usage is D34138.
The caller must hold a write lock in the inp and a reference count on inp->inp_socket.

[rscheff]

mostly mechanical changes, look good to me...

[glebius]

An inp with NULL inp_socket is a dying one. Why are we interested in supporting those setsockopts that may work without socket? Ah, I see the answer - to switch TCP stack on connections that are in TIME-WAIT. What do you think is the added complexity worth or not? Maybe just drop those TIME-WAITs?

[glebius]

Or maybe this suggestion is better: in the very beginning check that sopt_name does belong to small subset of options that require socket and only then check for inp_socket?

[glebius]

The only opts that require inp_socket are the TLS ones. That reminds me that 2-3 years ago I was arguing with Randall @rrs that these TLS options should be SOL_SOCKET level options, not IPPROTO_TCP, as they work solely on the socket buffer, not on TCP cb. I didn't convince back then. Now this decision bytes back :(

[tuexen]

In D34164#772909, @glebius wrote:

An inp with NULL inp_socket is a dying one. Why are we interested in supporting those setsockopts that may work without socket? Ah, I see the answer - to switch TCP stack on connections that are in TIME-WAIT. What do you think is the added complexity worth or not? Maybe just drop those TIME-WAITs?

Maybe I'm wrong. But can't live the TCP connection much longer than a socket exists?
We are not calling tcp_ctloutput_set() if the INP_TIMEWAIT or the INP_DROPPED flag is set. So are you saying that if none of these flags are set, inp_socket is not NULL?

[tuexen]

In D34164#772910, @glebius wrote:

Or maybe this suggestion is better: in the very beginning check that sopt_name does belong to small subset of options that require socket and only then check for inp_socket?

At the very beginning of which function?

[tuexen]

In D34164#772911, @glebius wrote:

The only opts that require inp_socket are the TLS ones. That reminds me that 2-3 years ago I was arguing with Randall @rrs that these TLS options should be SOL_SOCKET level options, not IPPROTO_TCP, as they work solely on the socket buffer, not on TCP cb. I didn't convince back then. Now this decision bytes back :(

But does the code for anything else than TCP?

[glebius]

In D34164#772966, @tuexen wrote:

We are not calling tcp_ctloutput_set() if the INP_TIMEWAIT or the INP_DROPPED flag is set. So are you saying that if none of these flags are set, inp_socket is not NULL?

When doing the SMR work I wanted it to be so and I wish achieved this, if inp is acquired through legal KPIs. Let's check... inp_socket is assigned immediately after inp allocation. The only place we clear inp_socket is in_pcbdetach(). Unfortunately this single point of destruction is called differently by different protocols, and TCP calls it in 5 different scenarios. However, one is embryonic failure case, 3 others are INP_TIMEWAIT|INP_DROPPED and the only last case left has a comment XXXRW: Does the second case still occur. This comment is consistent with comment above tcp_usr_detach(). So, I would vote - let's KASSERT that inp_socket is valid. If we catch a panic, it means something needs to be fixed outside of your code.

[glebius]

In D34164#772968, @tuexen wrote:

In D34164#772911, @glebius wrote:

The only opts that require inp_socket are the TLS ones. That reminds me that 2-3 years ago I was arguing with Randall @rrs that these TLS options should be SOL_SOCKET level options, not IPPROTO_TCP, as they work solely on the socket buffer, not on TCP cb. I didn't convince back then. Now this decision bytes back :(

But does the code for anything else than TCP?

No it doesn't. But TLS still remains socket level feature, not TCP. That was and is my point. Potentially you could run TLS on PF_UNIX/SOCK_STREAM. Doesn't make much sense of course. I don't know about SDP Infiniband sockets much, but they also are SOCK_STREAM so also could easily achieve TLS.

My point was not that I want to have TLS for PF_UNIX/SOCK_STREAM. Of course not. My point was that I desire architecturally clean design, where layering violations are avoided if possible. And if the sockopts were made SOL_SOCKET, today we won't have discussion on validity of inp_socket. :(

[tuexen]

In D34164#773013, @glebius wrote:

In D34164#772966, @tuexen wrote:

We are not calling tcp_ctloutput_set() if the INP_TIMEWAIT or the INP_DROPPED flag is set. So are you saying that if none of these flags are set, inp_socket is not NULL?

When doing the SMR work I wanted it to be so and I wish achieved this, if inp is acquired through legal KPIs. Let's check... inp_socket is assigned immediately after inp allocation. The only place we clear inp_socket is in_pcbdetach(). Unfortunately this single point of destruction is called differently by different protocols, and TCP calls it in 5 different scenarios. However, one is embryonic failure case, 3 others are INP_TIMEWAIT|INP_DROPPED and the only last case left has a comment XXXRW: Does the second case still occur. This comment is consistent with comment above tcp_usr_detach(). So, I would vote - let's KASSERT that inp_socket is valid. If we catch a panic, it means something needs to be fixed outside of your code.

Why don't we add a panic() call where the comment is XXXRW? Then we could figure out what the code path is we are looking for. Adding to the code path here, just indicates that something goes one we are not expecting.
Once we are sure, the panic at XXXRW is not triggered, we can clean the code up here.

[tuexen]

In D34164#773014, @glebius wrote:

In D34164#772968, @tuexen wrote:

In D34164#772911, @glebius wrote:

The only opts that require inp_socket are the TLS ones. That reminds me that 2-3 years ago I was arguing with Randall @rrs that these TLS options should be SOL_SOCKET level options, not IPPROTO_TCP, as they work solely on the socket buffer, not on TCP cb. I didn't convince back then. Now this decision bytes back :(

But does the code for anything else than TCP?

No it doesn't. But TLS still remains socket level feature, not TCP. That was and is my point. Potentially you could run TLS on PF_UNIX/SOCK_STREAM. Doesn't make much sense of course. I don't know about SDP Infiniband sockets much, but they also are SOCK_STREAM so also could easily achieve TLS.

My point was not that I want to have TLS for PF_UNIX/SOCK_STREAM. Of course not. My point was that I desire architecturally clean design, where layering violations are avoided if possible. And if the sockopts were made SOL_SOCKET, today we won't have discussion on validity of inp_socket. :(

We would still have them. At the beginning of tcp_ctloutput_set() we are delegating all socket options having a level different from IPPROTO_TCP to ip_ctloutput() or ip6_ctloutput().

But I agree, it would really be great to get it clear when inp_socket can be NULL. A similar case, where such a check has recently been added is tcp_subr.c:3923...

[tuexen]

Address Gleb's comments.

[glebius]

This comment has been deleted.