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
Details
Details
- Reviewers
jhb markj - Group Reviewers
bhyve - Commits
- rS349948: Tie the name limit of a VM to SPECNAMELEN from devfs instead of a
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - 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. |
Comment Actions
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.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/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. |
| 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. |