Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 9 Oct 2020 17:01:24 +0200
From:   Juri Lelli <juri.lelli@redhat.com>
To:     tglx@linutronix.de, bigeasy@linutronix.de
Cc:     linux-rt-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        bristot@redhat.com, williams@redhat.com, echaudro@redhat.com,
        atheurer@redhat.com
Subject: Re: [PATCH 5.9 RT] net: openvswitch: Fix using smp_processor_id() in
 preemptible code
Message-ID: <20201009150124.GV4352@localhost.localdomain>
References: <20201009124759.592550-1-juri.lelli@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201009124759.592550-1-juri.lelli@redhat.com>
Precedence: bulk

On 09/10/20 14:47, Juri Lelli wrote:
> The following BUG has been reported (slightly edited):
> 
>  BUG: using smp_processor_id() in preemptible [00000000] code: handler106/3082
>  caller is flow_lookup.isra.15+0x2c/0xf0 [openvswitch]
>  CPU: 46 PID: 3082 Comm: handler106 Not tainted ... #1
>  Hardware name: Dell Inc. PowerEdge R640/06DKY5, BIOS 2.5.4 01/13/2020
>  Call Trace:
>   dump_stack+0x5c/0x80
>   check_preemption_disabled+0xc4/0xd0
>   flow_lookup.isra.15+0x2c/0xf0 [openvswitch]
>   ovs_flow_tbl_lookup+0x3b/0x60 [openvswitch]
>   ovs_flow_cmd_new+0x2d8/0x430 [openvswitch]
>   ? __switch_to_asm+0x35/0x70
>   ? __switch_to_asm+0x41/0x70
>   ? __switch_to_asm+0x35/0x70
>   genl_family_rcv_msg+0x1d7/0x410
>   ? migrate_enable+0x123/0x3a0
>   genl_rcv_msg+0x47/0x8c
>   ? __kmalloc_node_track_caller+0xff/0x2e0
>   ? genl_family_rcv_msg+0x410/0x410
>   netlink_rcv_skb+0x4c/0x120
>   genl_rcv+0x24/0x40
>   netlink_unicast+0x197/0x230
>   netlink_sendmsg+0x204/0x3d0
>   sock_sendmsg+0x4c/0x50
>   ___sys_sendmsg+0x29f/0x300
>   ? migrate_enable+0x123/0x3a0
>   ? ep_send_events_proc+0x8a/0x1f0
>   ? ep_scan_ready_list.constprop.23+0x237/0x260
>   ? rt_spin_unlock+0x23/0x40
>   ? ep_poll+0x1b3/0x390
>   ? __fget+0x72/0xa0
>   __sys_sendmsg+0x57/0xa0
>   do_syscall_64+0x87/0x1a0
>   entry_SYSCALL_64_after_hwframe+0x65/0xca
>  RIP: 0033:0x7f1ed72ccb07
>  Code: ...
>  RSP: 002b:00007f1ecbd9ba80 EFLAGS: 00003293 ORIG_RAX: 000000000000002e
>  RAX: ffffffffffffffda RBX: 000000000000007b RCX: 00007f1ed72ccb07
>  RDX: 0000000000000000 RSI: 00007f1ecbd9bb10 RDI: 000000000000007b
>  RBP: 00007f1ecbd9bb10 R08: 0000000000000000 R09: 00007f1ecbd9d390
>  R10: 0000000019616156 R11: 0000000000003293 R12: 0000000000000000
>  R13: 00007f1ecbd9d338 R14: 00007f1ecbd9bfb0 R15: 00007f1ecbd9bb10
> 
> This happens because openvswitch/flow_table::flow_lookup() accesses
> per-cpu data while being preemptible (and migratable).
> 
> Fix it by adding get/put_cpu_light(), so that, even if preempted, the
> task executing this code is not migrated (operation is also guarded by
> ovs_mutex mutex).
> 

This actually wants also a (sorry for missing it in the first place)

Suggested-by: Daniel Bristot de Oliveira <bristot@redhat.com>

> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>  net/openvswitch/flow_table.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index e2235849a57e..7df27ef7da09 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -732,11 +732,14 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
>  				   u32 *n_cache_hit,
>  				   u32 *index)
>  {
> -	u64 *usage_counters = this_cpu_ptr(ma->masks_usage_cntr);
> +	u64 *usage_counters;
>  	struct sw_flow *flow;
>  	struct sw_flow_mask *mask;
>  	int i;
>  
> +	get_cpu_light();
> +	usage_counters = this_cpu_ptr(ma->masks_usage_cntr);
> +
>  	if (likely(*index < ma->max)) {
>  		mask = rcu_dereference_ovsl(ma->masks[*index]);
>  		if (mask) {
> @@ -746,6 +749,7 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
>  				usage_counters[*index]++;
>  				u64_stats_update_end(&ma->syncp);
>  				(*n_cache_hit)++;
> +				put_cpu_light();
>  				return flow;
>  			}
>  		}
> @@ -766,10 +770,12 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
>  			u64_stats_update_begin(&ma->syncp);
>  			usage_counters[*index]++;
>  			u64_stats_update_end(&ma->syncp);
> +			put_cpu_light();
>  			return flow;
>  		}
>  	}
>  
> +	put_cpu_light();
>  	return NULL;
>  }
>  
> -- 
> 2.26.2
>