[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y+m8F7Q95al39ctV@1wt.eu>
Date: Mon, 13 Feb 2023 05:27:03 +0100
From: Willy Tarreau <w@....eu>
To: Winter <winter@...ter.cafe>
Cc: stable@...r.kernel.org, regressions@...ts.linux.dev,
netdev@...r.kernel.org
Subject: Re: [REGRESSION] 5.15.88 and onwards no longer return EADDRINUSE
from bind
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@mail.gmail.com/,
> 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@linuxfoundation.org/
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)) {
Powered by blists - more mailing lists