[net-next] net: disable preempt for processed counter

When a kernel is built with CONFIG_PREEMPT and CONFIG_DEBUG_PREEMPT,
and a network driver is processing inbound data, we see

 BUG: using __this_cpu_add() in preemptible [00000000] code: nmd/4170
 caller is __this_cpu_preempt_check+0x18/0x20
 CPU: 1 PID: 4170 Comm: nmd Tainted: G           O    4.14.18 #1
 Hardware name: (xxxxx)
 Call trace:
 [<ffff0000080895d0>] dump_backtrace+0x0/0x2a0
 [<ffff000008089894>] show_stack+0x24/0x30
 [<ffff0000085ed64c>] dump_stack+0xac/0xf0
 [<ffff0000083bd538>] check_preemption_disabled+0xf8/0x100
 [<ffff0000083bd588>] __this_cpu_preempt_check+0x18/0x20
 [<ffff000008510f64>] __netif_receive_skb_core+0xa4/0xa10
 [<ffff000008514258>] __netif_receive_skb+0x28/0x80
 [<ffff0000085183b0>] netif_receive_skb_internal+0x30/0x120

We found that this gets hit inside of check_preemption_disabled(),
which is called by __netif_receive_skb_core() wanting to do a safe
per-cpu statistic increment with __this_cpu_inc(softnet_data.processed).
In order to be a safe increment, preemption needs to be disabled, but
in this case there are no calls to preempt_disable()/preempt_enable().
Note that a few lines later preempt_disable/preempt_enable() are used
around the call into do_xdp_generic().

This patch adds the preempt_disable()/preempt_enable() around
the call to __this_cpu_inc() as has been done in a few other
cases in the kernel.

Signed-off-by: Shannon Nelson <snelson@pensando.io>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

On 11/6/19 1:41 PM, Shannon Nelson wrote:
> When a kernel is built with CONFIG_PREEMPT and CONFIG_DEBUG_PREEMPT,
> and a network driver is processing inbound data, we see
> 
>  BUG: using __this_cpu_add() in preemptible [00000000] code: nmd/4170
>  caller is __this_cpu_preempt_check+0x18/0x20
>  CPU: 1 PID: 4170 Comm: nmd Tainted: G           O    4.14.18 #1
>  Hardware name: (xxxxx)
>  Call trace:
>  [<ffff0000080895d0>] dump_backtrace+0x0/0x2a0
>  [<ffff000008089894>] show_stack+0x24/0x30
>  [<ffff0000085ed64c>] dump_stack+0xac/0xf0
>  [<ffff0000083bd538>] check_preemption_disabled+0xf8/0x100
>  [<ffff0000083bd588>] __this_cpu_preempt_check+0x18/0x20
>  [<ffff000008510f64>] __netif_receive_skb_core+0xa4/0xa10
>  [<ffff000008514258>] __netif_receive_skb+0x28/0x80
>  [<ffff0000085183b0>] netif_receive_skb_internal+0x30/0x120
> 
> We found that this gets hit inside of check_preemption_disabled(),
> which is called by __netif_receive_skb_core() wanting to do a safe
> per-cpu statistic increment with __this_cpu_inc(softnet_data.processed).
> In order to be a safe increment, preemption needs to be disabled, but
> in this case there are no calls to preempt_disable()/preempt_enable().
> Note that a few lines later preempt_disable/preempt_enable() are used
> around the call into do_xdp_generic().

Interesting note, but this is not a good reason.

> 
> This patch adds the preempt_disable()/preempt_enable() around
> the call to __this_cpu_inc() as has been done in a few other
> cases in the kernel.
> 
> Signed-off-by: Shannon Nelson <snelson@pensando.io>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bb15800c8cb5..ffda48def391 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4976,7 +4976,9 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>  another_round:
>  	skb->skb_iif = skb->dev->ifindex;
>  
> +	preempt_disable();
>  	__this_cpu_inc(softnet_data.processed);
> +	preempt_enable();
>  
>  	if (static_branch_unlikely(&generic_xdp_needed_key)) {
>  		int ret2;
> 

The stack trace does not show which driver fed this packet.

Please tell us more about the driver, or give us the complete stack trace.