Page MenuHomeFreeBSD

vmm: Destroy associated VM objects when a jail is destroyed
AcceptedPublic

Authored by cyril_freebsdfoundation.org on Jul 19 2021, 11:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 24 2024, 1:44 AM
Unknown Object (File)
Dec 11 2023, 10:50 PM
Unknown Object (File)
Dec 8 2023, 9:46 AM
Unknown Object (File)
Dec 3 2023, 8:51 PM
Unknown Object (File)
Nov 27 2023, 7:20 AM
Unknown Object (File)
Oct 9 2023, 9:06 AM
Unknown Object (File)
Sep 18 2023, 11:12 AM
Unknown Object (File)
Aug 18 2023, 5:24 AM

Details

Reviewers
markj
jamie
Group Reviewers
bhyve
Summary

...and also when vmm is disabled for a jail.

This revision builds off https://reviews.freebsd.org/D31156.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

markj added inline comments.
sys/amd64/vmm/vmm_dev.c
1007

I would suggest asserting that vmmdev_mtx is held here.

1047

Pedantically, buflen should be a size_t.

1167

This is safe because vmmdev_destroy(), which frees sc, is called asynchronously. If that were not the case, and vmm_destroy() freed sc directly, then this would be a use-after-free, since after vmm_destroy() returns we would continue traversal by following sc->link.

I would suggest coding this a bit more defensively by using SLIST_FOREACH_SAFE to avoid referencing sc after vmm_destroy() returns, in case the implementation of vmm_destroy() changes at some point.

1195

You could get rid of vmm_disabled and just write

if (vfs_flagopt(opts, "allow.novmm", NULL, 0))
    vmmdev_prison_cleanup(pr);
tests/sys/vmm/vmm_remove_jail.sh
80

I tend to think that this file should be merged with the other test file you added in D31156, renaming it to plain vmm_jail.sh or so.

Fixed minor issues and merged test case files.

I'm not thrilled at the redundant call to vfs_flagopt(), which shouldn't be necessary because kern_jail_set has already looked for allow.novmm and set the permission bit accordingly. But by the time vmmdev_prison_set() is called, the old value of the permission bit is forgotten. So you're left with

  • Check the parameter directly (the current approach)
  • Check the permission bit, and call vmmdev_prison_cleanup() if it's not set (even it was already clear before and thus will have nothing to clear)
  • Going through all the effort of keeping your own state just for that one bit

Since all this happens only when jail_set(2) is called, which is hardly ever, there's no performance issue, and I guess it just comes down to what makes the code more readable (definitely not the third choice).

Just thinking out loud - I imagine you've already considered the options yourself and settled on what looked best.

This revision is now accepted and ready to land.Jul 27 2021, 11:10 PM