Message-ID: <CANn89i+028SO1q6Hz8E3X7mrzkGSW5mQSLaMj70qka7amsPZ3w@mail.gmail.com>
Date:   Wed, 7 Sep 2022 09:28:25 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Leonard Crestez <cdleonard@...il.com>
Cc:     David Ahern <dsahern@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Salam Noureddine <noureddine@...sta.com>,
        Philip Paeps <philip@...uble.is>,
        Shuah Khan <shuah@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.com>,
        Caowangbao <caowangbao@...wei.com>,
        Priyaranjan Jha <priyarjha@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 01/26] tcp: authopt: Initial support and key management

On Wed, Sep 7, 2022 at 9:19 AM Leonard Crestez <cdleonard@...il.com> wrote:
>
> On 9/7/22 01:57, Eric Dumazet wrote:
> > On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez <cdleonard@...il.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.

>
> >
> >> +       }
> >> +}
> >> +
> >> +/* 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 :/

>
> It also wastes some memory.
>
> >> +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.

>
> This is a getsockopt so caller ensures socket locking but
> rcu_read_lock_held() == 0.
>
> The sock_owned_by_me is indeed redundant because it seems very unlikely
> the sockopt calling conditions will be changes. It was mostly there to
> clarify for myself because I had probably at one time with locking
> warnings. I guess they can be removed.
>
> >> +int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
> >> +{
> >> +       struct tcp_authopt_key opt;
> >> +       struct tcp_authopt_info *info;
> >> +       struct tcp_authopt_key_info *key_info, *old_key_info;
> >> +       struct netns_tcp_authopt *net = sock_net_tcp_authopt(sk);
> >> +       int err;
> >> +
> >> +       sock_owned_by_me(sk);
> >> +       if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
> >> +               return -EPERM;
> >> +
> >> +       err = _copy_from_sockptr_tolerant((u8 *)&opt, sizeof(opt), optval, optlen);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       if (opt.flags & ~TCP_AUTHOPT_KEY_KNOWN_FLAGS)
> >> +               return -EINVAL;
> >> +
> >> +       if (opt.keylen > TCP_AUTHOPT_MAXKEYLEN)
> >> +               return -EINVAL;
> >> +
> >> +       /* Delete is a special case: */
> >> +       if (opt.flags & TCP_AUTHOPT_KEY_DEL) {
> >> +               mutex_lock(&net->mutex);
> >> +               key_info = tcp_authopt_key_lookup_exact(sk, net, &opt);
> >> +               if (key_info) {
> >> +                       tcp_authopt_key_del(net, key_info);
> >> +                       err = 0;
> >> +               } else {
> >> +                       err = -ENOENT;
> >> +               }
> >> +               mutex_unlock(&net->mutex);
> >> +               return err;
> >> +       }
> >> +
> >> +       /* check key family */
> >> +       if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
> >> +               if (sk->sk_family != opt.addr.ss_family)
> >> +                       return -EINVAL;
> >> +       }
> >> +
> >> +       /* Initialize tcp_authopt_info if not already set */
> >> +       info = __tcp_authopt_info_get_or_create(sk);
> >> +       if (IS_ERR(info))
> >> +               return PTR_ERR(info);
> >> +
> >> +       key_info = kmalloc(sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
> >
> > kzalloc() ?
>
> Yes
>
> >> +static int tcp_authopt_init_net(struct net *full_net)
> >
> > Hmmm... our convention is to use "struct net *net"
> >
> >> +{
> >> +       struct netns_tcp_authopt *net = &full_net->tcp_authopt;
> >
> > Here, you should use a different name ...
>
> OK, will replace with net_ao
>
> >> @@ -2267,10 +2268,11 @@ void tcp_v4_destroy_sock(struct sock *sk)
> >>                  tcp_clear_md5_list(sk);
> >>                  kfree_rcu(rcu_dereference_protected(tp->md5sig_info, 1), rcu);
> >>                  tp->md5sig_info = NULL;
> >>          }
> >>   #endif
> >> +       tcp_authopt_clear(sk);
> >
> > Do we really own the socket lock at this point ?
>
> Not sure how I would tell but there is a lockdep_sock_is_held check
> inside tcp_authopt_clear. I also added sock_owned_by_me and there were
> no warnings.

Ok then :)

Powered by blists - more mailing lists