libnet/d/bridge: handleFirewalldReloadNw: fix deadlock

[akerouanton]

• Fixes Deadlock when firewalld is reloaded while the bridge driver is initializing #50619
• Bug introduced in Stop firewalld reload re-creating rules for deleted networks #49728 (commit a527e5a)

- What I did

1st commit: libnet/d/bridge: driver: un-embed mutex

The bridge driver was embedding sync.Mutex which is unconventional and makes it harder to analyze locks ordering.

2nd commit: libnet/d/bridge: handleFirewalldReloadNw: fix deadlock

handleFirewalldReloadNw locks d.mu and then d.configNetworks. However, the rest of the driver locks d.configNetworks first and then d.mu.

This could result in deadlocks if handleFirewalldReloadNw is called while the bridge driver is already holding d.configNetworks lock.

Other code paths were checked to ensure that they all follow the same locking order.

Locks ordering analysis

d.configNetwork:

• (*driver).configure() locks d.configNetwork — d.mu not acquired
• (*driver).CreateNetwork() locks d.configNetwork - d.mu not acquired
• (*driver).DeleteNetwork() locks d.configNetwork - d.mu not acquired
• (*driver).handleFirewalldReloadNw locks d.configNetwork - d.mu IS acquired
• (*driver).ProgramExternalConnectivity locks d.configNetwork - d.mu not acquired

d.mu.Lock:

• (*driver).configure() locks d.mu - d.configNetwork not acquired
• (*driver).CreateEndpoint() locks d.mu - d.configNetwork not acquired
• (*driver).createNetwork() locks d.mu - d.configNetwork IS acquired by callers:
  • (*driver).CreateNetwork() LOCKS d.configNetwork -> (*driver).createNetwork()
  • (*driver).configure() LOCKS d.configNetwork -> (*driver).initStore() -> (*driver).populateNetworks() -> (*driver).createNetwork()
• (*driver).CreateNetwork() locks d.mu - d.configNetwork not acquired
• (*driver).DeleteEndpoint() locks d.mu - d.configNetwork not acquired
• (*driver).deleteNetwork() locks d.mu - d.configNetwork IS acquired by caller:
  • (*driver).DeleteNetwork() LOCKS d.configNetwork -> (*driver).deleteNetwork()
• (*driver).EndpointOperInfo() locks d.mu - d.configNetwork not acquired
• (*driver).getNetwork() locks d.mu — d.configNetwork IS acquired by caller:
  • (*driver).Join() doesn't lock d.configNetwork -> (*driver).getNetwork()
  • (*driver).Leave() doesn't lock d.configNetwork -> (*driver).getNetwork()
  • (*driver).ProgramExternalConnectivity() LOCKS d.configNetwork -> (*driver).getNetwork()
• (*driver).getNetworks() locks d.mu — d.configNetwork IS acquired by caller:
  • (*driver).CreateNetwork() LOCKS d.configNetwork -> (*driver).checkConflict() -> (*driver).getNetworks()
• (*driver).handleFirewalldReload() locks d.mu — d.configNetwork not acquired
• (*driver).handleFirewalldReloadNw() locks d.mu - d.configNetwork locked after d.mu

- Human readable description for the release notes

- Fix a deadlock that could happen if a firewalld reload was processed while the bridge networking driver was starting up, or creating or deleting a network, or creating port-mappings

[austinvazquez]

Change for consistent lock ordering LGTM.

[thaJeztah]

FWIW, it's common practice to put the mutex above the fields it protects; in this case (from the changes in this code), it's config and networks, so those could possibly be moved under this one (or the mutex moved to the top).

It's not 100% clear to me what configNetwork protects though. Perhaps worth (could be a follow-up) to add a comment on that as well.

[thaJeztah]

FWIW there's other places where we acquite a lock for configNetwork, but don't hold a lock for networks (mu), or at least not for the whole duration;

moby/daemon/libnetwork/drivers/bridge/bridge_linux.go

Lines 758 to 766 in cf15d5b

	// start the critical section, from this point onward we are dealing with the list of networks
	// so to be consistent we cannot allow that the list changes
	d.configNetwork.Lock()
	defer d.configNetwork.Unlock()
	// check network conflicts
	if err = d.checkConflict(config); err != nil {
	return err
	}

moby/daemon/libnetwork/drivers/bridge/bridge_linux.go

Lines 725 to 730 in cf15d5b

	d.Lock()
	if _, ok := d.networks[id]; ok {
	d.Unlock()
	return types.ForbiddenErrorf("network %s exists", id)
	}
	d.Unlock()

[akerouanton]

Yup, I checked all of them and wrote down my analysis in the PR description. This is the only case where the lock ordering is reversed.

[thaJeztah]

Oh! I missed the collapsed block, nice!

[thaJeztah]

LGTM

but there's definitely improvements to be made; at least the locking strategy seems complex and brittle (and could use touch-ups in documentation)