Messages in this thread | | | Date | Wed, 22 Nov 2023 01:00:03 +0000 | Subject | Re: [PATCH 4/7] net/tcp: Reset TCP-AO cached keys on listen() syscall | From | Dmitry Safonov <> |
| |
On 11/21/23 08:18, Eric Dumazet wrote: > On Tue, Nov 21, 2023 at 3:01 AM Dmitry Safonov <dima@arista.com> wrote: >> >> TCP_LISTEN sockets are not connected to any peer, so having >> current_key/rnext_key doesn't make sense. >> >> The userspace may falter over this issue by setting current or rnext >> TCP-AO key before listen() syscall. setsockopt(TCP_AO_DEL_KEY) doesn't >> allow removing a key that is in use (in accordance to RFC 5925), so >> it might be inconvenient to have keys that can be destroyed only with >> listener socket. > > I think this is the wrong way to solve this issue. listen() should not > mess with anything else than socket state. > >> >> Fixes: 4954f17ddefc ("net/tcp: Introduce TCP_AO setsockopt()s") >> Signed-off-by: Dmitry Safonov <dima@arista.com> [..] >> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c >> index fb81de10d332..a08d1266344f 100644 >> --- a/net/ipv4/af_inet.c >> +++ b/net/ipv4/af_inet.c >> @@ -200,6 +200,7 @@ int __inet_listen_sk(struct sock *sk, int backlog) >> * we can only allow the backlog to be adjusted. >> */ >> if (old_state != TCP_LISTEN) { >> + tcp_ao_listen(sk); > > Ouch... > > Please add your hook in tcp_disconnect() instead of this layering violation. > > I think you missed the fact that applications can call listen(fd, > backlog) multiple times, > if they need to dynamically adjust backlog.
Hmm, unsure, I've probably failed at describing the issue or failing to understand your reply :-)
Let me try again: 1. sk = socket(AF_*, SOCK_STREAM, IPPROTO_TCP) 2. setsockopt(sk, TCP_AO_ADD_KEY, ...) - adding a key to use later 3. setsockopt(sk, IPPROTO_TCP, TCP_AO_INFO, set_current=1) - could be done straight on adding a key at (2), but for an example, explicitely 4.a. connect(sk, peer) - all as expected, the current key will be the one that is used for SYN (and ending ACK if the peer doesn't request to switch) 4.b listen(sk, ...) - userspace shoots itself in foot: the current_key has no usage on TCP_LISTEN, so it just "hangs" as a pointer until the socket gets destroyed.
An alternative fix would be to make setsockopt(TCP_AO_DEL_KEY) remove a key even if it's current_key on TCP_LISTEN, re-setting that to NULL.
Now as I described, somewhat feeling like the alternative fix sounds better. Will proceed with that for v2.
Thanks, Dmitry
| |