[PATCH net-next 0/2] inet: ping: remove extra skb_clone()/consume_skb()

[PATCH net-next 0/2] inet: ping: remove extra skb_clone()/consume_skb()

[Eric Dumazet]

First patch in the series moves ICMP_EXT_ECHOREPLY handling in icmp_rcv()
to prepare the second patch.

The second patch removes one skb_clone()/consume_skb() pair
when processing ICMP_EXT_REPLY packets. Some people
use hundreds of "ping -fq ..." to stress hosts :)

Eric Dumazet (2):
  ipv4: icmp: do not process ICMP_EXT_ECHOREPLY for broadcast/multicast
    addresses
  inet: ping: avoid skb_clone() dance in ping_rcv()

 net/ipv4/icmp.c | 33 +++++++++++++++++----------------
 net/ipv4/ping.c | 20 +++++---------------
 net/ipv6/icmp.c |  7 ++-----
 3 files changed, 24 insertions(+), 36 deletions(-)

-- 
2.48.1.658.g4767266eb4-goog

[PATCH net-next 1/2] ipv4: icmp: do not process ICMP_EXT_ECHOREPLY for broadcast/multicast addresses

[Eric Dumazet]

There is no point processing ICMP_EXT_ECHOREPLY for routes
which would drop ICMP_ECHOREPLY (RFC 1122 3.2.2.6, 3.2.2.8)

This seems an oversight of the initial implementation.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/icmp.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 799775ba97d4f1bad1f070fb2a2596650df177af..058d4c1e300d0c0be7a04fd67e8e39924dfcd2cc 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1248,22 +1248,6 @@ int icmp_rcv(struct sk_buff *skb)
 		goto reason_check;
 	}
 
-	if (icmph->type == ICMP_EXT_ECHOREPLY) {
-		reason = ping_rcv(skb);
-		goto reason_check;
-	}
-
-	/*
-	 *	18 is the highest 'known' ICMP type. Anything else is a mystery
-	 *
-	 *	RFC 1122: 3.2.2  Unknown ICMP messages types MUST be silently
-	 *		  discarded.
-	 */
-	if (icmph->type > NR_ICMP_TYPES) {
-		reason = SKB_DROP_REASON_UNHANDLED_PROTO;
-		goto error;
-	}
-
 	/*
 	 *	Parse the ICMP message
 	 */
@@ -1290,6 +1274,22 @@ int icmp_rcv(struct sk_buff *skb)
 		}
 	}
 
+	if (icmph->type == ICMP_EXT_ECHOREPLY) {
+		reason = ping_rcv(skb);
+		goto reason_check;
+	}
+
+	/*
+	 *	18 is the highest 'known' ICMP type. Anything else is a mystery
+	 *
+	 *	RFC 1122: 3.2.2  Unknown ICMP messages types MUST be silently
+	 *		  discarded.
+	 */
+	if (icmph->type > NR_ICMP_TYPES) {
+		reason = SKB_DROP_REASON_UNHANDLED_PROTO;
+		goto error;
+	}
+
 	reason = icmp_pointers[icmph->type].handler(skb);
 reason_check:
 	if (!reason)  {
-- 
2.48.1.658.g4767266eb4-goog

[PATCH net-next 2/2] inet: ping: avoid skb_clone() dance in ping_rcv()

[Eric Dumazet]

ping_rcv() callers currently call skb_free() or consume_skb(),
forcing ping_rcv() to clone the skb.

After this patch ping_rcv() is now 'consuming' the original skb,
either moving to a socket receive queue, or dropping it.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/icmp.c |  5 +++--
 net/ipv4/ping.c | 20 +++++---------------
 net/ipv6/icmp.c |  7 ++-----
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 058d4c1e300d0c0be7a04fd67e8e39924dfcd2cc..717cb7d3607a1c77a3f54b56d2bb98b1064dd878 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -1274,9 +1274,10 @@ int icmp_rcv(struct sk_buff *skb)
 		}
 	}
 
