All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] xps: must clear sender_cpu before forwarding
       [not found] ` <CANn89iLHkYc2Kay7z+v=tHMW5aYSqUf8V4f70Z-d5fUuuUXnpw@mail.gmail.com>
@ 2015-03-12  1:42   ` Eric Dumazet
  2015-03-12  3:51     ` David Miller
  2015-03-12  5:21     ` John
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2015-03-12  1:42 UTC (permalink / raw)
  To: John, David Miller; +Cc: netdev

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);
 }
 

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net] xps: must clear sender_cpu before forwarding
  2015-03-12  1:42   ` [PATCH net] xps: must clear sender_cpu before forwarding Eric Dumazet
@ 2015-03-12  3:51     ` David Miller
  2015-03-13  2:36       ` John
  2015-03-12  5:21     ` John
  1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2015-03-12  3:51 UTC (permalink / raw)
  To: eric.dumazet; +Cc: jw, netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 11 Mar 2015 18:42:02 -0700

> 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")

Applied, thanks Eric.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] xps: must clear sender_cpu before forwarding
  2015-03-12  1:42   ` [PATCH net] xps: must clear sender_cpu before forwarding Eric Dumazet
  2015-03-12  3:51     ` David Miller
@ 2015-03-12  5:21     ` John
  1 sibling, 0 replies; 6+ messages in thread
From: John @ 2015-03-12  5:21 UTC (permalink / raw)
  To: Eric Dumazet, David Miller; +Cc: netdev

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);
>   }
>   
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] xps: must clear sender_cpu before forwarding
  2015-03-12  3:51     ` David Miller
@ 2015-03-13  2:36       ` John
  2015-03-13  2:53         ` David Miller
  2015-03-13  2:57         ` Eric Dumazet
  0 siblings, 2 replies; 6+ messages in thread
From: John @ 2015-03-13  2:36 UTC (permalink / raw)
  To: David Miller, eric.dumazet; +Cc: netdev

On 3/11/2015 8:51 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 11 Mar 2015 18:42:02 -0700
>
>> 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")
> Applied, thanks Eric.

Running this patch, I have confirmed that I no longer see panics under 
the conditions that I saw them previously.

-John

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] xps: must clear sender_cpu before forwarding
  2015-03-13  2:36       ` John
@ 2015-03-13  2:53         ` David Miller
  2015-03-13  2:57         ` Eric Dumazet
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2015-03-13  2:53 UTC (permalink / raw)
  To: jw; +Cc: eric.dumazet, netdev

From: John <jw@nuclearfallout.net>
Date: Thu, 12 Mar 2015 19:36:38 -0700

> Running this patch, I have confirmed that I no longer see panics under
> the conditions that I saw them previously.

Thanks for testing.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net] xps: must clear sender_cpu before forwarding
  2015-03-13  2:36       ` John
  2015-03-13  2:53         ` David Miller
@ 2015-03-13  2:57         ` Eric Dumazet
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2015-03-13  2:57 UTC (permalink / raw)
  To: John; +Cc: David Miller, netdev

On Thu, 2015-03-12 at 19:36 -0700, John wrote:

> Running this patch, I have confirmed that I no longer see panics under 
> the conditions that I saw them previously.

Thanks a lot John !

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-13  2:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <5500E52C.7080603@nuclearfallout.net>
     [not found] ` <CANn89iLHkYc2Kay7z+v=tHMW5aYSqUf8V4f70Z-d5fUuuUXnpw@mail.gmail.com>
2015-03-12  1:42   ` [PATCH net] xps: must clear sender_cpu before forwarding Eric Dumazet
2015-03-12  3:51     ` David Miller
2015-03-13  2:36       ` John
2015-03-13  2:53         ` David Miller
2015-03-13  2:57         ` Eric Dumazet
2015-03-12  5:21     ` John

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.