Hi, [CCed netdev] On Sun, Feb 12, 2023 at 10:38:40PM -0500, Winter wrote: > Hi all, > > I'm facing the same issue as > https://lore.kernel.org/stable/CAFsF8vL4CGFzWMb38_XviiEgxoKX0GYup=JiUFXUOmagdk9CRg@xxxxxxxxxxxxxx/, > but on 5.15. I've bisected it across releases to 5.15.88, and can reproduce > on 5.15.93. > > However, I cannot seem to find the identified problematic commit in the 5.15 > branch, so I'm unsure if this is a different issue or not. > > There's a few ways to reproduce this issue, but the one I've been using is > running libuv's (https://github.com/libuv/libuv) tests, specifically tests > 271 and 277. >From the linked patch: https://lore.kernel.org/stable/20221228144337.512799851@xxxxxxxxxxxxxxxxxxx/ I can see that: We assume the correct errno is -EADDRINUSE when sk->sk_prot->get_port() fails, so some ->get_port() functions return just 1 on failure and the callers return -EADDRINUSE instead. However, mptcp_get_port() can return -EINVAL. Let's not ignore the error. Note the only exception is inet_autobind(), all of whose callers return -EAGAIN instead. But the patch doesn't do what is documented, it preserves all return values and will happily return 1 if ->get_port() returns 1: > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -522,9 +522,9 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len, > /* Make sure we are allowed to bind here. */ > if (snum || !(inet->bind_address_no_port || > (flags & BIND_FORCE_ADDRESS_NO_PORT))) { > - if (sk->sk_prot->get_port(sk, snum)) { > + err = sk->sk_prot->get_port(sk, snum); > + if (err) { > inet->inet_saddr = inet->inet_rcv_saddr = 0; > - err = -EADDRINUSE; > goto out_release_sock; > } > if (!(flags & BIND_FROM_BPF)) { > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c > index eb31c7158b39..971969cc7e17 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -1041,7 +1041,7 @@ int inet_csk_listen_start(struct sock *sk) > { > struct inet_connection_sock *icsk = inet_csk(sk); > struct inet_sock *inet = inet_sk(sk); > - int err = -EADDRINUSE; > + int err; > > reqsk_queue_alloc(&icsk->icsk_accept_queue); > > @@ -1057,7 +1057,8 @@ int inet_csk_listen_start(struct sock *sk) > * after validation is complete. > */ > inet_sk_state_store(sk, TCP_LISTEN); > - if (!sk->sk_prot->get_port(sk, inet->inet_num)) { > + err = sk->sk_prot->get_port(sk, inet->inet_num); > + if (!err) { > inet->inet_sport = htons(inet->inet_num); IMHO in the "if (err)" block in all these places what is missing is: if (err > 0) err = -EADDRINUSE; so that all non-negative errors are properly mapped to -EADDRINUSE, like in the appended patch (if someone wants to give it a try, I've not even build-tested it). Note that I don't like it much and do not like the original patch either, I think a revert and a cleaner fix could be better :-/ Willy -- diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index cf11f10927e1..ce9960d9448d 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -526,6 +526,9 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len, err = sk->sk_prot->get_port(sk, snum); if (err) { inet->inet_saddr = inet->inet_rcv_saddr = 0; + /* some ->get_port() return 1 on failure */ + if (err > 0) + err = -EADDRINUSE; goto out_release_sock; } if (!(flags & BIND_FROM_BPF)) { diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index f2c43f67187d..7585c440fb8c 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -1241,6 +1241,9 @@ int inet_csk_listen_start(struct sock *sk) if (likely(!err)) return 0; } + /* some ->get_port() return 1 on failure */ + if (err > 0) + err = -EADDRINUSE; inet_sk_set_state(sk, TCP_CLOSE); return err; diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 847934763868..941c8ee4a144 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -415,6 +415,9 @@ static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len, if (err) { sk->sk_ipv6only = saved_ipv6only; inet_reset_saddr(sk); + /* some ->get_port() return 1 on failure */ + if (err > 0) + err = -EADDRINUSE; goto out; } if (!(flags & BIND_FROM_BPF)) {
