netdev - Re: [RFC PATCH net-next 05/13] net: bonding: hold the netdev lists lock when retrieving device statistics

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: <20201207010040.eriknpcidft3qul6@skbuf>
Date:   Mon, 7 Dec 2020 01:00:40 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Jiri Benc <jbenc@...hat.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Eric Dumazet <edumazet@...gle.com>,
        George McCollister <george.mccollister@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [RFC PATCH net-next 05/13] net: bonding: hold the netdev lists
 lock when retrieving device statistics

On Mon, Dec 07, 2020 at 01:59:11AM +0200, Vladimir Oltean wrote:
> In the effort of making .ndo_get_stats64 be able to sleep, we need to
> ensure the callers of dev_get_stats do not use atomic context.
>
> The bonding driver uses an RCU read-side critical section to ensure the
> integrity of the list of network interfaces, because the driver iterates
> through all net devices in the netns to find the ones which are its
> configured slaves. We still need some protection against an interface
> registering or deregistering, and the writer-side lock, the netns mutex,
> is fine for that, because it offers sleepable context.
>
> This mutex now serves double duty. It offers code serialization,
> something which the stats_lock already did. So now that serves no
> purpose, let's remove it.
>
> Cc: Jay Vosburgh <j.vosburgh@...il.com>
> Cc: Veaceslav Falico <vfalico@...il.com>
> Cc: Andy Gospodarek <andy@...yhouse.net>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

There is a very obvious deadlock here which happens when we have
bond-over-bond and the upper calls dev_get_stats from the lower.

Conceptually, the same can happen even in any number of stacking
combinations between bonding, net_failover, [ insert any other driver
that takes net->netdev_lists_lock here ].

There would be two approaches trying to solve this issue:
- using mutex_lock_nested where we aren't sure that we are top level
- ensuring through convention that user space always takes
  net->netdev_lists_lock when calling dev_get_stats, and documenting
  that, and therefore making it unnecessary to lock in bonding.

I took neither of the two approaches (I don't really like either one too
much), hence [ one of ] the reasons for the RFC. Comments?