1. Patchew
  2. MPTCP
  3. View series

[PATCH v1 net] net: Return errno in sk->sk_prot->get_port().

Kuniyuki Iwashima posted 1 patch 1 year, 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
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(-)
[PATCH v1 net] net: Return errno in sk->sk_prot->get_port().
Posted by Kuniyuki Iwashima 1 year, 5 months ago 
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(-)

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)) {
-- 
2.30.2
Re: [PATCH v1 net] net: Return errno in sk->sk_prot->get_port().
Posted by Paolo Abeni 1 year, 5 months ago 
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
Re: [PATCH v1 net] net: Return errno in sk->sk_prot->get_port().
Posted by Kuniyuki Iwashima 1 year, 5 months ago 
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)) {
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.