| Message ID | 20231121020111.1143180-8-dima@arista.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/7] Documentation/tcp: Fix an obvious typo | expand |
On Tue, Nov 21, 2023 at 3:01 AM Dmitry Safonov <dima@arista.com> wrote: > > This extra check doesn't work for a handshake when SYN segment has > (current_key.maclen != rnext_key.maclen). It could be amended to > preserve rnext_key.maclen instead of current_key.maclen, but that > requires a lookup on listen socket. > > Originally, this extra maclen check was introduced just because it was > cheap. Drop it and convert tcp_request_sock::maclen into boolean > tcp_request_sock::used_tcp_ao. > > Fixes: 06b22ef29591 ("net/tcp: Wire TCP-AO to request sockets") > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > include/linux/tcp.h | 10 ++++------ > net/ipv4/tcp_ao.c | 4 ++-- > net/ipv4/tcp_input.c | 5 +++-- > net/ipv4/tcp_output.c | 9 +++------ > 4 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > index 68f3d315d2e1..3af897b00920 100644 > --- a/include/linux/tcp.h > +++ b/include/linux/tcp.h > @@ -155,6 +155,9 @@ struct tcp_request_sock { > bool req_usec_ts; > #if IS_ENABLED(CONFIG_MPTCP) > bool drop_req; > +#endif > +#ifdef CONFIG_TCP_AO > + bool used_tcp_ao; Why adding another 8bit field here and creating a hole ? > #endif > u32 txhash; > u32 rcv_isn; > @@ -169,7 +172,6 @@ struct tcp_request_sock { > #ifdef CONFIG_TCP_AO > u8 ao_keyid; > u8 ao_rcv_next; > - u8 maclen; Just rename maclen here to used_tcp_ao ? > #endif > }; >
On 11/21/23 08:13, Eric Dumazet wrote: > On Tue, Nov 21, 2023 at 3:01 AM Dmitry Safonov <dima@arista.com> wrote: >> >> This extra check doesn't work for a handshake when SYN segment has >> (current_key.maclen != rnext_key.maclen). It could be amended to >> preserve rnext_key.maclen instead of current_key.maclen, but that >> requires a lookup on listen socket. >> >> Originally, this extra maclen check was introduced just because it was >> cheap. Drop it and convert tcp_request_sock::maclen into boolean >> tcp_request_sock::used_tcp_ao. >> >> Fixes: 06b22ef29591 ("net/tcp: Wire TCP-AO to request sockets") >> Signed-off-by: Dmitry Safonov <dima@arista.com> >> --- >> include/linux/tcp.h | 10 ++++------ >> net/ipv4/tcp_ao.c | 4 ++-- >> net/ipv4/tcp_input.c | 5 +++-- >> net/ipv4/tcp_output.c | 9 +++------ >> 4 files changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/include/linux/tcp.h b/include/linux/tcp.h >> index 68f3d315d2e1..3af897b00920 100644 >> --- a/include/linux/tcp.h >> +++ b/include/linux/tcp.h >> @@ -155,6 +155,9 @@ struct tcp_request_sock { >> bool req_usec_ts; >> #if IS_ENABLED(CONFIG_MPTCP) >> bool drop_req; >> +#endif >> +#ifdef CONFIG_TCP_AO >> + bool used_tcp_ao; > > Why adding another 8bit field here and creating a hole ? Sorry about it, it seems that I (1) checked with CONFIG_MPTCP=n and it seemed like a hole (2) was planning to unify it with other booleans under bitfileds (3) found that some bitfileds require protection as set not only on initialization, so in the end it either should be flags+set_bit() or something well-thought on (that separate bitfileds won't be able to change at the same time) (4) decided to focus on fixing the issue, rather than doing 2 things with the same patch Will fix it up for v2, thanks! > >> #endif >> u32 txhash; >> u32 rcv_isn; >> @@ -169,7 +172,6 @@ struct tcp_request_sock { >> #ifdef CONFIG_TCP_AO >> u8 ao_keyid; >> u8 ao_rcv_next; >> - u8 maclen; > > Just rename maclen here to used_tcp_ao ? > >> #endif >> }; >> Thanks, Dmitry
diff --git a/include/linux/tcp.h b/include/linux/tcp.h index 68f3d315d2e1..3af897b00920 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -155,6 +155,9 @@ struct tcp_request_sock { bool req_usec_ts; #if IS_ENABLED(CONFIG_MPTCP) bool drop_req; +#endif +#ifdef CONFIG_TCP_AO + bool used_tcp_ao; #endif u32 txhash; u32 rcv_isn; @@ -169,7 +172,6 @@ struct tcp_request_sock { #ifdef CONFIG_TCP_AO u8 ao_keyid; u8 ao_rcv_next; - u8 maclen; #endif }; @@ -180,14 +182,10 @@ static inline struct tcp_request_sock *tcp_rsk(const struct request_sock *req) static inline bool tcp_rsk_used_ao(const struct request_sock *req) { - /* The real length of MAC is saved in the request socket, - * signing anything with zero-length makes no sense, so here is - * a little hack.. - */ #ifndef CONFIG_TCP_AO return false; #else - return tcp_rsk(req)->maclen != 0; + return tcp_rsk(req)->used_tcp_ao; #endif } diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c index 9b7f1970c2e9..07221319e8c5 100644 --- a/net/ipv4/tcp_ao.c +++ b/net/ipv4/tcp_ao.c @@ -851,7 +851,7 @@ void tcp_ao_syncookie(struct sock *sk, const struct sk_buff *skb, const struct tcp_ao_hdr *aoh; struct tcp_ao_key *key; - treq->maclen = 0; + treq->used_tcp_ao = false; if (tcp_parse_auth_options(th, NULL, &aoh) || !aoh) return; @@ -863,7 +863,7 @@ void tcp_ao_syncookie(struct sock *sk, const struct sk_buff *skb, treq->ao_rcv_next = aoh->keyid; treq->ao_keyid = aoh->rnext_keyid; - treq->maclen = tcp_ao_maclen(key); + treq->used_tcp_ao = true; } static enum skb_drop_reason diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 78896c8be0d4..89cb6912dd91 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -7182,11 +7182,12 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, if (tcp_parse_auth_options(tcp_hdr(skb), NULL, &aoh)) goto drop_and_release; /* Invalid TCP options */ if (aoh) { - tcp_rsk(req)->maclen = aoh->length - sizeof(struct tcp_ao_hdr); + tcp_rsk(req)->used_tcp_ao = true; tcp_rsk(req)->ao_rcv_next = aoh->keyid; tcp_rsk(req)->ao_keyid = aoh->rnext_keyid; + } else { - tcp_rsk(req)->maclen = 0; + tcp_rsk(req)->used_tcp_ao = false; } #endif tcp_rsk(req)->snt_isn = isn; diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 93eef1dbbc55..f5ef15e1d9ac 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3720,7 +3720,6 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst, if (tcp_rsk_used_ao(req)) { #ifdef CONFIG_TCP_AO struct tcp_ao_key *ao_key = NULL; - u8 maclen = tcp_rsk(req)->maclen; u8 keyid = tcp_rsk(req)->ao_keyid; ao_key = tcp_sk(sk)->af_specific->ao_lookup(sk, req_to_sk(req), @@ -3730,13 +3729,11 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst, * for another peer-matching key, but the peer has requested * ao_keyid (RFC5925 RNextKeyID), so let's keep it simple here. */ - if (unlikely(!ao_key || tcp_ao_maclen(ao_key) != maclen)) { - u8 key_maclen = ao_key ? tcp_ao_maclen(ao_key) : 0; - + if (unlikely(!ao_key)) { rcu_read_unlock(); kfree_skb(skb); - net_warn_ratelimited("TCP-AO: the keyid %u with maclen %u|%u from SYN packet is not present - not sending SYNACK\n", - keyid, maclen, key_maclen); + net_warn_ratelimited("TCP-AO: the keyid %u from SYN packet is not present - not sending SYNACK\n", + keyid); return NULL; } key.ao_key = ao_key;
This extra check doesn't work for a handshake when SYN segment has (current_key.maclen != rnext_key.maclen). It could be amended to preserve rnext_key.maclen instead of current_key.maclen, but that requires a lookup on listen socket. Originally, this extra maclen check was introduced just because it was cheap. Drop it and convert tcp_request_sock::maclen into boolean tcp_request_sock::used_tcp_ao. Fixes: 06b22ef29591 ("net/tcp: Wire TCP-AO to request sockets") Signed-off-by: Dmitry Safonov <dima@arista.com> --- include/linux/tcp.h | 10 ++++------ net/ipv4/tcp_ao.c | 4 ++-- net/ipv4/tcp_input.c | 5 +++-- net/ipv4/tcp_output.c | 9 +++------ 4 files changed, 12 insertions(+), 16 deletions(-)