Should we check for TCP_FUNC_BEING_REMOVED?

In D45411#1035980, @jtl wrote:

Should we check for TCP_FUNC_BEING_REMOVED?

I thought about this and did again after you raised the question. The original code didn't. I think for consistency with the code path in the setsockopt() handler for TCP_FUNCTION_BLK it makes sense to use:

	if (V_functions_inherit_listen_socket_stack)
		tfb = tcp_find_and_ref_fb(sototcpcb(lso)->t_fb);
	else
		tfb = tcp_find_and_ref_default_fb();
	KASSERT(tfb != NULL, ("Cannot find TCP function block."));
	if ((tfb->tfb_flags & TCP_FUNC_BEING_REMOVED) ||
	    (tp = tcp_newtcpcb(inp, tfb)) == NULL) {
		refcount_release(&tfb->tfb_refcnt);
		in_pcbfree(inp);
		sodealloc(so);
		goto allocfail;
	}

However, since we don't hold the tcp_function_lock, the tfb can be removed any time before or after we set the TCP function block in tcp_newtcpcb. It cannot go away, since we hold a reference count. I guess that is the reason why there was no check...

In D45411#1036082, @tuexen wrote:

In D45411#1035980, @jtl wrote:

Should we check for TCP_FUNC_BEING_REMOVED?

I thought about this and did again after you raised the question. The original code didn't.
...
However, since we don't hold the tcp_function_lock, the tfb can be removed any time before or after we set the TCP function block in tcp_newtcpcb. It cannot go away, since we hold a reference count. I guess that is the reason why there was no check...

I agree it won't cause a fatal problem (i.e. panic). But, it could cause problems where it is hard to remove TFBs.

Without the check, we could end up in a situation where:

• Removal code marks TCP_FUNC_BEING_REMOVED.
• Removal code walks TCBs to switch them off the TFB.
• After removal code starts walking TCBs, syncache_socket() creates a new TCB using this TFB. (The removal code's TCB walk won't see this as the new TCB is added to the head of the list the removal code is walking.)
• So, once it is done switching TCBs, removal code will see that TCBs are still using this TFB.

If we had a working, coordinated (i.e. lock-protected or atomic) check for TCP_FUNC_BEING_REMOVED in syncache_socket, this race could not happen.

Whether it is worth adding code to cover this case is another question. I think it depends on whether we expect this to be used primarily as a development feature or a user-facing feature. If the latter, its probably more important it be reliable.

In D45411#1036189, @jtl wrote:

In D45411#1036082, @tuexen wrote:

In D45411#1035980, @jtl wrote:

Should we check for TCP_FUNC_BEING_REMOVED?

I thought about this and did again after you raised the question. The original code didn't.
...
However, since we don't hold the tcp_function_lock, the tfb can be removed any time before or after we set the TCP function block in tcp_newtcpcb. It cannot go away, since we hold a reference count. I guess that is the reason why there was no check...

I agree it won't cause a fatal problem (i.e. panic). But, it could cause problems where it is hard to remove TFBs.

Without the check, we could end up in a situation where:

• Removal code marks TCP_FUNC_BEING_REMOVED.
• Removal code walks TCBs to switch them off the TFB.
• After removal code starts walking TCBs, syncache_socket() creates a new TCB using this TFB. (The removal code's TCB walk won't see this as the new TCB is added to the head of the list the removal code is walking.)
• So, once it is done switching TCBs, removal code will see that TCBs are still using this TFB.

Correct. That can happen. But I think this is not introduced by this patch or am I missing something?

If we had a working, coordinated (i.e. lock-protected or atomic) check for TCP_FUNC_BEING_REMOVED in syncache_socket, this race could not happen.

Are you proposing to hold the tcp_function_lock from before the looking up the TFB until it is set and the inp can be found? Wouldn't that be too long?

Whether it is worth adding code to cover this case is another question. I think it depends on whether we expect this to be used primarily as a development feature or a user-facing feature. If the latter, its probably more important it be reliable.

I think the use case is that you are using some stack (for example the RACK stack) even as the default stack and want to switch all endpoints to a different one (for example the freebsd stack). So I would do this by

1. sudo sysctl net.inet.tcp.functions_default=freebsd
2. sudo tcpsso -s LISTEN -Srack TCP_FUNCTION_BLK freebsd
3. sudo tcpsso -Srack TCP_FUNCTION_BLK freebsd

You might need to run the second and third command multiple time, but after that you should be able to kldunload tcp_rack.

Would the following address your issue:

1. pass the relevant information to tcp_newtcpcb().
2. In tcp_newtcpcb, lock the tcp_function_lock, ensure that the TCP_FUNC_BEING_REMOVED flag is not set (else return NULL), increment the refcount, set the pointer and unlock the tcp_function_lock.

jtl: Does the new version address your concern? It is much smaller and cleaner, so I went ahead and updated the proposed patch.

Thanks! I like this approach. I've added a few comments about potential enhancements.