Re: [PATCH net] xps: must clear sender_cpu before forwarding

[John <jw@nuclearfallout.net>]

Thanks, Eric. I will test again tomorrow.

-John

On 3/11/2015 6:42 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> John reported that my previous commit added a regression
> on his router.
>
> This is because sender_cpu & napi_id share a common location,
> so get_xps_queue() can see garbage and perform an out of bound access.
>
> We need to make sure sender_cpu is cleared before doing the transmit,
> otherwise any NIC busy poll enabled (skb_mark_napi_id()) can trigger
> this bug.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: John <jw@nuclearfallout.net>
> Bisected-by: John <jw@nuclearfallout.net>
> Fixes: 2bd82484bb4c ("xps: fix xps for stacked devices")
> ---
>   include/linux/skbuff.h |    7 +++++++
>   net/core/skbuff.c      |    2 +-
>   net/ipv4/ip_forward.c  |    1 +
>   net/ipv6/ip6_output.c  |    1 +
>   4 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 30007afe70b3..f54d6659713a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -948,6 +948,13 @@ static inline void skb_copy_hash(struct sk_buff *to, const struct sk_buff *from)
>   	to->l4_hash = from->l4_hash;
>   };
>   
> +static inline void skb_sender_cpu_clear(struct sk_buff *skb)
> +{
> +#ifdef CONFIG_XPS
> +	skb->sender_cpu = 0;
> +#endif
> +}
> +
>   #ifdef NET_SKBUFF_DATA_USES_OFFSET
>   static inline unsigned char *skb_end_pointer(const struct sk_buff *skb)
>   {
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f80507823531..434e78e5254d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4173,7 +4173,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
>   	skb->ignore_df = 0;
>   	skb_dst_drop(skb);
>   	skb->mark = 0;
> -	skb->sender_cpu = 0;
> +	skb_sender_cpu_clear(skb);
>   	skb_init_secmark(skb);
>   	secpath_reset(skb);
>   	nf_reset(skb);
> diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
> index 787b3c294ce6..d9bc28ac5d1b 100644
> --- a/net/ipv4/ip_forward.c
> +++ b/net/ipv4/ip_forward.c
> @@ -67,6 +67,7 @@ static int ip_forward_finish(struct sk_buff *skb)
>   	if (unlikely(opt->optlen))
>   		ip_forward_options(skb);
>   
> +	skb_sender_cpu_clear(skb);
>   	return dst_output(skb);
>   }
>   
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 0a04a37305d5..7e80b61b51ff 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -318,6 +318,7 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
>   
>   static inline int ip6_forward_finish(struct sk_buff *skb)
>   {
> +	skb_sender_cpu_clear(skb);
>   	return dst_output(skb);
>   }
>   
>
>