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: <589e17df-e321-c8ad-5360-e286c10cb1a3@gmail.com>
Date:   Thu, 8 Sep 2022 13:47:23 +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:     Paolo Abeni <pabeni@redhat.com>, David Ahern <dsahern@kernel.org>,
        Eric Dumazet <edumazet@google.com>
Cc:     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@vger.kernel.org,
        linux-crypto@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org, Dmitry Safonov <0x7f454c46@gmail.com>
References: <cover.1662361354.git.cdleonard@gmail.com>
 <0e4c0a98509b907e33c2f80b95cc6cfe713ac2b2.1662361354.git.cdleonard@gmail.com>
 <9bb98d13313d2ebeb5804d67285e8e6320ce4e74.camel@redhat.com>
From:   Leonard Crestez <cdleonard@gmail.com>
In-Reply-To: <9bb98d13313d2ebeb5804d67285e8e6320ce4e74.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/8/22 09:35, Paolo Abeni wrote:
> On Mon, 2022-09-05 at 10:05 +0300, Leonard Crestez wrote:
> [...]
>> diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
>> new file mode 100644
>> index 000000000000..d38e9c89c89d
>> --- /dev/null
>> +++ b/net/ipv4/tcp_authopt.c
>> @@ -0,0 +1,317 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +#include <net/tcp_authopt.h>
>> +#include <net/ipv6.h>
>> +#include <net/tcp.h>
>> +#include <linux/kref.h>
>> +
>> +/* This is enabled when first struct tcp_authopt_info is allocated and never released */
>> +DEFINE_STATIC_KEY_FALSE(tcp_authopt_needed_key);
>> +EXPORT_SYMBOL(tcp_authopt_needed_key);
>> +
>> +static inline struct netns_tcp_authopt *sock_net_tcp_authopt(const struct sock *sk)
>> +{
>> +	return &sock_net(sk)->tcp_authopt;
>> +}
> 
> Please have a look at PW report for this series, there are a bunch of
> issues to be addressed, e.g. above 'static inline' should be just
> 'static'

What is a "PW report"? I can't find any info about this.

>> +static void tcp_authopt_key_release_kref(struct kref *ref)
>> +{
>> +	struct tcp_authopt_key_info *key = container_of(ref, struct tcp_authopt_key_info, ref);
>> +
>> +	kfree_rcu(key, rcu);
>> +}
>> +
>> +static void tcp_authopt_key_put(struct tcp_authopt_key_info *key)
>> +{
>> +	if (key)
>> +		kref_put(&key->ref, tcp_authopt_key_release_kref);
>> +}
>> +
>> +static void tcp_authopt_key_del(struct netns_tcp_authopt *net,
>> +				struct tcp_authopt_key_info *key)
>> +{
>> +	lockdep_assert_held(&net->mutex);
>> +	hlist_del_rcu(&key->node);
>> +	key->flags |= TCP_AUTHOPT_KEY_DEL;
>> +	kref_put(&key->ref, tcp_authopt_key_release_kref);
>> +}
>> +
>> +/* 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)
> 
> this need to be 'static'.

Tried this and it's later called from tcp_twsk_destructor.

> I'm sorry to bring the next topic this late (If already discussed, I
> missed that point), is possible to split this series in smaller chunks?

It's already 26 patches and 3675 added lines, less that 150 lines per 
patch seems reasonable?

The split is already somewhat artificial, for example there are patches 
that "add crypto" without actually using it because then it would be too 
large.

Some features could be dropped for later in order to make this smaller, 
for example TCP_REPAIR doesn't have many usecases. Features like 
prefixlen, vrf binding and ipv4-mapped-ipv6 were explicitly requested by 
maintainers so I included them as separate patches in the main series.

--
Regards,
Leonard