From: Dmitry Antipov <dmantipov@xxxxxxxxx>
Date: Tue, 30 Jul 2024 16:46:05 +0300
> Hm. Note both 'reuseport_add_sock()' and 'reuseport_detach_sock()' uses
> global 'reuseport_lock' internally. So what about using read lock to protect
> 'sctp_for_each_entry()' loop and upgrade to write lock only if hash bucket
> should be actually updated? E.g.:
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 17fcaa9b0df9..4fbff388b1b4 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -735,6 +735,7 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
> struct sock *sk = ep->base.sk;
> struct net *net = sock_net(sk);
> struct sctp_hashbucket *head;
> + int err = 0;
>
> ep->hashent = sctp_ep_hashfn(net, ep->base.bind_addr.port);
> head = &sctp_ep_hashtable[ep->hashent];
> @@ -743,11 +744,14 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
> bool any = sctp_is_ep_boundall(sk);
> struct sctp_endpoint *ep2;
> struct list_head *list;
> - int cnt = 0, err = 1;
> + int cnt = 0;
> +
> + err = 1;
>
> list_for_each(list, &ep->base.bind_addr.address_list)
> cnt++;
>
> + read_lock(&head->lock);
> sctp_for_each_hentry(ep2, &head->chain) {
> struct sock *sk2 = ep2->base.sk;
>
> @@ -760,25 +764,30 @@ static int __sctp_hash_endpoint(struct sctp_endpoint *ep)
> sctp_sk(sk), cnt);
> if (!err) {
> err = reuseport_add_sock(sk, sk2, any);
> - if (err)
> - return err;
> + if (err) {
> + read_unlock(&head->lock);
> + goto out;
> + }
> break;
> } else if (err < 0) {
> - return err;
> + read_unlock(&head->lock);
> + goto out;
> }
> }
> + read_unlock(&head->lock);
>
> if (err) {
> err = reuseport_alloc(sk, any);
What happens if two sockets matching each other reach here ?
There would be no error but the first hashed socket will be silently
dead as lookup stops at the 2nd sk ?
> if (err)
> - return err;
> + goto out;
> }
> }
>
> write_lock(&head->lock);
> hlist_add_head(&ep->node, &head->chain);
> write_unlock(&head->lock);
> - return 0;
> +out:
> + return err;
> }
>
> /* Add an endpoint to the hash. Local BH-safe. */
>
> Dmitry
