Fix deadlock during NRI plugin registration

bobbypage · bobbypage · commit c4893c7e31c3 · 2024-04-18T14:31:32.000-07:00
During NRI external plugin registration: * `acceptPluginConnections()` is called * adaptation lock from `nri/pkg/adaptation` is acquired * `syncFn` is invoked * `syncFn` acquires NRI lock in `pkg/nri/nri.go` During container lifecycle events such as `ContainerStart` * NRI lock is acquired in pkg/nri.go * adaptation lock is acquired in `StateChange()` in `nri/pkg/adaptation` As a result, the locking order during NRI plugin registration is: * adaptation lock -> NRI lock While the locking order during container starts is: * NRI lock -> adaptation lock Due the fact that the locking order is inverted and not consistent, it it possible to encounter a deadlock. To fix the issue, during NRI plugin registration, first acquire the NRI lock (done via `syncFn` call) and only after acquire the adaptation lock. This ensures that NRI plugin registration the locking order is adaption lock -> NRI lock, which is consistent with the locking order during container lifecycle events. Fixes containerd/containerd#10085 Signed-off-by: David Porter <porterdavid@google.com>
diff --git a/pkg/adaptation/adaptation.go b/pkg/adaptation/adaptation.go
@@ -431,18 +431,16 @@ func (r *Adaptation) acceptPluginConnections(l net.Listener) error {
 				continue
 			}
 
-			r.Lock()
-
 			err = r.syncFn(ctx, p.synchronize)
 			if err != nil {
 				log.Infof(ctx, "failed to synchronize plugin: %v", err)
 			} else {
+				r.Lock()
 				r.plugins = append(r.plugins, p)
 				r.sortPlugins()
+				r.Unlock()
 			}
 
-			r.Unlock()
-
 			log.Infof(ctx, "plugin %q connected", p.name())
 		}
 	}()

Original file line number	Diff line number	Diff line change
`@@ -431,18 +431,16 @@ func (r *Adaptation) acceptPluginConnections(l net.Listener) error {`
`431`	`431`	`continue`
`432`	`432`	`}`
`433`	`433`
`434`		`- r.Lock()`
`435`		`-`
`436`	`434`	`err = r.syncFn(ctx, p.synchronize)`
`437`	`435`	`if err != nil {`
`438`	`436`	`log.Infof(ctx, "failed to synchronize plugin: %v", err)`
`439`	`437`	`} else {`
	`438`	`+ r.Lock()`
`440`	`439`	`r.plugins = append(r.plugins, p)`
`441`	`440`	`r.sortPlugins()`
	`441`	`+ r.Unlock()`
`442`	`442`	`}`
`443`	`443`
`444`		`- r.Unlock()`
`445`		`-`
`446`	`444`	`log.Infof(ctx, "plugin %q connected", p.name())`
`447`	`445`	`}`
`448`	`446`	`}()`