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=JiUFXUOmag..., 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;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)) {