Page MenuHomeFreeBSD

Increase VM name limit
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/amd64/include/vmm.h
128 ↗(On Diff #59643)

Should this be "VM name" instead of "VM string"?

sys/amd64/vmm/vmm_dev.c
912 ↗(On Diff #59643)

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.

985 ↗(On Diff #59643)

Same here.

sys/amd64/include/vmm_dev.h
54 ↗(On Diff #59643)

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 ↗(On Diff #59643)

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 ↗(On Diff #59643)

I would use sizeof() here as well. devfs just ignores the trailing nul, it doesn't actively try to resize the allocation?

903 ↗(On Diff #59643)

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.

sys/amd64/include/vmm_dev.h
54 ↗(On Diff #59643)

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 ↗(On Diff #59643)

Are you saying that we should drop the +1?

Switch to using sizeof() inplace of prescribing VM_MAX_SUFFIXLEN

sys/amd64/include/vmm.h
139 ↗(On Diff #59695)

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?

sys/amd64/vmm/vmm_dev.c
954 ↗(On Diff #59695)

It is redundant to set error here.

1038 ↗(On Diff #59695)

It is redundant to set error here.

sys/amd64/include/vmm.h
139 ↗(On Diff #59695)

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
This revision was automatically updated to reflect the committed changes.
sys/amd64/include/vmm.h
139 ↗(On Diff #59695)

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.