| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221118181041.5393-1-kuniyu@amazon.com>
Date: Fri, 18 Nov 2022 10:10:40 -0800
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <cpaasch@...le.com>, <davem@...emloft.net>, <dsahern@...nel.org>,
<edumazet@...gle.com>, <fw@...len.de>, <kuba@...nel.org>,
<kuni1840@...il.com>, <kuniyu@...zon.com>, <mptcp@...ts.linux.dev>,
<netdev@...r.kernel.org>, <peter.krystad@...ux.intel.com>,
<yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH v1 net] net: Return errno in sk->sk_prot->get_port().
From: Paolo Abeni <pabeni@...hat.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@...zon.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)) {
>
Powered by blists - more mailing lists