| Message ID | 20221117184723.29318-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | [v1,net] net: Return errno in sk->sk_prot->get_port(). | expand |
On Thu, 2022-11-17 at 10:47 -0800, Kuniyuki Iwashima wrote: > 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 that such return value is on a WARN_ON_ONCE() check. I think such condition is actually an overzealot error checking and we could possibly/likelly remove it in the future. Still the change below IMHO makes the code cleaner, so I'm not opposing. Possibly could be targeting net-next instead. Thanks, Paolo > Note the only exception is inet_autobind(), all of whose callers return > -EAGAIN instead. > > Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections") > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/ipv4/af_inet.c | 4 ++-- > net/ipv4/inet_connection_sock.c | 7 ++++--- > net/ipv4/ping.c | 2 +- > net/ipv4/udp.c | 2 +- > net/ipv6/af_inet6.c | 4 ++-- > 5 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 4728087c42a5..4799eb55c830 100644 > --- 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 4e84ed21d16f..4a34bc7cb15e 100644 > --- a/net/ipv4/inet_connection_sock.c > +++ b/net/ipv4/inet_connection_sock.c > @@ -471,11 +471,11 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum) > bool reuse = sk->sk_reuse && sk->sk_state != TCP_LISTEN; > bool found_port = false, check_bind_conflict = true; > bool bhash_created = false, bhash2_created = false; > + int ret = -EADDRINUSE, port = snum, l3mdev; > struct inet_bind_hashbucket *head, *head2; > struct inet_bind2_bucket *tb2 = NULL; > struct inet_bind_bucket *tb = NULL; > bool head2_lock_acquired = false; > - int ret = 1, port = snum, l3mdev; > struct net *net = sock_net(sk); > > l3mdev = inet_sk_bound_l3mdev(sk); > @@ -1186,7 +1186,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); > > @@ -1202,7 +1202,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); > > sk_dst_reset(sk); > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > index bde333b24837..bb9854c2b7a1 100644 > --- a/net/ipv4/ping.c > +++ b/net/ipv4/ping.c > @@ -138,7 +138,7 @@ int ping_get_port(struct sock *sk, unsigned short ident) > > fail: > spin_unlock(&ping_table.lock); > - return 1; > + return -EADDRINUSE; > } > EXPORT_SYMBOL_GPL(ping_get_port); > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index 6a320a614e54..b30137f48fff 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -234,7 +234,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, > { > struct udp_hslot *hslot, *hslot2; > struct udp_table *udptable = sk->sk_prot->h.udp_table; > - int error = 1; > + int error = -EADDRINUSE; > struct net *net = sock_net(sk); > > if (!snum) { > diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c > index 024191004982..7b0cd54da452 100644 > --- a/net/ipv6/af_inet6.c > +++ b/net/ipv6/af_inet6.c > @@ -409,10 +409,10 @@ static int __inet6_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) { > sk->sk_ipv6only = saved_ipv6only; > inet_reset_saddr(sk); > - err = -EADDRINUSE; > goto out; > } > if (!(flags & BIND_FROM_BPF)) { Th
From: Paolo Abeni <pabeni@redhat.com> Date: Fri, 18 Nov 2022 10:35:07 +0100 > On Thu, 2022-11-17 at 10:47 -0800, Kuniyuki Iwashima wrote: > > 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 that such return value is on a WARN_ON_ONCE() check. I think such > condition is actually an overzealot error checking and we could > possibly/likelly remove it in the future. > > Still the change below IMHO makes the code cleaner, so I'm not > opposing. Possibly could be targeting net-next instead. Even without this patch we can catch the error as -EADDRINUSE, and I don't have strong preference, so I'll respin for net-next. Thank you. > > Thanks, > > Paolo > > > Note the only exception is inet_autobind(), all of whose callers return > > -EAGAIN instead. > > > > Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections") > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > net/ipv4/af_inet.c | 4 ++-- > > net/ipv4/inet_connection_sock.c | 7 ++++--- > > net/ipv4/ping.c | 2 +- > > net/ipv4/udp.c | 2 +- > > net/ipv6/af_inet6.c | 4 ++-- > > 5 files changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > > index 4728087c42a5..4799eb55c830 100644 > > --- 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 4e84ed21d16f..4a34bc7cb15e 100644 > > --- a/net/ipv4/inet_connection_sock.c > > +++ b/net/ipv4/inet_connection_sock.c > > @@ -471,11 +471,11 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum) > > bool reuse = sk->sk_reuse && sk->sk_state != TCP_LISTEN; > > bool found_port = false, check_bind_conflict = true; > > bool bhash_created = false, bhash2_created = false; > > + int ret = -EADDRINUSE, port = snum, l3mdev; > > struct inet_bind_hashbucket *head, *head2; > > struct inet_bind2_bucket *tb2 = NULL; > > struct inet_bind_bucket *tb = NULL; > > bool head2_lock_acquired = false; > > - int ret = 1, port = snum, l3mdev; > > struct net *net = sock_net(sk); > > > > l3mdev = inet_sk_bound_l3mdev(sk); > > @@ -1186,7 +1186,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); > > > > @@ -1202,7 +1202,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); > > > > sk_dst_reset(sk); > > diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c > > index bde333b24837..bb9854c2b7a1 100644 > > --- a/net/ipv4/ping.c > > +++ b/net/ipv4/ping.c > > @@ -138,7 +138,7 @@ int ping_get_port(struct sock *sk, unsigned short ident) > > > > fail: > > spin_unlock(&ping_table.lock); > > - return 1; > > + return -EADDRINUSE; > > } > > EXPORT_SYMBOL_GPL(ping_get_port); > > > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > > index 6a320a614e54..b30137f48fff 100644 > > --- a/net/ipv4/udp.c > > +++ b/net/ipv4/udp.c > > @@ -234,7 +234,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, > > { > > struct udp_hslot *hslot, *hslot2; > > struct udp_table *udptable = sk->sk_prot->h.udp_table; > > - int error = 1; > > + int error = -EADDRINUSE; > > struct net *net = sock_net(sk); > > > > if (!snum) { > > diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c > > index 024191004982..7b0cd54da452 100644 > > --- a/net/ipv6/af_inet6.c > > +++ b/net/ipv6/af_inet6.c > > @@ -409,10 +409,10 @@ static int __inet6_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) { > > sk->sk_ipv6only = saved_ipv6only; > > inet_reset_saddr(sk); > > - err = -EADDRINUSE; > > goto out; > > } > > if (!(flags & BIND_FROM_BPF)) { >
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 4728087c42a5..4799eb55c830 100644 --- 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 4e84ed21d16f..4a34bc7cb15e 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -471,11 +471,11 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum) bool reuse = sk->sk_reuse && sk->sk_state != TCP_LISTEN; bool found_port = false, check_bind_conflict = true; bool bhash_created = false, bhash2_created = false; + int ret = -EADDRINUSE, port = snum, l3mdev; struct inet_bind_hashbucket *head, *head2; struct inet_bind2_bucket *tb2 = NULL; struct inet_bind_bucket *tb = NULL; bool head2_lock_acquired = false; - int ret = 1, port = snum, l3mdev; struct net *net = sock_net(sk); l3mdev = inet_sk_bound_l3mdev(sk); @@ -1186,7 +1186,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); @@ -1202,7 +1202,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); sk_dst_reset(sk); diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index bde333b24837..bb9854c2b7a1 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -138,7 +138,7 @@ int ping_get_port(struct sock *sk, unsigned short ident) fail: spin_unlock(&ping_table.lock); - return 1; + return -EADDRINUSE; } EXPORT_SYMBOL_GPL(ping_get_port); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 6a320a614e54..b30137f48fff 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -234,7 +234,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum, { struct udp_hslot *hslot, *hslot2; struct udp_table *udptable = sk->sk_prot->h.udp_table; - int error = 1; + int error = -EADDRINUSE; struct net *net = sock_net(sk); if (!snum) { diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 024191004982..7b0cd54da452 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -409,10 +409,10 @@ static int __inet6_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) { sk->sk_ipv6only = saved_ipv6only; inet_reset_saddr(sk); - err = -EADDRINUSE; goto out; } if (!(flags & BIND_FROM_BPF)) {
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. Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections") Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- net/ipv4/af_inet.c | 4 ++-- net/ipv4/inet_connection_sock.c | 7 ++++--- net/ipv4/ping.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c | 4 ++-- 5 files changed, 10 insertions(+), 9 deletions(-)