Re: [PATCH v8 01/26] tcp: authopt: Initial support and key management

From: Leonard Crestez <cdleonard@gmail.com>
To: Eric Dumazet <edumazet@google.com>
Cc: David Ahern <dsahern@kernel.org>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Francesco Ruggeri <fruggeri@arista.com>,
	Salam Noureddine <noureddine@arista.com>,
	Philip Paeps <philip@trouble.is>, Shuah Khan <shuah@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Kuniyuki Iwashima <kuniyu@amazon.co.jp>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Jakub Kicinski <kuba@kernel.org>,
	Yuchung Cheng <ycheng@google.com>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Christoph Paasch <cpaasch@apple.com>,
	Ivan Delalande <colona@arista.com>,
	Caowangbao <caowangbao@huawei.com>,
	Priyaranjan Jha <priyarjha@google.com>,
	netdev <netdev@vger.kernel.org>,
	"open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 01/26] tcp: authopt: Initial support and key management
Date: Wed, 7 Sep 2022 21:09:08 +0300	[thread overview]
Message-ID: <9d26bcb4-b55f-29d5-9790-2a800b22a3a5@gmail.com> (raw)
In-Reply-To: <CANn89i+028SO1q6Hz8E3X7mrzkGSW5mQSLaMj70qka7amsPZ3w@mail.gmail.com>

On 9/7/22 19:28, Eric Dumazet wrote:
> On Wed, Sep 7, 2022 at 9:19 AM Leonard Crestez <cdleonard@gmail.com> wrote:
>>
>> On 9/7/22 01:57, Eric Dumazet wrote:
>>> On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez <cdleonard@gmail.com> wrote:
>>>>
>>>> This commit adds support to add and remove keys but does not use them
>>>> further.
>>>>
>>>> Similar to tcp md5 a single pointer to a struct tcp_authopt_info* struct
>>>> is added to struct tcp_sock, this avoids increasing memory usage. The
>>>> data structures related to tcp_authopt are initialized on setsockopt and
>>>> only freed on socket close.
>>>>
>>>
>>> Thanks Leonard.
>>>
>>> Small points from my side, please find them attached.
>>
>> ...
>>
>>>> +/* Free info and keys.
>>>> + * Don't touch tp->authopt_info, it might not even be assigned yes.
>>>> + */
>>>> +void tcp_authopt_free(struct sock *sk, struct tcp_authopt_info *info)
>>>> +{
>>>> +       kfree_rcu(info, rcu);
>>>> +}
>>>> +
>>>> +/* Free everything and clear tcp_sock.authopt_info to NULL */
>>>> +void tcp_authopt_clear(struct sock *sk)
>>>> +{
>>>> +       struct tcp_authopt_info *info;
>>>> +
>>>> +       info = rcu_dereference_protected(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));
>>>> +       if (info) {
>>>> +               tcp_authopt_free(sk, info);
>>>> +               tcp_sk(sk)->authopt_info = NULL;
>>>
>>> RCU rules at deletion mandate that the pointer must be cleared before
>>> the call_rcu()/kfree_rcu() call.
>>>
>>> It is possible that current MD5 code has an issue here, let's not copy/paste it.
>>
>> OK. Is there a need for some special form of assignment or is current
>> plain form enough?
> 
> It is the right way (when clearing the pointer), no need for another form.

OK

>>>> +/* checks that ipv4 or ipv6 addr matches. */
>>>> +static bool ipvx_addr_match(struct sockaddr_storage *a1,
>>>> +                           struct sockaddr_storage *a2)
>>>> +{
>>>> +       if (a1->ss_family != a2->ss_family)
>>>> +               return false;
>>>> +       if (a1->ss_family == AF_INET &&
>>>> +           (((struct sockaddr_in *)a1)->sin_addr.s_addr !=
>>>> +            ((struct sockaddr_in *)a2)->sin_addr.s_addr))
>>>> +               return false;
>>>> +       if (a1->ss_family == AF_INET6 &&
>>>> +           !ipv6_addr_equal(&((struct sockaddr_in6 *)a1)->sin6_addr,
>>>> +                            &((struct sockaddr_in6 *)a2)->sin6_addr))
>>>> +               return false;
>>>> +       return true;
>>>> +}
>>>
>>> Always surprising to see this kind of generic helper being added in a patch.
>>
>> I remember looking for an equivalent and not finding it. Many places
>> have distinct code paths for ipv4 and ipv6 and my use of
>> "sockaddr_storage" as ipv4/ipv6 union is uncommon.
> 
> inetpeer_addr_cmp() might do it (and we also could fix a bug in it it
> seems, at least for __tcp_get_metrics() usage :/

That uses a different `struct inetpeer_addr` which also has some extra 
"vif" fields for ipv4 that I don't know about.

Everybody seems to be rolling their own ipv4/v6 union, other examples 
are `struct tcp_md5_addr` and `xfrm_address_t`. struct sockaddr_storage 
is "more standard" but also larger so maybe that's why others don't use it.

>>>> +int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
>>>> +{
>>>> +       struct tcp_sock *tp = tcp_sk(sk);
>>>> +       struct tcp_authopt_info *info;
>>>> +
>>>> +       memset(opt, 0, sizeof(*opt));
>>>> +       sock_owned_by_me(sk);
>>>> +
>>>> +       info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk));
>>>
>>> Probably not a big deal, but it seems the prior sock_owned_by_me()
>>> might be redundant.
>>
>> The sock_owned_by_me call checks checks lockdep_sock_is_held
>>
>> The rcu_dereference_check call checks lockdep_sock_is_held ||
>> rcu_read_lock_held()
> 
> Then if you own the socket lock, no need for rcu_dereference_check()
> 
> It could be instead an rcu_dereference_protected(). This is stronger, because
> if your thread no longer owns the socket lock, but is inside
> rcu_read_lock(), we would
> still get a proper lockdep splat.

Ok, I think there are several places where rcu_dereference_check is 
incorrectly used instead of rcu_dereference_protected.