if_ovpn: Destroy cloned interfaces via a prison removal callback
ClosedPublic
Actions

Authored by markj on Fri, Jul 25, 2:22 PM.

Details

Reviewers

Group Reviewers

Commits

rGdfd8306a79b5: if_ovpn: Destroy cloned interfaces via a prison removal callback
rG96b29c7f0cff: if_ovpn: Destroy cloned interfaces via a prison removal callback

Summary

A if_ovpn interface carries a reference to a socket, which has a
credential reference, which holds a reference on the containing prison
and prevents SYSUNINITs from being invoked. So, register a
PR_METHOD_REMOVE callback and destroy the cloner from there instead,
since that mechanism doesn't require the prison refcount to drop to zero
first.

This fixes a bug where jails get left stuck in the DYING state after
running if_ovpn regression tests.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65802
Build 62685: arc lint + arc unit

Event Timeline

markj created this revision.Fri, Jul 25, 2:22 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptFri, Jul 25, 2:22 PM

markj requested review of this revision.Fri, Jul 25, 2:22 PM

Harbormaster completed remote builds in B65713: Diff 159086.Fri, Jul 25, 2:22 PM

markj mentioned this in D51524: jail: Make prison_owns_vnet() operate on a prison instead of a ucred.Fri, Jul 25, 5:28 PM

Should we unconditionally detach the V_ovpn_cloner cloner in the non-VIMAGE case? Or under MOD_UNLOAD?
If I'm reading this right we wouldn't do so now, so we could end up unloading if_ovpn.ko and being left with the cloner functions pointing to now uninitialised memory.

Be sure to detach the root VNET's cloner during module unload.

Harbormaster completed remote builds in B65799: Diff 159320.Mon, Jul 28, 2:25 PM

In D51526#1178362, @kp wrote:

Should we unconditionally detach the V_ovpn_cloner cloner in the non-VIMAGE case? Or under MOD_UNLOAD?
If I'm reading this right we wouldn't do so now, so we could end up unloading if_ovpn.ko and being left with the cloner functions pointing to now uninitialised memory.

Yes, you're completely right. I think it has to happen under MOD_UNLOAD.

kp added inline comments.Mon, Jul 28, 2:33 PM

sys/net/if_ovpn.c
2608	I think I'd feel better if we did V_ovpn_cloner = NULL here, because I can't quite convince myself that we're not going to double-detach the host vnet's cloner on module unload.

Defend against double detaches of the cloner.

Harbormaster completed remote builds in B65802: Diff 159324.Mon, Jul 28, 2:35 PM

kp added inline comments.Mon, Jul 28, 2:41 PM

sys/net/if_ovpn.c

2605

/usr/src/sys/net/if_ovpn.c:2796:23: error: incompatible pointer types passing 'struct prison *' to parameter of type 'struct ucred *' [-Werror,-Wincompatible-pointer-types]
 2796 |         if (prison_owns_vnet(pr)) {
      |                              ^~
/usr/src/sys/sys/jail.h:438:37: note: passing argument to parameter here
  438 | bool prison_owns_vnet(struct ucred *);
      |                                     ^
1 error generated.

markj added inline comments.Mon, Jul 28, 2:42 PM