netdev - Re: Correct usage of dev_base

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: <20201201144238.GA5970@salvia>
Date:   Tue, 1 Dec 2020 15:42:38 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        netdev <netdev@...r.kernel.org>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Jiri Benc <jbenc@...hat.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>, fw@...len.de
Subject: Re: Correct usage of dev_base_lock in 2020

On Mon, Nov 30, 2020 at 08:48:28PM +0200, Vladimir Oltean wrote:
[...]
> There are 2 separate classes of problems:
> - We already have two ways of protecting pure readers: via RCU and via
>   the rwlock. It doesn't help if we also add a second way of locking out
>   pure writers. We need to first clean up what we have. That's the
>   reason why I started the discussion about the rwlock.
> - Some callers appear to not use any sort of protection at all. Does
>   this code path look ok to you?
> 
> nfnetlink_rcv_batch
> -> NFT_MSG_NEWCHAIN

This path holds the nft commit mutex.

>    -> nf_tables_addchain
>       -> nft_chain_parse_hook
>          -> nft_chain_parse_netdev
>             -> nf_tables_parse_netdev_hooks
>                -> nft_netdev_hook_alloc
>                   -> __dev_get_by_name
>                      -> netdev_name_node_lookup: must be under RTNL mutex or dev_base_lock protection

The nf_tables_netdev_event() notifier holds the nft commit mutex too.
Assuming worst case, race between __dev_get_by_name() and device
removal, the notifier waits for the NFT_MSG_NEWCHAIN path to finish.
If the nf_tables_netdev_event() notifier wins race, then
__dev_get_by_name() hits ENOENT.

The idea is explained here:

commit 90d2723c6d4cb2ace50fc3b932a2bcc77710450b
Author: Pablo Neira Ayuso <pablo@...filter.org>
Date:   Tue Mar 20 17:00:19 2018 +0100

    netfilter: nf_tables: do not hold reference on netdevice from preparation phase

    The netfilter netdevice event handler hold the nfnl_lock mutex, this
    avoids races with a device going away while such device is being
    attached to hooks from the netlink control plane. Therefore, either
    control plane bails out with ENOENT or netdevice event path waits until
    the hook that is attached to net_device is registered.

I can submit a patch adding a comment so anyone else does not get
confused :-)