| 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
| ||
|
Date: Fri, 1 Dec 2023 15:45:09 -0800 (PST)
From: Mat Martineau <martineau@...nel.org>
To: David Laight <David.Laight@...lab.com>
cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
Stephen Hemminger <stephen@...workplumber.org>,
Eric Dumazet <edumazet@...gle.com>, David Ahern <dsahern@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"jakub@...udflare.com" <jakub@...udflare.com>
Subject: Re: [PATCH net-next] ipv4: Use READ/WRITE_ONCE() for IP
local_port_range
On Wed, 29 Nov 2023, David Laight wrote:
> Commit 227b60f5102cd added a seqlock to ensure that the low and high
> port numbers were always updated together.
> This is overkill because the two 16bit port numbers can be held in
> a u32 and read/written in a single instruction.
>
> More recently 91d0b78c5177f added support for finer per-socket limits.
> The user-supplied value is 'high << 16 | low' but they are held
> separately and the socket options protected by the socket lock.
>
> Use a u32 containing 'high << 16 | low' for both the 'net' and 'sk'
> fields and use READ_ONCE()/WRITE_ONCE() to ensure both values are
> always updated together.
>
> Change (the now trival) inet_get_local_port_range() to a static inline
> to optimise the calling code.
> (In particular avoiding returning integers by reference.)
>
> Signed-off-by: David Laight <david.laight@...lab.com>
> ---
> include/net/inet_sock.h | 5 +----
> include/net/ip.h | 7 ++++++-
> include/net/netns/ipv4.h | 3 +--
> net/ipv4/af_inet.c | 4 +---
> net/ipv4/inet_connection_sock.c | 29 ++++++++++------------------
> net/ipv4/ip_sockglue.c | 34 ++++++++++++++++-----------------
> net/ipv4/sysctl_net_ipv4.c | 12 ++++--------
> 7 files changed, 40 insertions(+), 54 deletions(-)
>
> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> index 74db6d97cae1..ebf71410aa2b 100644
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@ -234,10 +234,7 @@ struct inet_sock {
> int uc_index;
> int mc_index;
> __be32 mc_addr;
> - struct {
> - __u16 lo;
> - __u16 hi;
> - } local_port_range;
> + u32 local_port_range;
>
> struct ip_mc_socklist __rcu *mc_list;
> struct inet_cork_full cork;
> diff --git a/include/net/ip.h b/include/net/ip.h
> index 1fc4c8d69e33..154f9dd75fe6 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -349,7 +349,12 @@ static inline u64 snmp_fold_field64(void __percpu *mib, int offt, size_t syncp_o
> } \
> }
>
> -void inet_get_local_port_range(const struct net *net, int *low, int *high);
> +static inline void inet_get_local_port_range(const struct net *net, int *low, int *high)
> +{
> + u32 range = READ_ONCE(net->ipv4.ip_local_ports.range);
> + *low = range & 0xffff;
> + *high = range >> 16;
> +}
> void inet_sk_get_local_port_range(const struct sock *sk, int *low, int *high);
>
> #ifdef CONFIG_SYSCTL
> diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
> index 73f43f699199..30ba5359da84 100644
> --- a/include/net/netns/ipv4.h
> +++ b/include/net/netns/ipv4.h
> @@ -19,8 +19,7 @@ struct hlist_head;
> struct fib_table;
> struct sock;
> struct local_ports {
> - seqlock_t lock;
> - int range[2];
> + u32 range; /* high << 16 | low */
> bool warned;
> };
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index fb81de10d332..b8964b40c3c0 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1847,9 +1847,7 @@ static __net_init int inet_init_net(struct net *net)
> /*
> * Set defaults for local port range
> */
> - seqlock_init(&net->ipv4.ip_local_ports.lock);
> - net->ipv4.ip_local_ports.range[0] = 32768;
> - net->ipv4.ip_local_ports.range[1] = 60999;
> + net->ipv4.ip_local_ports.range = 60999 << 16 | 32768;
Hi David -
Better to use unsigned integer constants here, since 60999 << 16 doesn't
fit in a signed int on 32-bit platforms.
>
> seqlock_init(&net->ipv4.ping_group_range.lock);
> /*
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 394a498c2823..1a45d41f8b39 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -117,34 +117,25 @@ bool inet_rcv_saddr_any(const struct sock *sk)
> return !sk->sk_rcv_saddr;
> }
>
> -void inet_get_local_port_range(const struct net *net, int *low, int *high)
> -{
> - unsigned int seq;
> -
> - do {
> - seq = read_seqbegin(&net->ipv4.ip_local_ports.lock);
> -
> - *low = net->ipv4.ip_local_ports.range[0];
> - *high = net->ipv4.ip_local_ports.range[1];
> - } while (read_seqretry(&net->ipv4.ip_local_ports.lock, seq));
> -}
> -EXPORT_SYMBOL(inet_get_local_port_range);
> -
> void inet_sk_get_local_port_range(const struct sock *sk, int *low, int *high)
> {
> const struct inet_sock *inet = inet_sk(sk);
> const struct net *net = sock_net(sk);
> int lo, hi, sk_lo, sk_hi;
> + u32 sk_range;
>
> inet_get_local_port_range(net, &lo, &hi);
>
> - sk_lo = inet->local_port_range.lo;
> - sk_hi = inet->local_port_range.hi;
> + sk_range = READ_ONCE(inet->local_port_range);
> + if (unlikely(sk_range)) {
> + sk_lo = sk_range & 0xffff;
> + sk_hi = sk_range >> 16;
>
> - if (unlikely(lo <= sk_lo && sk_lo <= hi))
> - lo = sk_lo;
> - if (unlikely(lo <= sk_hi && sk_hi <= hi))
> - hi = sk_hi;
> + if (unlikely(lo <= sk_lo && sk_lo <= hi))
> + lo = sk_lo;
> + if (unlikely(lo <= sk_hi && sk_hi <= hi))
> + hi = sk_hi;
> + }
>
> *low = lo;
> *high = hi;
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 2efc53526a38..bf940fe249a5 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1055,6 +1055,20 @@ int do_ip_setsockopt(struct sock *sk, int level, int optname,
> case IP_TOS: /* This sets both TOS and Precedence */
> ip_sock_set_tos(sk, val);
> return 0;
> +
> + case IP_LOCAL_PORT_RANGE:
> + {
> + const __u16 lo = val;
> + const __u16 hi = val >> 16;
Suggest casting 'val' to an unsigned int before shifting right, even
though assigning to a __u16 will mask off any surprising bits introduced
by an arithmetic right shift of a 32-bit signed int.
> +
> + if (optlen != sizeof(__u32))
> + return -EINVAL;
> + if (lo != 0 && hi != 0 && lo > hi)
> + return -EINVAL;
> +
> + WRITE_ONCE(inet->local_port_range, val);
> + return 0;
> + }
> }
>
> err = 0;
> @@ -1332,20 +1346,6 @@ int do_ip_setsockopt(struct sock *sk, int level, int optname,
> err = xfrm_user_policy(sk, optname, optval, optlen);
> break;
>
> - case IP_LOCAL_PORT_RANGE:
> - {
> - const __u16 lo = val;
> - const __u16 hi = val >> 16;
> -
> - if (optlen != sizeof(__u32))
> - goto e_inval;
> - if (lo != 0 && hi != 0 && lo > hi)
> - goto e_inval;
> -
> - inet->local_port_range.lo = lo;
> - inet->local_port_range.hi = hi;
> - break;
> - }
> default:
> err = -ENOPROTOOPT;
> break;
> @@ -1692,6 +1692,9 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
> return -EFAULT;
> return 0;
> }
> + case IP_LOCAL_PORT_RANGE:
> + val = READ_ONCE(inet->local_port_range);
> + goto copyval;
> }
>
> if (needs_rtnl)
> @@ -1721,9 +1724,6 @@ int do_ip_getsockopt(struct sock *sk, int level, int optname,
> else
> err = ip_get_mcast_msfilter(sk, optval, optlen, len);
> goto out;
> - case IP_LOCAL_PORT_RANGE:
> - val = inet->local_port_range.hi << 16 | inet->local_port_range.lo;
> - break;
> case IP_PROTOCOL:
> val = inet_sk(sk)->inet_num;
> break;
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index f63a545a7374..b008b6b5e85d 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -54,22 +54,18 @@ static void set_local_port_range(struct net *net, int range[2])
> {
> bool same_parity = !((range[0] ^ range[1]) & 1);
>
> - write_seqlock_bh(&net->ipv4.ip_local_ports.lock);
> if (same_parity && !net->ipv4.ip_local_ports.warned) {
> net->ipv4.ip_local_ports.warned = true;
> pr_err_ratelimited("ip_local_port_range: prefer different parity for start/end values.\n");
> }
> - net->ipv4.ip_local_ports.range[0] = range[0];
> - net->ipv4.ip_local_ports.range[1] = range[1];
> - write_sequnlock_bh(&net->ipv4.ip_local_ports.lock);
> + WRITE_ONCE(net->ipv4.ip_local_ports.range, range[1] << 16 | range[0]);
Similar, make sure the value is cast to unsigned before shifting here.
- Mat
Powered by blists - more mailing lists