* [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers
@ 2020-07-01 18:43 Eric Dumazet
2020-07-01 18:48 ` Mathieu Desnoyers
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Eric Dumazet @ 2020-07-01 18:43 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Mathieu Desnoyers,
Herbert Xu, Marco Elver
My prior fix went a bit too far, according to Herbert and Mathieu.
Since we accept that concurrent TCP MD5 lookups might see inconsistent
keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
Clearing all key->key[] is needed to avoid possible KMSAN reports,
if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
data_race() was added in linux-5.8 and will prevent KCSAN reports,
this can safely be removed in stable backports, if data_race() is
not yet backported.
v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add()
Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Marco Elver <elver@google.com>
---
net/ipv4/tcp.c | 8 ++++----
net/ipv4/tcp_ipv4.c | 19 ++++++++++++++-----
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f111660453241692a17c881dd6dc2910a1236263..c33f7c6aff8eea81d374644cd251bd2b96292651 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4033,14 +4033,14 @@ EXPORT_SYMBOL(tcp_md5_hash_skb_data);
int tcp_md5_hash_key(struct tcp_md5sig_pool *hp, const struct tcp_md5sig_key *key)
{
- u8 keylen = key->keylen;
+ u8 keylen = READ_ONCE(key->keylen); /* paired with WRITE_ONCE() in tcp_md5_do_add */
struct scatterlist sg;
- smp_rmb(); /* paired with smp_wmb() in tcp_md5_do_add() */
-
sg_init_one(&sg, key->key, keylen);
ahash_request_set_crypt(hp->md5_req, &sg, NULL, keylen);
- return crypto_ahash_update(hp->md5_req);
+
+ /* We use data_race() because tcp_md5_do_add() might change key->key under us */
+ return data_race(crypto_ahash_update(hp->md5_req));
}
EXPORT_SYMBOL(tcp_md5_hash_key);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 99916fcc15ca0be12c2c133ff40516f79e6fdf7f..04bfcbbfee83aadf5bca0332275c57113abdbc75 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1111,12 +1111,21 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index);
if (key) {
- /* Pre-existing entry - just update that one. */
- memcpy(key->key, newkey, newkeylen);
+ /* Pre-existing entry - just update that one.
+ * Note that the key might be used concurrently.
+ * data_race() is telling kcsan that we do not care of
+ * key mismatches, since changing MD5 key on live flows
+ * can lead to packet drops.
+ */
+ data_race(memcpy(key->key, newkey, newkeylen));
- smp_wmb(); /* pairs with smp_rmb() in tcp_md5_hash_key() */
+ /* Pairs with READ_ONCE() in tcp_md5_hash_key().
+ * Also note that a reader could catch new key->keylen value
+ * but old key->key[], this is the reason we use __GFP_ZERO
+ * at sock_kmalloc() time below these lines.
+ */
+ WRITE_ONCE(key->keylen, newkeylen);
- key->keylen = newkeylen;
return 0;
}
@@ -1132,7 +1141,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
rcu_assign_pointer(tp->md5sig_info, md5sig);
}
- key = sock_kmalloc(sk, sizeof(*key), gfp);
+ key = sock_kmalloc(sk, sizeof(*key), gfp | __GFP_ZERO);
if (!key)
return -ENOMEM;
if (!tcp_alloc_md5sig_pool()) {
--
2.27.0.212.ge8ba1cc988-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers
2020-07-01 18:43 [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers Eric Dumazet
@ 2020-07-01 18:48 ` Mathieu Desnoyers
2020-07-01 21:37 ` Herbert Xu
2020-07-02 0:31 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2020-07-01 18:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, netdev, Eric Dumazet, Herbert Xu, Marco Elver
----- On Jul 1, 2020, at 2:43 PM, Eric Dumazet edumazet@google.com wrote:
> My prior fix went a bit too far, according to Herbert and Mathieu.
>
> Since we accept that concurrent TCP MD5 lookups might see inconsistent
> keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
>
> Clearing all key->key[] is needed to avoid possible KMSAN reports,
> if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
> using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
>
> data_race() was added in linux-5.8 and will prevent KCSAN reports,
> this can safely be removed in stable backports, if data_race() is
> not yet backported.
>
> v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add()
>
> Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in
> tcp_md5_do_add()/tcp_md5_hash_key()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Thanks !
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers
2020-07-01 18:43 [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers Eric Dumazet
2020-07-01 18:48 ` Mathieu Desnoyers
@ 2020-07-01 21:37 ` Herbert Xu
2020-07-02 0:31 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Herbert Xu @ 2020-07-01 21:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, netdev, Eric Dumazet, Mathieu Desnoyers, Marco Elver
On Wed, Jul 01, 2020 at 11:43:04AM -0700, Eric Dumazet wrote:
> My prior fix went a bit too far, according to Herbert and Mathieu.
>
> Since we accept that concurrent TCP MD5 lookups might see inconsistent
> keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
>
> Clearing all key->key[] is needed to avoid possible KMSAN reports,
> if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
> using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
>
> data_race() was added in linux-5.8 and will prevent KCSAN reports,
> this can safely be removed in stable backports, if data_race() is
> not yet backported.
>
> v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add()
>
> Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Marco Elver <elver@google.com>
> ---
> net/ipv4/tcp.c | 8 ++++----
> net/ipv4/tcp_ipv4.c | 19 ++++++++++++++-----
> 2 files changed, 18 insertions(+), 9 deletions(-)
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers
2020-07-01 18:43 [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers Eric Dumazet
2020-07-01 18:48 ` Mathieu Desnoyers
2020-07-01 21:37 ` Herbert Xu
@ 2020-07-02 0:31 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2020-07-02 0:31 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, mathieu.desnoyers, herbert, elver
From: Eric Dumazet <edumazet@google.com>
Date: Wed, 1 Jul 2020 11:43:04 -0700
> My prior fix went a bit too far, according to Herbert and Mathieu.
>
> Since we accept that concurrent TCP MD5 lookups might see inconsistent
> keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
>
> Clearing all key->key[] is needed to avoid possible KMSAN reports,
> if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
> using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
>
> data_race() was added in linux-5.8 and will prevent KCSAN reports,
> this can safely be removed in stable backports, if data_race() is
> not yet backported.
>
> v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add()
>
> Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-02 0:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-01 18:43 [PATCH v2 net] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers Eric Dumazet
2020-07-01 18:48 ` Mathieu Desnoyers
2020-07-01 21:37 ` Herbert Xu
2020-07-02 0:31 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.