netdev - Re: [net-next PATCH] net: devmap fix mutex in rcu critical section

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5984EDF7.3060808@gmail.com>
Date:   Fri, 04 Aug 2017 14:58:15 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     davem@...emloft.net, daniel@...earbox.net,
        alexander.levin@...izon.com
CC:     netdev@...r.kernel.org
Subject: Re: [net-next PATCH] net: devmap fix mutex in rcu critical section

On 08/04/2017 02:21 PM, John Fastabend wrote:
> Originally we used a mutex to protect concurrent devmap update
> and delete operations from racing with netdev unregister notifier
> callbacks.
> 

[...]

>  }
> @@ -396,22 +385,20 @@ static int dev_map_notification(struct notifier_block *notifier,
>  

Daniel reminds me this is not in a rcu_read_lock/unlock() section as
needed, so v2 on its way. Thanks!

>  	switch (event) {
>  	case NETDEV_UNREGISTER:
> -		mutex_lock(&dev_map_list_mutex);

		rcu_read_lock();

>  		list_for_each_entry(dtab, &dev_map_list, list) {
>  			for (i = 0; i < dtab->map.max_entries; i++) {
> -				struct bpf_dtab_netdev *dev;
> +				struct bpf_dtab_netdev *dev, *odev;
>  
> -				dev = dtab->netdev_map[i];
> +				dev = READ_ONCE(dtab->netdev_map[i]);
>  				if (!dev ||
>  				    dev->dev->ifindex != netdev->ifindex)
>  					continue;
> -				dev = xchg(&dtab->netdev_map[i], NULL);
> -				if (dev)
> +				odev = cmpxchg(&dtab->netdev_map[i], dev, NULL);
> +				if (dev == odev)
>  					call_rcu(&dev->rcu,
>  						 __dev_map_entry_free);
>  			}
>  		}

		rcu_read_unlock();

> -		mutex_unlock(&dev_map_list_mutex);
>  		break;
>  	default:
>  		break;
>