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