Increase VM name limit
ClosedPublic
Actions

Authored by scottl on Jul 11 2019, 6:00 PM.

Details

Reviewers

jhb
markj

Group Reviewers

bhyve

Commits

rS349948: Tie the name limit of a VM to SPECNAMELEN from devfs instead of a

Summary

Tie the name limit of a VM to SPECNAMELEN from devfs instead of a
hard-coded value. Don't allocate space for it from the kernel stack.
Account for prefix, suffix, and separator space in the name. This
takes the effective length up to 229 bytes on 13-current, and 37 bytes
on 12-stable. 37 bytes is enough to hold a full GUID, which has been
asked for in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234134

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25322
Build 23983: arc lint + arc unit

Event Timeline

scottl created this revision.Jul 11 2019, 6:00 PM

Herald added a reviewer: bhyve. · View Herald TranscriptJul 11 2019, 6:00 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

Harbormaster completed remote builds in B25298: Diff 59643.Jul 11 2019, 6:00 PM

scottl added a reviewer: jhb.Jul 11 2019, 6:21 PM

scottl added a reviewer: markj.Jul 11 2019, 10:15 PM

markj added inline comments.Jul 12 2019, 3:39 PM

sys/amd64/include/vmm.h
128	Should this be "VM name" instead of "VM string"?
sys/amd64/vmm/vmm_dev.c
912–915	Shouldn't we add one byte for a nul terminator here? VM_MAX_NAMELEN, like SPECNAMELEN, doesn't count the nul terminator anymore with this change.
986–989	Same here.

jhb added inline comments.Jul 12 2019, 4:56 PM

sys/amd64/include/vmm_dev.h
54	I think this is an ABI change (this structure is used by an ioctl), so it wouldn't be MFC'able without some compat shims. It doesn't hurt to just leave this as SPECNAMELEN though, at least for an MFC. I don't think we try to let bhyve binaries from older stable branches work, but we probably want them to work if it's within the same stable branch.
sys/amd64/vmm/vmm_dev.c
268–269	It might be nicer to use sizeof() instead of the constants as then there'd be less to change for an MFC if you left the memseg size as SPECNAMELEN in an MFC. In this case dsc->name is always a valid C string, so it would just be 'sizeof(mseg->name)' instead of the constant.
294–295	I would use sizeof() here as well. devfs just ignores the trailing nul, it doesn't actively try to resize the allocation?
901–902	FWIW, even with the larger size, it's probably ok for this to be on the stack still. sysctl handlers are called from a pretty shallow stack (sys__sysctl -> userland_sysctl -> sysctl_root -> sysctl_root_handler_locked -> handler). It's also fine to malloc. It's not a critical path so the cost of malloc doesn't matter.

Fix and expand comments, add a compile time assert for safety.
Pad the buffer length in the sysctl handlers to the null terminator.

Harbormaster completed remote builds in B25319: Diff 59692.Jul 12 2019, 5:02 PM

scottl added inline comments.Jul 12 2019, 5:13 PM

sys/amd64/include/vmm_dev.h
54	It was always bogus to use SPECNAMELEN here, but it's fine if this part doesn't get MFC'd as-is.
sys/amd64/vmm/vmm_dev.c
294–295	Are you saying that we should drop the +1?

Switch to using sizeof() inplace of prescribing VM_MAX_SUFFIXLEN

Harbormaster completed remote builds in B25322: Diff 59695.Jul 12 2019, 5:17 PM

scottl marked 4 inline comments as done.Jul 12 2019, 5:18 PM

jhb added inline comments.Jul 12 2019, 5:43 PM

sys/amd64/include/vmm.h
139	One last nit. Can we keep as many of these constants under #ifdef _KERNEL to avoid leaking them into userland and ending up as part of the ABI? Maybe at least VM_MIN_NAMELEN and VM_MAX_NAMELEN could be under #ifdef _KERNEL still. Does anything use VM_MIN_NAMELEN btw?

markj added inline comments.Jul 12 2019, 6:00 PM

sys/amd64/vmm/vmm_dev.c
954	It is redundant to set `error` here.
1038	It is redundant to set `error` here.

scottl added inline comments.Jul 12 2019, 6:31 PM

sys/amd64/include/vmm.h
139	MIN is used by the CTASSERT below. I'm deliberately keeping them out of _KERNEL because the app should know the limitations on the lengths before passing them to the kernel. The only one that the app can't do much about is VM_MAX_PREFIXLEN, but separating out just that one has minimal value.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 12 2019, 6:38 PM

Closed by commit rS349948: Tie the name limit of a VM to SPECNAMELEN from devfs instead of a (authored by scottl). · Explain Why

This revision was automatically updated to reflect the committed changes.

scottl added a commit: rS349948: Tie the name limit of a VM to SPECNAMELEN from devfs instead of a.

jhb added inline comments.Jul 12 2019, 7:17 PM

sys/amd64/include/vmm.h
139	Are you planning on changing userland vs just letting it return the error from the ioctl / sysctl when it fails as it does today? If not, I think it's better to let the limit be set by the kernel and not really visible to the application as a constant.