Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9d26bcb4-b55f-29d5-9790-2a800b22a3a5@gmail.com>
Date:   Wed, 7 Sep 2022 21:09:08 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v8 01/26] tcp: authopt: Initial support and key management
Content-Language: en-US
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>
References: <cover.1662361354.git.cdleonard@gmail.com>
 <0e4c0a98509b907e33c2f80b95cc6cfe713ac2b2.1662361354.git.cdleonard@gmail.com>
 <CANn89i+a0mMUMhUhTPoshifNzzuR_gfThPKptB8cuBiw6Bs5jw@mail.gmail.com>
 <4a47b4ea-750c-a569-5754-4aa0cd5218fc@gmail.com>
 <CANn89i+028SO1q6Hz8E3X7mrzkGSW5mQSLaMj70qka7amsPZ3w@mail.gmail.com>
From:   Leonard Crestez <cdleonard@gmail.com>
In-Reply-To: <CANn89i+028SO1q6Hz8E3X7mrzkGSW5mQSLaMj70qka7amsPZ3w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.