Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jun 30, 2020, at 8:55 PM, Eric Dumazet edumazet@google.com wrote:

> On Tue, Jun 30, 2020 at 5:53 PM Hideaki Yoshifuji
> <hideaki.yoshifuji@miraclelinux.com> wrote:
>>
>> Hi,
>>
>> 2020年7月1日(水) 8:41 Eric Dumazet <edumazet@google.com>:
>> :
>> > We only want to make sure that in the case key->keylen
>> > is changed, cpus in tcp_md5_hash_key() wont try to use
>> > uninitialized data, or crash because key->keylen was
>> > read twice to feed sg_init_one() and ahash_request_set_crypt()
>> >
>> > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock")
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> > ---
>> >  net/ipv4/tcp.c      | 7 +++++--
>> >  net/ipv4/tcp_ipv4.c | 3 +++
>> >  2 files changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> > index
>> > 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263
>> > 100644
>> > --- a/net/ipv4/tcp.c
>> > +++ b/net/ipv4/tcp.c
>> > @@ -4033,10 +4033,13 @@ 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;
>> >         struct scatterlist sg;
>>
>> ACCESS_ONCE here, no?
> 
> Not needed, the smp_rmb() barrier is stronger.

ACCESS_ONCE() is now deprecated in favor of READ_ONCE()/WRITE_ONCE(),
which are needed here. smp_rmb()/smp_wmb() are needed in addition, but
have a different purpose. smp_rmb() only guarantees ordering, but does
nothing to prevent the compiler from turning the load into something
unexpected. Likewise for WRITE_ONCE. See linux/compiler.h READ_ONCE and
WRITE_ONCE comment:

"2) Ensuring that the compiler does not fold, spindle, or otherwise
mutilate accesses that either do not require ordering or that interact
with an explicit memory barrier or atomic instruction that provides the
required ordering."

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com