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: <20201201185822.ggkdhnkmdh3wi5v2@skbuf>
Date:   Tue, 1 Dec 2020 20:58:22 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
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 Tue, Dec 01, 2020 at 03:42:38PM +0100, Pablo Neira Ayuso wrote:
> 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 :-)

Ok, so since you are holding the net->nft.commit_mutex from a code path
that is called under RTNL (call_netdevice_notifiers_info), then you are
indirectly serializing all the other holders of the commit_mutex with
the RTNL mutex.

I think it would really help if you could add a comment, yes.


Some other code paths that call __dev_get_by_name() while not holding
the RTNL mutex or the dev_base_lock:

atrtr_ioctl_addrt() <- atalk_ioctl() <- not under RTNL or dev_base_lock
handle_ip_over_ddp() <- atalk_rcv() <- not under RTNL or dev_base_lock
net/bridge/br_sysfs_if.c: store_backup_port() <- sysfs <- not under RTNL or dev_base_lock

net/netfilter/ipvs/ip_vs_sync.c: start_sync_thread() <- not under RTNL or dev_base_lock

include/net/pkt_cls.h: tcf_change_indev() <- fl_set_key() <- fl_set_parms <- RTNL potentially held depending on bool rtnl_held, dev_base_lock definitely not