The first step in doing a TCP stack switch is calling the tfb_tcp_handoff_ok() function. If a stack switch is not allowed, an error code is returned, which is then returned to the caller of tcp_ctloutput_set().
In case tcp_ctloutput_set() is called via the sysctl_setsockopt() code path (for example by using tcpsso) and the error code is EAGAIN or EWOULDBLOCK, this results in a loop in userland_sysctl() until the returned error code changes.
To avoid this, lie about error in sysctl_setsockopt() and map EAGAIN to EINVAL.
Details
Details
- Reviewers
timo.voelker_fh-muenster.de glebius rrs nickbanks_netflix.com lstewart peter.lei_ieee.org - Group Reviewers
transport
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
An alternative approach would be to change int (*tfb_tcp_handoff_ok)(struct tcpcb *) to bool (*tfb_tcp_handoff_ok)(struct tcpcb *) and let the caller map false to EINVAL. Would that be clearer? That would also allow the compiler to catch the change in TCP stacks other than BBR and RACK.
Comment Actions
Let tfb_tcp_handoff_ok() return bool. That is simpler and out of tree stacks a caught by the compiler.
Comment Actions
So what happens here is that the KPI of TCP stack switching is being changed under pressure of one of its consumers that has a special handling of EAGAIN. And we are substituting this EAGAIN to factually incorrect EINVAL. No, the argument is valid, the connection exists it is just its state that doesn't allow the switch (supposedly temporarily).
If I remember it correctly, back when tcpsso was introduced I suggested ioctl(2) as we were not yet fully done with Netlink. Note that tcpdrop(8) also uses sysctl :( IMHO, both should be netlinked...
Let's imagine for a second that we don't have this pressure from sysctl(2). We won't do this change to the TCP stack switching KPI and tfb_tcp_handoff_ok would return a spectrum of errors. Thus, I would suggest to confine the workaround to the code that has problems with EAGAIN - to sysctl_setsockopt. Probably tcp_sysctl_setsockopt would be a wrong choice, as we want to workaround all possible EAGAINs, not only this one. So my suggestion is for sysctl_setsockopt to workaround EAGAIN.
Comment Actions
Wait... Maybe we should understand why userland_sysctl() does this and is it needed at all today? It originates from 7a69d9230f834f562d2c5c36c837e67eacdf5ab3. @phk maybe you can comment on this one?
Also in af80b2c90182cbbc0580d237bf3049090d511897 @kib definitely also answered the question "why?" to himself and maybe Kostik can explain.
Comment Actions
See 3ce93e4e80a580b81a4c71723a7bf07f067b1424
I'm not sure how to locate dg's original commit to the stable branch, but it looks like it has something to do with a process exit'ing while we're sysctl'ing it ?
I dont know if anything else relies on that hack today.
Comment Actions
As suggested by Gleb: lie about error in sysctl_setsockopt() to avoid a loop in userland_sysctl().
Comment Actions
I agree with the first sentence. However, I am not sure how important it is to provide different causes back. In the case of RACK, we provide EAGAIN except for the case that SACK is not supported and rack_sack_not_required is false. In this case we return EINVAL. This makes sense because SACK cannot enabled at anytime for this TCP connection. But one could set rack_sack_not_required to true.
I also thought that a function with a name ending of ok should have bool as its return type...
But you are right, this changes the KPI and that is a reason to not fix the problem that way.
If I remember it correctly, back when tcpsso was introduced I suggested ioctl(2) as we were not yet fully done with Netlink. Note that tcpdrop(8) also uses sysctl :( IMHO, both should be netlinked...
tcpsso(8) was modeled after tcpdrop(8). I think they should keep in sync. If you think both should be changed to use netlink-interface instead of the sysctl-interface, that is fine. I have no preference.
I can try to change that, if you prefer.
Let's imagine for a second that we don't have this pressure from sysctl(2). We won't do this change to the TCP stack switching KPI and tfb_tcp_handoff_ok would return a spectrum of errors. Thus, I would suggest to confine the workaround to the code that has problems with EAGAIN - to sysctl_setsockopt. Probably tcp_sysctl_setsockopt would be a wrong choice, as we want to workaround all possible EAGAINs, not only this one. So my suggestion is for sysctl_setsockopt to workaround EAGAIN.
That was also my first fix I tried. However, this is lying about errno and that is what I don't like. That is why I went down other paths. But I really don't like them either. So I change the patch in the review to the one you suggested and I tried initially.
Comment Actions
Syscalls definitely need (and have) a way to retry from the top level. On the other hand, I think that the specific retry in the sysctl handled on EAGAIN probably outlived its use. At least I cannot find a possible use for it in the sysctl kern.proc subtree. It was too long time ago when af80b2c90182cb was written.
If a restart is needed, it is better to route it back to userspace, which is served by ERESTART. So perhaps we should remove the loop from the sysctl.
Comment Actions
I have no objection to removing the sysctl EAGAIN loop, if anything else uses it, that needs to be fixed properly.