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

Message-ID: <CAM_iQpW_yg+=rqz90vicP5LcPdGiXA0X4_zqQ-gp1zFR5rn=bQ@mail.gmail.com>
Date:   Sun, 5 Feb 2017 22:29:07 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Joel Cunningham <joel.cunningham@...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 Fri, Feb 3, 2017 at 7:25 PM, Joel Cunningham <joel.cunningham@...com> wrote:
> 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.
>  */

This comment is pretty old, it was added prior to git history.


> 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
>

I think dev_base_lock is supposed to speed up the readers when
we only read one or a few fields from netdevice, otherwise it would
be pretty pointless since we already have the RTNL lock.

Unfortunately, as you noticed, not all of these fields are protected
by dev_base_lock, therefore the readers who only take this read
lock is not enough to read an atomic result.

RCU doesn't seem to be the solution here, since it still requires
a whole copy of netdevice even we only update, for example MTU.
This is very inconvenient.

It is also kinda messy due to the mix of dev_base_lock, RCU,
and RTNL. Sigh...