Re: Correct usage of dev_base_lock in 2020

[Vladimir Oltean <olteanv@gmail.com>]

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@netfilter.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