-	if (icmph->type == ICMP_EXT_ECHOREPLY) {
+	if (icmph->type == ICMP_EXT_ECHOREPLY ||
+	    icmph->type == ICMP_ECHOREPLY) {
 		reason = ping_rcv(skb);
-		goto reason_check;
+		return reason ? NET_RX_DROP : NET_RX_SUCCESS;
 	}
 
 	/*
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 85d09f2ecadcb690f01985771afa37ce2cd0befc..c14baa6589c748026b49416688cbea399e6d461a 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -966,10 +966,9 @@ EXPORT_SYMBOL_GPL(ping_queue_rcv_skb);
 
 enum skb_drop_reason ping_rcv(struct sk_buff *skb)
 {
-	enum skb_drop_reason reason = SKB_DROP_REASON_NO_SOCKET;
-	struct sock *sk;
 	struct net *net = dev_net(skb->dev);
 	struct icmphdr *icmph = icmp_hdr(skb);
+	struct sock *sk;
 
 	/* We assume the packet has already been checked by icmp_rcv */
 
@@ -980,20 +979,11 @@ enum skb_drop_reason ping_rcv(struct sk_buff *skb)
 	skb_push(skb, skb->data - (u8 *)icmph);
 
 	sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
-	if (sk) {
-		struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
-
-		pr_debug("rcv on socket %p\n", sk);
-		if (skb2)
-			reason = __ping_queue_rcv_skb(sk, skb2);
-		else
-			reason = SKB_DROP_REASON_NOMEM;
-	}
-
-	if (reason)
-		pr_debug("no socket, dropping\n");
+	if (sk)
+		return __ping_queue_rcv_skb(sk, skb);
 
-	return reason;
+	kfree_skb_reason(skb, SKB_DROP_REASON_NO_SOCKET);
+	return SKB_DROP_REASON_NO_SOCKET;
 }
 EXPORT_SYMBOL_GPL(ping_rcv);
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 4d14ab7f7e99f152cd5f5adaa023f0280957f275..3fd19a84b358d169bbdc351c43ede830c60afcf3 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -957,12 +957,9 @@ static int icmpv6_rcv(struct sk_buff *skb)
 		break;
 
 	case ICMPV6_ECHO_REPLY:
-		reason = ping_rcv(skb);
-		break;
-
 	case ICMPV6_EXT_ECHO_REPLY:
-		reason = ping_rcv(skb);
-		break;
+		ping_rcv(skb);
+		return 0;
 
 	case ICMPV6_PKT_TOOBIG:
 		/* BUGGG_FUTURE: if packet contains rthdr, we cannot update
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH net-next 1/2] ipv4: icmp: do not process ICMP_EXT_ECHOREPLY for broadcast/multicast addresses

[David Ahern]

On 2/26/25 11:34 AM, Eric Dumazet wrote:
> There is no point processing ICMP_EXT_ECHOREPLY for routes
> which would drop ICMP_ECHOREPLY (RFC 1122 3.2.2.6, 3.2.2.8)
> 
> This seems an oversight of the initial implementation.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/icmp.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>


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

Re: [PATCH net-next 2/2] inet: ping: avoid skb_clone() dance in ping_rcv()

[David Ahern]

On 2/26/25 11:34 AM, Eric Dumazet wrote:
> ping_rcv() callers currently call skb_free() or consume_skb(),
> forcing ping_rcv() to clone the skb.
> 
> After this patch ping_rcv() is now 'consuming' the original skb,
> either moving to a socket receive queue, or dropping it.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/icmp.c |  5 +++--
>  net/ipv4/ping.c | 20 +++++---------------
>  net/ipv6/icmp.c |  7 ++-----
>  3 files changed, 10 insertions(+), 22 deletions(-)
> 

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

Re: [PATCH net-next 0/2] inet: ping: remove extra skb_clone()/consume_skb()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 26 Feb 2025 18:34:35 +0000 you wrote:
> First patch in the series moves ICMP_EXT_ECHOREPLY handling in icmp_rcv()
> to prepare the second patch.
> 
> The second patch removes one skb_clone()/consume_skb() pair
> when processing ICMP_EXT_REPLY packets. Some people
> use hundreds of "ping -fq ..." to stress hosts :)
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] ipv4: icmp: do not process ICMP_EXT_ECHOREPLY for broadcast/multicast addresses
    https://git.kernel.org/netdev/net-next/c/daeb6a8f3b00
  - [net-next,2/2] inet: ping: avoid skb_clone() dance in ping_rcv()
    https://git.kernel.org/netdev/net-next/c/a7e38208fe71

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html