On 9/7/22 19:28, Eric Dumazet wrote:

On Wed, Sep 7, 2022 at 9:19 AM Leonard Crestez [off-list ref] wrote:

quoted

On 9/7/22 01:57, Eric Dumazet wrote:

quoted

On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez [off-list ref] wrote:

quoted

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.

...

quoted

quoted

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

quoted

quoted

quoted

+/* 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.

quoted

quoted

quoted

+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.