----- On Jun 30, 2020, at 6:38 PM, Eric Dumazet eduma...@google.com wrote:
[...]
>
> For updates of keys, it seems existing code lacks some RCU care.
>
> MD5 keys use RCU for lookups/hashes, but the replacement of a key does
> not allocate a new piece of memory.
How is that RCU-safe ?
Based on what I see here:
tcp_md5_do_add() has a comment stating:
"/* This can be called on a newly created socket, from other files */"
which appears to be untrue if this can indeed be called on a live socket.
The path for pre-existing keys does:
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);
key->keylen = newkeylen;
return 0;
}
AFAIU, this works only if you assume there are no concurrent readers
accessing key->key, else they can see a corrupted key.
The change you are proposing adds smp_wmb/smp_rmb to pair stores
to key before key_len with loads of key_len before key. I'm not sure
what this is trying to achieve, and how it prevents the readers from
observing a corrupted state if the key is updated on a live socket ?
Based on my understanding, this path which deals with pre-existing keys
in-place should only ever be used when there are no concurrent readers,
else a new memory allocation would be needed to guarantee that readers
always observe a valid copy.
Thanks,
Mathieu
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index
> 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..ecc0e3fabce8b03bef823cbfc5c1b0a9e24df124
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4034,9 +4034,12 @@ EXPORT_SYMBOL(tcp_md5_hash_skb_data);
> int tcp_md5_hash_key(struct tcp_md5sig_pool *hp, const struct
> tcp_md5sig_key *key)
> {
> struct scatterlist sg;
> + u8 keylen = key->keylen;
>
> - sg_init_one(&sg, key->key, key->keylen);
> - ahash_request_set_crypt(hp->md5_req, &sg, NULL, key->keylen);
> + 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);
> }
> EXPORT_SYMBOL(tcp_md5_hash_key);
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index
> ad6435ba6d72ffd8caf783bb25cad7ec151d6909..99916fcc15ca0be12c2c133ff40516f79e6fdf7f
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1113,6 +1113,9 @@ int tcp_md5_do_add(struct sock *sk, const union
> tcp_md5_addr *addr,
> if (key) {
> /* Pre-existing entry - just update that one. */
> memcpy(key->key, newkey, newkeylen);
> +
> + smp_wmb(); /* pairs with smp_rmb() in tcp_md5_hash_key() */
> +
> key->keylen = newkeylen;
> return 0;
> }
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
