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

[Leonard Crestez]

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