Re: Correct usage of dev_base_lock in 2020

Vladimir Oltean
2020-12-01T18:59:15Z

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