kern: Introduce RLIMIT_VMM

[bnovkov]

Summary

This change introduces a new per-UID limit for controlling the
number of vmm instances, in anticipation of unprivileged bhyve.
This limit allows us to control the maximum amount of kernel memory allocated
by the vmm driver and prevent potential memory exhaustion attacks.
The initial value of the limit was chosen arbitrarily and may be
adjusted in the future.

Sponsored by: The FreeBSD Foundation
Sponsored by: Klara, Inc.

[corvink]

sys/dev/vmm/vmm_dev.c

990

Why changing this to thread?

1187

It would be great to explain in the commit message why we want to restrict the number of vmms and especially, why we're choosing this default value.

[bnovkov]

Address @corvink 's comments.

[bnovkov]

sys/dev/vmm/vmm_dev.c

990

Whoops, that was a leftover from an unrelated previous attempt, thanks for catching it!

1187

I've expanded the commit message, but the limit itself is fairly arbitrary. I'm open to suggestions if you think there's a more reasonable value.

[corvink]

sys/dev/vmm/vmm_dev.c

1187

I just prefer that we're adding a short reason why choosing this default limit. If it's just a random pick, it's fine but it's worth mentioning in the commit message that it's just a random pick. So, if someone ever wonders why we've chosen this limit he can check the commit message.

[markj]

usr.bin/procstat/procstat_rlimit.c should be updated too.

sys/dev/vmm/vmm_dev.c

20

This sorts after queue.h.

1187

I agree, there should be a comment.

Thinking a bit more, having a max of 1024 is a bit silly, it's possibly too low on large count-count machines. Maybe let's just have it be 4*mp_ncpus or 8*mp_ncpus?

Also this needs to be overridable with a tunable, like hw.vmm.maxcpu above.

sys/kern/kern_resource.c

1776

[bnovkov]

Address @markj 's comments

[markj]

sys/dev/vmm/vmm_dev.c

1008

You'd need to decrement the counter again in the error cases below. Better would be to postpone it.

[bnovkov]

Defer incrementing the vmm counter.

[markj]

sys/dev/vmm/vmm_dev.c

1032

You need to destroy the cdev here too. vmmdev_destroy() won't do it for you.

[bnovkov]

sys/dev/vmm/vmm_dev.c

1032

whoops, that was silly, fixed!

[olce]

There are already inline comments asking for a comment on the current limit. If possible, I think it would be great also to explain the point of this resource limit. In particular, which resources a VMM consumes that is not already accounted into existing resource limits, such as for memory: RLIMIT_AS, RLIMIT_RSS, RLIMIT_STACK, or CPU: RLIMIT_CPU? I suspect some kernel memory? Are there hardware limitations to the number of VMs that can be created?

sys/dev/vmm/vmm_dev.c

906–907

Small in-passing question: Can sc->ucred really be NULL here? I'm asking to be able to verify that chgvmmcnt() calls are indeed paired correctly.

[bnovkov]

Address @olce 's comments.

[bnovkov]

In D53728#1231626, @olce wrote:

There are already inline comments asking for a comment on the current limit. If possible, I think it would be great also to explain the point of this resource limit. In particular, which resources a VMM consumes that is not already accounted into existing resource limits, such as for memory: RLIMIT_AS, RLIMIT_RSS, RLIMIT_STACK, or CPU: RLIMIT_CPU? I suspect some kernel memory? Are there hardware limitations to the number of VMs that can be created?

Thank you for the suggestion, the limit covers kernel memory allocated by the vmm driver (both the MI and platform-specific parts) and is meant to prevent resource exhaustion.
I've reworded the commit message to include this point.

sys/dev/vmm/vmm_dev.c

906–907

hm, I don't think it can since it'll always get set in vm_create. The same goes for sc->vm as well.
Looking at the commit history, I am not sure why these check have been placed.
I've converted the sc->ucred check into an assertion and will do the same for sc->vm in a separate patch.