[PATCH net-next] net: optimize eth_type_trans() vs CONFIG_STACKPROTECTOR_STRONG=y

[PATCH net-next] net: optimize eth_type_trans() vs CONFIG_STACKPROTECTOR_STRONG=y

[Eric Dumazet]

Some platforms exhibit very high costs with CONFIG_STACKPROTECTOR_STRONG=y
when a function needs to pass the address of a local variable to external
functions.

eth_type_trans() (and its callers) is showing this anomaly on AMD EPYC 7B12
platforms (and maybe others).

We could :

1) inline eth_type_trans()

   This would help if its callers also has the same issue, and the canary cost
   would be paid by the callers already.

   This is a bit cumbersome because netdev_uses_dsa() is pulling
   whole <net/dsa.h> definitions.

2) Compile net/ethernet/eth.c with -fno-stack-protector

   This would weaken security.

3) Hack eth_type_trans() to temporarily use skb->dev as a place holder
   if skb_header_pointer() needs to pull 2 bytes not present in skb->head.

This patch implements 3), and brings a 5% improvement on TX/RX intensive
workload (tcp_rr 10,000 flows) on AMD EPYC 7B12.

Removing CONFIG_STACKPROTECTOR_STRONG on this platform can improve
performance by 25 %.
This means eth_type_trans() issue is not an isolated artifact.

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

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 43e211e611b1698cbec5f6256ffd59975584bf04..13a63b48b7eeb896dfe98eb0070a261eed2c384b 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -154,9 +154,9 @@ EXPORT_SYMBOL(eth_get_headlen);
  */
 __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 {
-	unsigned short _service_access_point;
 	const unsigned short *sap;
 	const struct ethhdr *eth;
+	__be16 res;
 
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
@@ -181,15 +181,15 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
 	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
 	 *      won't work for fault tolerant netware but does for the rest.
+	 *	We use skb->dev as temporary storage to not hit
+	 *	CONFIG_STACKPROTECTOR_STRONG=y costs on some platforms.
 	 */
-	sap = skb_header_pointer(skb, 0, sizeof(*sap), &_service_access_point);
-	if (sap && *sap == 0xFFFF)
-		return htons(ETH_P_802_3);
+	sap = skb_header_pointer(skb, 0, sizeof(*sap), &skb->dev);
+	res = (sap && *sap == 0xFFFF) ? htons(ETH_P_802_3) : htons(ETH_P_802_2);
 
-	/*
-	 *      Real 802.2 LLC
-	 */
-	return htons(ETH_P_802_2);
+	/* restore skb->dev in case it was mangled by skb_header_pointer(). */
+	skb->dev = dev;
+	return res;
 }
 EXPORT_SYMBOL(eth_type_trans);
 
-- 
2.52.0.460.gd25c4c69ec-goog

Re: [PATCH net-next] net: optimize eth_type_trans() vs CONFIG_STACKPROTECTOR_STRONG=y

[Willem de Bruijn]

Eric Dumazet wrote:
> Some platforms exhibit very high costs with CONFIG_STACKPROTECTOR_STRONG=y
> when a function needs to pass the address of a local variable to external
> functions.
> 
> eth_type_trans() (and its callers) is showing this anomaly on AMD EPYC 7B12
> platforms (and maybe others).
> 
> We could :
> 
> 1) inline eth_type_trans()
> 
>    This would help if its callers also has the same issue, and the canary cost
>    would be paid by the callers already.
> 
>    This is a bit cumbersome because netdev_uses_dsa() is pulling
>    whole <net/dsa.h> definitions.
> 
> 2) Compile net/ethernet/eth.c with -fno-stack-protector
> 
>    This would weaken security.
> 
> 3) Hack eth_type_trans() to temporarily use skb->dev as a place holder
>    if skb_header_pointer() needs to pull 2 bytes not present in skb->head.
> 
> This patch implements 3), and brings a 5% improvement on TX/RX intensive
> workload (tcp_rr 10,000 flows) on AMD EPYC 7B12.
> 
> Removing CONFIG_STACKPROTECTOR_STRONG on this platform can improve
> performance by 25 %.
> This means eth_type_trans() issue is not an isolated artifact.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Good catch.

I guess this applies to many callers of skb_header_pointer.

The protected against risk is that the caller passes a len smaller
than sizeof(buffer), or that __skb_header_pointer/skb_copy_bits cannot
be trusted. The second we could analyze and allow-list.

I wonder if there is a (known?) mitigation. Using sizeof for stack
alloc'd structs in a special (macro) __skb_header_pointer rather than
having the caller pass a separate length arg, __attribute__
((no_stack_protector)), percpu storage, others.

Re: [PATCH net-next] net: optimize eth_type_trans() vs CONFIG_STACKPROTECTOR_STRONG=y

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 21 Nov 2025 06:17:25 +0000 you wrote:
> Some platforms exhibit very high costs with CONFIG_STACKPROTECTOR_STRONG=y
> when a function needs to pass the address of a local variable to external
> functions.
> 
> eth_type_trans() (and its callers) is showing this anomaly on AMD EPYC 7B12
> platforms (and maybe others).
> 
> [...]

Here is the summary with links:
  - [net-next] net: optimize eth_type_trans() vs CONFIG_STACKPROTECTOR_STRONG=y
    https://git.kernel.org/netdev/net-next/c/ec1e48e97feb

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