> On Feb 3, 2017, at 3:40 PM, Cong Wang <xiyou.wangc...@gmail.com> wrote: > > On Thu, Feb 2, 2017 at 6:05 PM, Joel Cunningham <joel.cunning...@me.com> > wrote: >> >> In the case of SIOCSIFHWADDR, we get a pointer to the net_device through >> __dev_get_by_name() and then pass it to dev_set_mac_address() to modify >> through ndo_set_mac_address(). I didn’t see any uses of RCU APIs on the >> writer side and that’s why I figured there was something going on with >> rtnl_lock() that I didn’t understand or that the dev_ioctl function wasn’t >> re-entrant from another CPU >> > > You are right, that RCU read lock could merely protect the netdevice from > being unregistered concurrently, can't prevent a concurrent dev_ifsioc(). > > I don't know why Eric changed it to RCU read lock, it is not a hot path, using > rtnl lock is fine and can guarantee a atomic read.
Thanks for confirming what I was seeing. I took a look through the history and the change happened in 3710becf8a58a5c6c4e797e3a3c968c161abdb41. It was previously holding the dev_base_lock(). From the documentation in dev.c: /* * The @dev_base_head list is protected by @dev_base_lock and the rtnl * semaphore. * * Pure readers hold dev_base_lock for reading, or rcu_read_lock() * * Writers must hold the rtnl semaphore while they loop through the * dev_base_head list, and hold dev_base_lock for writing when they do the * actual updates. This allows pure readers to access the list even * while a writer is preparing to update it. * * To put it another way, dev_base_lock is held for writing only to * protect against pure readers; the rtnl semaphore provides the * protection against other writers. * * See, for example usages, register_netdevice() and * unregister_netdevice(), which must be called with the rtnl * semaphore held. */ Is the correct usage is to hold both rtnl_lock() and dev_base_lock when modifying a member of a struct net_device? The wording seems vague as to which synchronization issue holding both covers. What does “do the actual update” mean, updating the list or structure member? If the latter, then maybe the concurrent dev_ioctl() case has never been safe Joel
