Re: Understanding mutual exclusion between rtnl_lock and rcu_read_lock

On Feb 2, 2017, at 11:21 PM, Eric Dumazet [off-list ref] wrote:

On Thu, 2017-02-02 at 15:52 -0800, Alexander Duyck wrote:

quoted

On Thu, Feb 2, 2017 at 3:47 PM, Joel Cunningham [off-list ref] wrote:

quoted

Hi,

I’m studying the synchronization used on different parts of struct net_device and I’m struggling to understand how structure member modifications in dev_ioctl are synchronized.  Getters in dev_ifsioc_locked() are only holding rcu_read_lock() while setters in dev_ifsioc() are holding rtnl_lock, but not using RCU APIs.  I was specifically looking at SIOCGIFHWADDR/SIOCSIFHWADDR.  What’s to prevent one CPU from executing a getter and another CPU from executing a setter resulting in possibly a torn read/write?  I didn’t see anything in rtnl_lock() that would wait for any rcu_reader_lock() critical sections (on other CPUs) to finish before acquiring the mutex.

Is there something about dev_ioctl that prevents parallel execution? or maybe something I still don’t understand about the RCU implementation?

Thanks,

Joel

My advice would be to spend more time familiarizing yourself with RCU.
The advantage of RCU is that it allows for updates while other threads
are accessing the data.  The rtnl_lock is just meant to prevent
multiple writers from updating the data simultaneously.  So between
writers the rtnl_lock is used to keep things synchronized, but between
writers and readers the mechanism that is meant to protect the data
and keep it sane is RCU.

Note that sometimes we do not properly handle the case one field can be
written by a writer holding RTNL (or socket lock or something else)

We often believe compiler wont do something stupid, but it can
sometimes.

We definitely should scrutinize things a bit more, or maybe add __rcu
like annotations to catch potential problems earlier.

This is my hunch from looking at dev_ioctl().  For some of the other fields, there is additional support to detect a write during the read, but not any of the ioctls handled in dev_ifsioc_locked().  For example, SIOCGIFNAME, dev_ifname() calls netdev_get_name() to copy dev->name, which uses devnet_rename_seq seqcount to detect if another thread called dev_change_name() and updated the name.

I found more examples of accessing net_device fields in net-sysfs.c and these instances are all acquire dev_base_lock/rtnl_lock before reading fields.  Maybe dev_ioctl should be implemented this way

Joel