netdev - Re: INFO: task hung in ovs_exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <35ADBFE5-87FF-4E9E-A8FD-BB586E9F663F@redhat.com>
Date:   Thu, 23 Jul 2020 13:48:59 +0200
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Hillf Danton" <hdanton@...a.com>
Cc:     syzbot <syzbot+2c4ff3614695f75ce26c@...kaller.appspotmail.com>,
        davem@...emloft.net, dev@...nvswitch.org, kuba@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        "Paolo Abeni" <pabeni@...hat.com>, pshelar@....org,
        syzkaller-bugs@...glegroups.com,
        "Markus Elfring" <Markus.Elfring@....de>
Subject: Re: INFO: task hung in ovs_exit_net



On 23 Jul 2020, at 13:06, Hillf Danton wrote:

> Wed, 22 Jul 2020 23:27:19 -0700
>> syzbot found the following issue on:
  <SNIP>
>
> Fixes: eac87c413bf9 ("net: openvswitch: reorder masks array based on 
> usage")
> by moving cancel_delayed_work_sync() in to the rcu cb, therefore out 
> of ovs
> lock. To facilitate that, add a flag in datapath to inform the kworker 
> that
> there is no more work needed.

I was thinking of re-working the patch and move the handling to the 
“struct ovs_net” instead of the datapath. This way the rebalance 
worker can rebalance all datapaths in the netns. Than I can move 
cancel_delayed_work_sync() from __dp_destroy()
to ovs_exit_net(), i.e. outside the ovs lock scope.

However, your fix would be way less intrusive. Are you planning on 
sending it as a patch? If so, maybe add a comment around the called_rcu 
variable to be more clear where it’s used for, or maybe rename it to 
something like called_destory_rcu?

If you think my approach would be better let me know, and I work on a 
patch.

Feedback anyone?

> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -82,6 +82,7 @@ struct datapath {
>
>  	u32 max_headroom;
>
> +	int called_rcu;
>  	/* Switch meters. */
>  	struct dp_meter_table meter_tbl;
>
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -161,6 +161,7 @@ static void destroy_dp_rcu(struct rcu_he
>  {
>  	struct datapath *dp = container_of(rcu, struct datapath, rcu);
>
> +	cancel_delayed_work_sync(&dp->masks_rebalance);
>  	ovs_flow_tbl_destroy(&dp->table);
>  	free_percpu(dp->stats_percpu);
>  	kfree(dp->ports);
> @@ -1760,11 +1761,9 @@ static void __dp_destroy(struct datapath
>  	 */
>  	ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
>
> +	dp->called_rcu = true;
>  	/* RCU destroy the flow table */
>  	call_rcu(&dp->rcu, destroy_dp_rcu);
> -
> -	/* Cancel remaining work. */
> -	cancel_delayed_work_sync(&dp->masks_rebalance);
>  }
>
>  static int ovs_dp_cmd_del(struct sk_buff *skb, struct genl_info 
> *info)
> @@ -2356,6 +2355,8 @@ static void ovs_dp_masks_rebalance(struc
>  	ovs_flow_masks_rebalance(&dp->table);
>  	ovs_unlock();
>
> +	if (dp->called_rcu)
> +		return;
>  	schedule_delayed_work(&dp->masks_rebalance,
>  			      msecs_to_jiffies(DP_MASKS_REBALANCE_INTERVAL));
>  }