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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.