On Mon, Dec 07, 2020 at 01:59:11AM +0200, Vladimir Oltean wrote: > In the effort of making .ndo_get_stats64 be able to sleep, we need to > ensure the callers of dev_get_stats do not use atomic context. > > The bonding driver uses an RCU read-side critical section to ensure the > integrity of the list of network interfaces, because the driver iterates > through all net devices in the netns to find the ones which are its > configured slaves. We still need some protection against an interface > registering or deregistering, and the writer-side lock, the netns mutex, > is fine for that, because it offers sleepable context. > > This mutex now serves double duty. It offers code serialization, > something which the stats_lock already did. So now that serves no > purpose, let's remove it. > > Cc: Jay Vosburgh <j.vosbu...@gmail.com> > Cc: Veaceslav Falico <vfal...@gmail.com> > Cc: Andy Gospodarek <a...@greyhouse.net> > Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com> > ---
There is a very obvious deadlock here which happens when we have bond-over-bond and the upper calls dev_get_stats from the lower. Conceptually, the same can happen even in any number of stacking combinations between bonding, net_failover, [ insert any other driver that takes net->netdev_lists_lock here ]. There would be two approaches trying to solve this issue: - using mutex_lock_nested where we aren't sure that we are top level - ensuring through convention that user space always takes net->netdev_lists_lock when calling dev_get_stats, and documenting that, and therefore making it unnecessary to lock in bonding. I took neither of the two approaches (I don't really like either one too much), hence [ one of ] the reasons for the RFC. Comments?
