| Message ID | 20220726115743.2759832-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e62d2e110356093c034998e093675df83057e511 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tcp: md5: fix IPv4-mapped support | expand |
On 7/26/22 5:57 AM, Eric Dumazet wrote: > After the blamed commit, IPv4 SYN packets handled > by a dual stack IPv6 socket are dropped, even if > perfectly valid. > > $ nstat | grep MD5 > TcpExtTCPMD5Failure 5 0.0 > > For a dual stack listener, an incoming IPv4 SYN packet > would call tcp_inbound_md5_hash() with @family == AF_INET, > while tp->af_specific is pointing to tcp_sock_ipv6_specific. > > Only later when an IPv4-mapped child is created, tp->af_specific > is changed to tcp_sock_ipv6_mapped_specific. > > Fixes: 7bbb765b7349 ("net/tcp: Merge TCP-MD5 inbound callbacks") > Reported-by: Brian Vazquez <brianvv@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: David Ahern <dsahern@kernel.org> > Cc: Leonard Crestez <cdleonard@gmail.com> > --- > net/ipv4/tcp.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 002a4a04efbe076ba603d7d42eb85e60d9bf4fb8..766881775abb795c884d048d51c361e805b91989 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4459,9 +4459,18 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb, > return SKB_DROP_REASON_TCP_MD5UNEXPECTED; > } > > - /* check the signature */ > - genhash = tp->af_specific->calc_md5_hash(newhash, hash_expected, > - NULL, skb); > + /* Check the signature. > + * To support dual stack listeners, we need to handle > + * IPv4-mapped case. > + */ > + if (family == AF_INET) > + genhash = tcp_v4_md5_hash_skb(newhash, > + hash_expected, > + NULL, skb); > + else > + genhash = tp->af_specific->calc_md5_hash(newhash, > + hash_expected, > + NULL, skb); > > if (genhash || memcmp(hash_location, newhash, 16) != 0) { > NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE); We should get v4-mapped cases added to the fcnal-test.sh permutations. Reviewed-by: David Ahern <dsahern@kernel.org>
On 7/26/22 12:57, Eric Dumazet wrote: > After the blamed commit, IPv4 SYN packets handled > by a dual stack IPv6 socket are dropped, even if > perfectly valid. > > $ nstat | grep MD5 > TcpExtTCPMD5Failure 5 0.0 > > For a dual stack listener, an incoming IPv4 SYN packet > would call tcp_inbound_md5_hash() with @family == AF_INET, > while tp->af_specific is pointing to tcp_sock_ipv6_specific. > > Only later when an IPv4-mapped child is created, tp->af_specific > is changed to tcp_sock_ipv6_mapped_specific. Makes sense. Sorry, I didn't spot it at that moment. > > Fixes: 7bbb765b7349 ("net/tcp: Merge TCP-MD5 inbound callbacks") > Reported-by: Brian Vazquez <brianvv@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: David Ahern <dsahern@kernel.org> > Cc: Leonard Crestez <cdleonard@gmail.com> Reviewed-by: Dmitry Safonov <dima@arista.com> Thanks, Dmitry
On Tue, Jul 26, 2022 at 5:06 PM David Ahern <dsahern@kernel.org> wrote: > > On 7/26/22 5:57 AM, Eric Dumazet wrote: > > After the blamed commit, IPv4 SYN packets handled > > by a dual stack IPv6 socket are dropped, even if > > perfectly valid. > > > > $ nstat | grep MD5 > > TcpExtTCPMD5Failure 5 0.0 > > > > For a dual stack listener, an incoming IPv4 SYN packet > > would call tcp_inbound_md5_hash() with @family == AF_INET, > > while tp->af_specific is pointing to tcp_sock_ipv6_specific. > > > > Only later when an IPv4-mapped child is created, tp->af_specific > > is changed to tcp_sock_ipv6_mapped_specific. > > > > Fixes: 7bbb765b7349 ("net/tcp: Merge TCP-MD5 inbound callbacks") > > Reported-by: Brian Vazquez <brianvv@google.com> > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Dmitry Safonov <dima@arista.com> > > Cc: David Ahern <dsahern@kernel.org> > > Cc: Leonard Crestez <cdleonard@gmail.com> > > --- > > net/ipv4/tcp.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 002a4a04efbe076ba603d7d42eb85e60d9bf4fb8..766881775abb795c884d048d51c361e805b91989 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -4459,9 +4459,18 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb, > > return SKB_DROP_REASON_TCP_MD5UNEXPECTED; > > } > > > > - /* check the signature */ > > - genhash = tp->af_specific->calc_md5_hash(newhash, hash_expected, > > - NULL, skb); > > + /* Check the signature. > > + * To support dual stack listeners, we need to handle > > + * IPv4-mapped case. > > + */ > > + if (family == AF_INET) > > + genhash = tcp_v4_md5_hash_skb(newhash, > > + hash_expected, > > + NULL, skb); > > + else > > + genhash = tp->af_specific->calc_md5_hash(newhash, > > + hash_expected, > > + NULL, skb); > > > > if (genhash || memcmp(hash_location, newhash, 16) != 0) { > > NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE); > > We should get v4-mapped cases added to the fcnal-test.sh permutations. > > Reviewed-by: David Ahern <dsahern@kernel.org> Although fcnal-test.sh uses ~45 minutes currently :/ Maybe we should make it multi netns and multi threaded to speed up things. And/or replace various "sleep 1" with more appropriate sync to make this faster and not flaky in case of system load.
On 7/26/22 11:57 AM, Eric Dumazet wrote: > Although fcnal-test.sh uses ~45 minutes currently :/ > Maybe we should make it multi netns and multi threaded to speed up things. > > And/or replace various "sleep 1" with more appropriate sync to make > this faster and not flaky in case of system load. There are currently 700+ permutations (800+ if Mike's vrf patch is only fcnal-test). That's why the script takes a `-t TEST` argument - to only run a subset. nettest now has the capability for 1 command to run both client and server in different namespaces. I have a branch that did the conversion of fcnal-test.sh; validating the output to ensure no degradation in test results (not just pass / fail but tests "fail" (negative tests) for the right reason) took more time than I had. In the end it did not shorten the test time by any significant margin so lost the motivation to wade through the output on the before and after.
On 7/26/22 14:57, Eric Dumazet wrote: > After the blamed commit, IPv4 SYN packets handled > by a dual stack IPv6 socket are dropped, even if > perfectly valid. > > $ nstat | grep MD5 > TcpExtTCPMD5Failure 5 0.0 > > For a dual stack listener, an incoming IPv4 SYN packet > would call tcp_inbound_md5_hash() with @family == AF_INET, > while tp->af_specific is pointing to tcp_sock_ipv6_specific. > > Only later when an IPv4-mapped child is created, tp->af_specific > is changed to tcp_sock_ipv6_mapped_specific. > > Fixes: 7bbb765b7349 ("net/tcp: Merge TCP-MD5 inbound callbacks") > Reported-by: Brian Vazquez <brianvv@google.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Dmitry Safonov <dima@arista.com> > Cc: David Ahern <dsahern@kernel.org> > Cc: Leonard Crestez <cdleonard@gmail.com> I had a test in this area for AO and MD5 but it was incorrect (it did not actually use an ipv4-mapped-ipv6 address for the ipv6 socket, it used an ipv6 wildcard address). After fixing the test I can confirm that this patch does in fact fix something. https://github.com/cdleonard/tcp-authopt-test/commit/662a6a7e1a818f4581fc0055e821bc1b4c8d04e8 Tested-by: Leonard Crestez <cdleonard@gmail.com> > --- > net/ipv4/tcp.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 002a4a04efbe076ba603d7d42eb85e60d9bf4fb8..766881775abb795c884d048d51c361e805b91989 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4459,9 +4459,18 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb, > return SKB_DROP_REASON_TCP_MD5UNEXPECTED; > } > > - /* check the signature */ > - genhash = tp->af_specific->calc_md5_hash(newhash, hash_expected, > - NULL, skb); > + /* Check the signature. > + * To support dual stack listeners, we need to handle > + * IPv4-mapped case. > + */ > + if (family == AF_INET) > + genhash = tcp_v4_md5_hash_skb(newhash, > + hash_expected, > + NULL, skb); > + else > + genhash = tp->af_specific->calc_md5_hash(newhash, > + hash_expected, > + NULL, skb); > > if (genhash || memcmp(hash_location, newhash, 16) != 0) { > NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE);
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 26 Jul 2022 11:57:43 +0000 you wrote: > After the blamed commit, IPv4 SYN packets handled > by a dual stack IPv6 socket are dropped, even if > perfectly valid. > > $ nstat | grep MD5 > TcpExtTCPMD5Failure 5 0.0 > > [...] Here is the summary with links: - [net] tcp: md5: fix IPv4-mapped support https://git.kernel.org/netdev/net/c/e62d2e110356 You are awesome, thank you!
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 002a4a04efbe076ba603d7d42eb85e60d9bf4fb8..766881775abb795c884d048d51c361e805b91989 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4459,9 +4459,18 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb, return SKB_DROP_REASON_TCP_MD5UNEXPECTED; } - /* check the signature */ - genhash = tp->af_specific->calc_md5_hash(newhash, hash_expected, - NULL, skb); + /* Check the signature. + * To support dual stack listeners, we need to handle + * IPv4-mapped case. + */ + if (family == AF_INET) + genhash = tcp_v4_md5_hash_skb(newhash, + hash_expected, + NULL, skb); + else + genhash = tp->af_specific->calc_md5_hash(newhash, + hash_expected, + NULL, skb); if (genhash || memcmp(hash_location, newhash, 16) != 0) { NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE);
After the blamed commit, IPv4 SYN packets handled by a dual stack IPv6 socket are dropped, even if perfectly valid. $ nstat | grep MD5 TcpExtTCPMD5Failure 5 0.0 For a dual stack listener, an incoming IPv4 SYN packet would call tcp_inbound_md5_hash() with @family == AF_INET, while tp->af_specific is pointing to tcp_sock_ipv6_specific. Only later when an IPv4-mapped child is created, tp->af_specific is changed to tcp_sock_ipv6_mapped_specific. Fixes: 7bbb765b7349 ("net/tcp: Merge TCP-MD5 inbound callbacks") Reported-by: Brian Vazquez <brianvv@google.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Dmitry Safonov <dima@arista.com> Cc: David Ahern <dsahern@kernel.org> Cc: Leonard Crestez <cdleonard@gmail.com> --- net/ipv4/tcp.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)