| Message ID | 20231129165721.337302-5-dima@arista.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4,1/7] Documentation/tcp: Fix an obvious typo | expand |
On Wed, Nov 29, 2023 at 5:57 PM 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. I do not understand this patch. This seems that the clearing should happen at disconnect time, from tcp_disconnect() ? Why forcing user to set a socket option to clear these fields ? > > 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. > > Fixes: 4954f17ddefc ("net/tcp: Introduce TCP_AO setsockopt()s") > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > net/ipv4/tcp_ao.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c > index c8be1d526eac..bf41be6d4721 100644 > --- a/net/ipv4/tcp_ao.c > +++ b/net/ipv4/tcp_ao.c > @@ -1818,8 +1818,16 @@ static int tcp_ao_del_cmd(struct sock *sk, unsigned short int family, > if (!new_rnext) > return -ENOENT; > } > - if (cmd.del_async && sk->sk_state != TCP_LISTEN) > - return -EINVAL; > + if (sk->sk_state == TCP_LISTEN) { > + /* Cleaning up possible "stale" current/rnext keys state, > + * that may have preserved from TCP_CLOSE, before sys_listen() > + */ > + ao_info->current_key = NULL; > + ao_info->rnext_key = NULL; > + } else { > + if (cmd.del_async) > + return -EINVAL; > + } > > if (family == AF_INET) { > struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.addr; > -- > 2.43.0 >
On 11/29/23 17:53, Eric Dumazet wrote: > On Wed, Nov 29, 2023 at 5:57 PM 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. > > I do not understand this patch. > > This seems that the clearing should happen at disconnect time, from > tcp_disconnect() ? Yeah, probably the patch description could have been better. The key here is that TCP_CLOSE may have current/rnext keys: they will be the ones that are used on connect() for 3way handshake. While for TCP_LISTEN it doesn't make any sense to have them (as they otherwise should be per-peer ip/netmask). So, initially I thought of cleaning them up on listen() syscall [1]. But obviously the result was a bit gross. So, I decided to just let userspace delete any keys on TCP_LISTEN by cleaning current/rnext pointers before the checks that don't allow removing current/rnext keys as they are in use by connection. For TCP_CLOSE it's a lesser deal: - the socket may just be closed - alternatively, the user may add a new key and set it as current/rnext and then remove the old key (as it won't be current/rnext anymore). I also should note that currently it's not possible to set/change current/rnext key on TCP_LISTEN, this situation is only a theoretical issue that may be encountered by userspace if it sets those keys by any random reason before listen(): static bool tcp_ao_can_set_current_rnext(struct sock *sk) { /* There aren't current/rnext keys on TCP_LISTEN sockets */ if (sk->sk_state == TCP_LISTEN) return false; return true; } > Why forcing user to set a socket option to clear these fields ? It's just before the checks that disallow removing keys in use: static int tcp_ao_delete_key(struct sock *sk, struct tcp_ao_info *ao_info, bool del_async, struct tcp_ao_key *key, struct tcp_ao_key *new_current, struct tcp_ao_key *new_rnext) { ... if (unlikely(READ_ONCE(ao_info->current_key) == key || READ_ONCE(ao_info->rnext_key) == key)) { err = -EBUSY; goto add_key; } >> 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. >> >> Fixes: 4954f17ddefc ("net/tcp: Introduce TCP_AO setsockopt()s") >> Signed-off-by: Dmitry Safonov <dima@arista.com> >> --- >> net/ipv4/tcp_ao.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c >> index c8be1d526eac..bf41be6d4721 100644 >> --- a/net/ipv4/tcp_ao.c >> +++ b/net/ipv4/tcp_ao.c >> @@ -1818,8 +1818,16 @@ static int tcp_ao_del_cmd(struct sock *sk, unsigned short int family, >> if (!new_rnext) >> return -ENOENT; >> } >> - if (cmd.del_async && sk->sk_state != TCP_LISTEN) >> - return -EINVAL; >> + if (sk->sk_state == TCP_LISTEN) { >> + /* Cleaning up possible "stale" current/rnext keys state, >> + * that may have preserved from TCP_CLOSE, before sys_listen() >> + */ >> + ao_info->current_key = NULL; >> + ao_info->rnext_key = NULL; >> + } else { >> + if (cmd.del_async) >> + return -EINVAL; >> + } >> >> if (family == AF_INET) { >> struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.addr; >> -- >> 2.43.0 >> [1] https://lore.kernel.org/all/CANn89i+xvBQY5HLXNkjW0o9R4SX1hqRisJnr54ZqwuOpEJdHeA@mail.gmail.com/T/#mfd4461bf1dabf6e4b994d85f5191b6cefce337cd Thanks, Dmitry
diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c index c8be1d526eac..bf41be6d4721 100644 --- a/net/ipv4/tcp_ao.c +++ b/net/ipv4/tcp_ao.c @@ -1818,8 +1818,16 @@ static int tcp_ao_del_cmd(struct sock *sk, unsigned short int family, if (!new_rnext) return -ENOENT; } - if (cmd.del_async && sk->sk_state != TCP_LISTEN) - return -EINVAL; + if (sk->sk_state == TCP_LISTEN) { + /* Cleaning up possible "stale" current/rnext keys state, + * that may have preserved from TCP_CLOSE, before sys_listen() + */ + ao_info->current_key = NULL; + ao_info->rnext_key = NULL; + } else { + if (cmd.del_async) + return -EINVAL; + } if (family == AF_INET) { struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.addr;
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. Fixes: 4954f17ddefc ("net/tcp: Introduce TCP_AO setsockopt()s") Signed-off-by: Dmitry Safonov <dima@arista.com> --- net/ipv4/tcp_ao.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)