Re: [regression] TCP_MD5SIG on established sockets

[Eric Dumazet <edumazet@google.com>]

On Tue, Jun 30, 2020 at 4:44 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Jun 30, 2020, at 6:38 PM, Eric Dumazet edumazet@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.

"This can be called" is not the same than "this is always called for
newly created 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.

This is fine.

>
> 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 ?



By definition if you change the MD5 key on a socket while packets are
flying, the incoming packet could either

1) See old key (packet is dropped)
2) See new key.

So any other decision (catching intermediate state) is really not an
issue, since you already accepted the fact that a packet could be
dropped,
and TCP will retransmit.

TCP MD5 implementation does not support multiple keys  for one flow,
you can not have both old and new keys being checked.


>
> 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.

This is not _needed,_ and since memory allocations can fail, we would
potentially break applications
assuming that changing MD5 key would never fail.

Patch has been sent for review on netdev@ (
https://patchwork.ozlabs.org/project/netdev/patch/20200630234101.3259179-1-edumazet@google.com/
)