netdev - Re: Understanding mutual exclusion between rtnl_lock and rcu_read

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

Message-id: <F7A17A24-4FCC-4036-A7F6-B8374DFD665F@me.com>
Date:   Fri, 03 Feb 2017 21:25:25 -0600
From:   Joel Cunningham <joel.cunningham@...com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Alexander Duyck <alexander.duyck@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: Understanding mutual exclusion between rtnl_lock and rcu_read_lock

> On Feb 3, 2017, at 3:40 PM, Cong Wang <xiyou.wangcong@...il.com> wrote:
> 
> On Thu, Feb 2, 2017 at 6:05 PM, Joel Cunningham <joel.cunningham@...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