[PATCH net-next 0/3] inet: ping: misc changes

[PATCH net-next 0/3] inet: ping: misc changes

[Eric Dumazet]

First and third patches improve security a bit.

Second patch (ping_hash removal) is a cleanup.

Eric Dumazet (3):
  inet: ping: check sock_net() in ping_get_port()
  inet: ping: remove ping_hash()
  inet: ping: make ping_port_rover per netns

 include/net/netns/ipv4.h |  1 +
 net/ipv4/ping.c          | 32 +++++++++++++-------------------
 net/ipv6/ping.c          |  1 -
 3 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 1/3] inet: ping: check sock_net() in ping_get_port()

[Eric Dumazet]

We need to check socket netns before considering them in ping_get_port().
Otherwise, one malicious netns could 'consume' all ports.

Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ping.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 031df4c19fcc5ca18137695c78358c3ad96a2c4a..3734e2d5c4814ea0a8d318c54e38b4dc978f6c77 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -77,6 +77,7 @@ static inline struct hlist_head *ping_hashslot(struct ping_table *table,
 
 int ping_get_port(struct sock *sk, unsigned short ident)
 {
+	struct net *net = sock_net(sk);
 	struct inet_sock *isk, *isk2;
 	struct hlist_head *hlist;
 	struct sock *sk2 = NULL;
@@ -90,9 +91,10 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 		for (i = 0; i < (1L << 16); i++, result++) {
 			if (!result)
 				result++; /* avoid zero */
-			hlist = ping_hashslot(&ping_table, sock_net(sk),
-					    result);
+			hlist = ping_hashslot(&ping_table, net, result);
 			sk_for_each(sk2, hlist) {
+				if (!net_eq(sock_net(sk2), net))
+					continue;
 				isk2 = inet_sk(sk2);
 
 				if (isk2->inet_num == result)
@@ -108,8 +110,10 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 		if (i >= (1L << 16))
 			goto fail;
 	} else {
-		hlist = ping_hashslot(&ping_table, sock_net(sk), ident);
+		hlist = ping_hashslot(&ping_table, net, ident);
 		sk_for_each(sk2, hlist) {
+			if (!net_eq(sock_net(sk2), net))
+				continue;
 			isk2 = inet_sk(sk2);
 
 			/* BUG? Why is this reuse and not reuseaddr? ping.c
@@ -129,7 +133,7 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 		pr_debug("was not hashed\n");
 		sk_add_node_rcu(sk, hlist);
 		sock_set_flag(sk, SOCK_RCU_FREE);
-		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
+		sock_prot_inuse_add(net, sk->sk_prot, 1);
 	}
 	spin_unlock(&ping_table.lock);
 	return 0;
-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 2/3] inet: ping: remove ping_hash()

[Eric Dumazet]

There is no point in keeping ping_hash().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/ping.c | 10 ----------
 net/ipv6/ping.c |  1 -
 2 files changed, 11 deletions(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 3734e2d5c4814ea0a8d318c54e38b4dc978f6c77..efceb2e17887f32d89c85161ccd818b12e38ff20 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -67,7 +67,6 @@ static inline u32 ping_hashfn(const struct net *net, u32 num, u32 mask)
 	pr_debug("hash(%u) = %u\n", num, res);
 	return res;
 }
-EXPORT_SYMBOL_GPL(ping_hash);
 
 static inline struct hlist_head *ping_hashslot(struct ping_table *table,
 					       struct net *net, unsigned int num)
@@ -144,14 +143,6 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 }
 EXPORT_SYMBOL_GPL(ping_get_port);
 
-int ping_hash(struct sock *sk)
-{
-	pr_debug("ping_hash(sk->port=%u)\n", inet_sk(sk)->inet_num);
-	BUG(); /* "Please do not press this button again." */
-
-	return 0;
-}
-
 void ping_unhash(struct sock *sk)
 {
 	struct inet_sock *isk = inet_sk(sk);
@@ -1006,7 +997,6 @@ struct proto ping_prot = {
 	.bind =		ping_bind,
 	.backlog_rcv =	ping_queue_rcv_skb,
 	.release_cb =	ip4_datagram_release_cb,
-	.hash =		ping_hash,
 	.unhash =	ping_unhash,
 	.get_port =	ping_get_port,
 	.put_port =	ping_unhash,
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index 82b0492923d458213ac7a6f9316158af2191e30f..d7a2cdaa26312b44f1fe502d3d40f3e27f961fa8 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -208,7 +208,6 @@ struct proto pingv6_prot = {
 	.recvmsg =	ping_recvmsg,
 	.bind =		ping_bind,
 	.backlog_rcv =	ping_queue_rcv_skb,
-	.hash =		ping_hash,
 	.unhash =	ping_unhash,
 	.get_port =	ping_get_port,
 	.put_port =	ping_unhash,
-- 
2.51.0.261.g7ce5a0a67e-goog

[PATCH net-next 3/3] inet: ping: make ping_port_rover per netns

[Eric Dumazet]

Provide isolation between netns for ping idents.

Randomize initial ping_port_rover value at netns creation.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/netns/ipv4.h |  1 +
 net/ipv4/ping.c          | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 6373e3f17da84ebc5c11058763932e595f0fd205..54a7d187f62a2e995076e85f1e6b2fd70f84b2c1 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -251,6 +251,7 @@ struct netns_ipv4 {
 	int sysctl_igmp_qrv;
 
 	struct ping_group_range ping_group_range;
+	u16			ping_port_rover;
 
 	atomic_t dev_addr_genid;
 
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index efceb2e17887f32d89c85161ccd818b12e38ff20..accfc249f6ceb29805e3bbec25d0721d2563cb4f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -58,8 +58,6 @@ static struct ping_table ping_table;
 struct pingv6_ops pingv6_ops;
 EXPORT_SYMBOL_GPL(pingv6_ops);
 
-static u16 ping_port_rover;
-
 static inline u32 ping_hashfn(const struct net *net, u32 num, u32 mask)
 {
 	u32 res = (num + net_hash_mix(net)) & mask;
@@ -84,12 +82,12 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 	isk = inet_sk(sk);
 	spin_lock(&ping_table.lock);
 	if (ident == 0) {
+		u16 result = net->ipv4.ping_port_rover + 1;
 		u32 i;
-		u16 result = ping_port_rover + 1;
 
 		for (i = 0; i < (1L << 16); i++, result++) {
 			if (!result)
-				result++; /* avoid zero */
+				continue; /* avoid zero */
 			hlist = ping_hashslot(&ping_table, net, result);
 			sk_for_each(sk2, hlist) {
 				if (!net_eq(sock_net(sk2), net))
@@ -101,7 +99,7 @@ int ping_get_port(struct sock *sk, unsigned short ident)
 			}
 
 			/* found */
-			ping_port_rover = ident = result;
+			net->ipv4.ping_port_rover = ident = result;
 			break;
 next_port:
 			;
@@ -1144,6 +1142,8 @@ static int __net_init ping_v4_proc_init_net(struct net *net)
 	if (!proc_create_net("icmp", 0444, net->proc_net, &ping_v4_seq_ops,
 			sizeof(struct ping_iter_state)))
 		return -ENOMEM;
+
+	net->ipv4.ping_port_rover = get_random_u16();
 	return 0;
 }
 
-- 
2.51.0.261.g7ce5a0a67e-goog

Re: [PATCH net-next 1/3] inet: ping: check sock_net() in ping_get_port()

[David Ahern]

On 8/27/25 6:05 AM, Eric Dumazet wrote:
> We need to check socket netns before considering them in ping_get_port().
> Otherwise, one malicious netns could 'consume' all ports.
> 
> Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/ping.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 2/3] inet: ping: remove ping_hash()

[David Ahern]

On 8/27/25 6:05 AM, Eric Dumazet wrote:
> There is no point in keeping ping_hash().
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/ping.c | 10 ----------
>  net/ipv6/ping.c |  1 -
>  2 files changed, 11 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 3/3] inet: ping: make ping_port_rover per netns

[David Ahern]

On 8/27/25 6:05 AM, Eric Dumazet wrote:
> @@ -84,12 +82,12 @@ int ping_get_port(struct sock *sk, unsigned short ident)
>  	isk = inet_sk(sk);
>  	spin_lock(&ping_table.lock);
>  	if (ident == 0) {
> +		u16 result = net->ipv4.ping_port_rover + 1;
>  		u32 i;
> -		u16 result = ping_port_rover + 1;
>  
>  		for (i = 0; i < (1L << 16); i++, result++) {
>  			if (!result)
> -				result++; /* avoid zero */
> +				continue; /* avoid zero */
>  			hlist = ping_hashslot(&ping_table, net, result);
>  			sk_for_each(sk2, hlist) {
>  				if (!net_eq(sock_net(sk2), net))
> @@ -101,7 +99,7 @@ int ping_get_port(struct sock *sk, unsigned short ident)
>  			}
>  
>  			/* found */
> -			ping_port_rover = ident = result;
> +			net->ipv4.ping_port_rover = ident = result;

READ_ONCE above and WRITE_ONCE here?

Re: [PATCH net-next 3/3] inet: ping: make ping_port_rover per netns

[Eric Dumazet]

On Wed, Aug 27, 2025 at 7:57 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 8/27/25 6:05 AM, Eric Dumazet wrote:
> > @@ -84,12 +82,12 @@ int ping_get_port(struct sock *sk, unsigned short ident)
> >       isk = inet_sk(sk);
> >       spin_lock(&ping_table.lock);
> >       if (ident == 0) {
> > +             u16 result = net->ipv4.ping_port_rover + 1;
> >               u32 i;
> > -             u16 result = ping_port_rover + 1;
> >
> >               for (i = 0; i < (1L << 16); i++, result++) {
> >                       if (!result)
> > -                             result++; /* avoid zero */
> > +                             continue; /* avoid zero */
> >                       hlist = ping_hashslot(&ping_table, net, result);
> >                       sk_for_each(sk2, hlist) {
> >                               if (!net_eq(sock_net(sk2), net))
> > @@ -101,7 +99,7 @@ int ping_get_port(struct sock *sk, unsigned short ident)
> >                       }
> >
> >                       /* found */
> > -                     ping_port_rover = ident = result;
> > +                     net->ipv4.ping_port_rover = ident = result;
>
> READ_ONCE above and WRITE_ONCE here?

Note we hold ping_table.lock for both the read and write,
so there is no need for READ_ONCE() or WRITE_ONCE() here.

Thank you !

Re: [PATCH net-next 3/3] inet: ping: make ping_port_rover per netns

[David Ahern]

On 8/27/25 9:00 AM, Eric Dumazet wrote:
> On Wed, Aug 27, 2025 at 7:57 AM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 8/27/25 6:05 AM, Eric Dumazet wrote:
>>> @@ -84,12 +82,12 @@ int ping_get_port(struct sock *sk, unsigned short ident)
>>>       isk = inet_sk(sk);
>>>       spin_lock(&ping_table.lock);
>>>       if (ident == 0) {
>>> +             u16 result = net->ipv4.ping_port_rover + 1;
>>>               u32 i;
>>> -             u16 result = ping_port_rover + 1;
>>>
>>>               for (i = 0; i < (1L << 16); i++, result++) {
>>>                       if (!result)
>>> -                             result++; /* avoid zero */
>>> +                             continue; /* avoid zero */
>>>                       hlist = ping_hashslot(&ping_table, net, result);
>>>                       sk_for_each(sk2, hlist) {
>>>                               if (!net_eq(sock_net(sk2), net))
>>> @@ -101,7 +99,7 @@ int ping_get_port(struct sock *sk, unsigned short ident)
>>>                       }
>>>
>>>                       /* found */
>>> -                     ping_port_rover = ident = result;
>>> +                     net->ipv4.ping_port_rover = ident = result;
>>
>> READ_ONCE above and WRITE_ONCE here?
> 
> Note we hold ping_table.lock for both the read and write,
> so there is no need for READ_ONCE() or WRITE_ONCE() here.
> 
> Thank you !

missed that.

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 1/3] inet: ping: check sock_net() in ping_get_port()

[Eric Dumazet]

On Wed, Aug 27, 2025 at 7:50 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 8/27/25 6:05 AM, Eric Dumazet wrote:
> > We need to check socket netns before considering them in ping_get_port().
> > Otherwise, one malicious netns could 'consume' all ports.
> >
> > Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/ipv4/ping.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
>
> Reviewed-by: David Ahern <dsahern@kernel.org>

I will add in V2 this part as well, and will retain your tag.

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index accfc249f6ceb29805e3bbec25d0721d2563cb4f..b6b336dac961bbabbc30f7f7fb5fe41d2ee54125
100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -181,6 +181,8 @@ static struct sock *ping_lookup(struct net *net,
struct sk_buff *skb, u16 ident)
        }

        sk_for_each_rcu(sk, hslot) {
+               if (!net_eq(sock_net(sk), net))
+                       continue;
                isk = inet_sk(sk);

                pr_debug("iterate\n");