[PATCH v3 3/8] qemu: add /dev/mshv to default cgroup acl

Praveen K Paladugu posted 8 patches 5 months, 1 week ago
There is a newer version of this series
[PATCH v3 3/8] qemu: add /dev/mshv to default cgroup acl
Posted by Praveen K Paladugu 5 months, 1 week ago
Add /dev/mshv to default set of devices allowed into domain's cgroup.

Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
---
 src/qemu/qemu.conf.in              | 2 +-
 src/qemu/qemu_cgroup.c             | 2 +-
 src/qemu/test_libvirtd_qemu.aug.in | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index fc91ba8f08..1b5b96c37f 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -618,7 +618,7 @@
 #cgroup_device_acl = [
 #    "/dev/null", "/dev/full", "/dev/zero",
 #    "/dev/random", "/dev/urandom",
-#    "/dev/ptmx", "/dev/kvm",
+#    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
 #    "/dev/userfaultfd"
 #]
 #
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index f10976c2b0..b9b68d38d5 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -41,7 +41,7 @@ VIR_LOG_INIT("qemu.qemu_cgroup");
 const char *const defaultDeviceACL[] = {
     "/dev/null", "/dev/full", "/dev/zero",
     "/dev/random", "/dev/urandom",
-    "/dev/ptmx", "/dev/kvm",
+    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
     "/dev/userfaultfd",
     NULL,
 };
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 90012b3f52..0deae04b83 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -77,7 +77,8 @@ module Test_libvirtd_qemu =
     { "5" = "/dev/urandom" }
     { "6" = "/dev/ptmx" }
     { "7" = "/dev/kvm" }
-    { "8" = "/dev/userfaultfd" }
+    { "8" = "/dev/mshv" }
+    { "9" = "/dev/userfaultfd" }
 }
 { "save_image_format" = "raw" }
 { "dump_image_format" = "raw" }
-- 
2.50.1
Re: [PATCH v3 3/8] qemu: add /dev/mshv to default cgroup acl
Posted by Daniel P. Berrangé via Devel 4 months, 1 week ago
On Fri, Aug 29, 2025 at 03:28:33PM -0500, Praveen K Paladugu wrote:
> Add /dev/mshv to default set of devices allowed into domain's cgroup.
> 
> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
> ---
>  src/qemu/qemu.conf.in              | 2 +-
>  src/qemu/qemu_cgroup.c             | 2 +-
>  src/qemu/test_libvirtd_qemu.aug.in | 3 ++-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
> index fc91ba8f08..1b5b96c37f 100644
> --- a/src/qemu/qemu.conf.in
> +++ b/src/qemu/qemu.conf.in
> @@ -618,7 +618,7 @@
>  #cgroup_device_acl = [
>  #    "/dev/null", "/dev/full", "/dev/zero",
>  #    "/dev/random", "/dev/urandom",
> -#    "/dev/ptmx", "/dev/kvm",
> +#    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
>  #    "/dev/userfaultfd"
>  #]
>  #
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index f10976c2b0..b9b68d38d5 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -41,7 +41,7 @@ VIR_LOG_INIT("qemu.qemu_cgroup");
>  const char *const defaultDeviceACL[] = {
>      "/dev/null", "/dev/full", "/dev/zero",
>      "/dev/random", "/dev/urandom",
> -    "/dev/ptmx", "/dev/kvm",
> +    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
>      "/dev/userfaultfd",
>      NULL,
>  };

This is not your fault, but now we're adding MSHV support the
pre-existing mistake is more obvious. We ought not hardcode
/dev/kvm or /dev/mshv in this global ACL, as when using TCG
emulation the QEMU process should not have access to either
of these devices.

IMHO we should change our code to add /dev/kvm or /dev/mshv
to the allow list dynamically based on whether the guest is
configured to use KVM or MSHV.

So could you drop this patch and instead create two other
patches:

 1. Drop /dev/kvm from the defaultDevice ACL & config,
    while modifying qemuCgroupAllowDevicesPaths to dynamically
    grant /dev/kvm if vm->def->virtType == VIR_DOMAIN_VIRT_KVM

 2. Grant /dev/mshv if vm->def->virtType = VIR_DOMAIN_VIRT_HYPERV

> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
> index 90012b3f52..0deae04b83 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -77,7 +77,8 @@ module Test_libvirtd_qemu =
>      { "5" = "/dev/urandom" }
>      { "6" = "/dev/ptmx" }
>      { "7" = "/dev/kvm" }
> -    { "8" = "/dev/userfaultfd" }
> +    { "8" = "/dev/mshv" }
> +    { "9" = "/dev/userfaultfd" }
>  }
>  { "save_image_format" = "raw" }
>  { "dump_image_format" = "raw" }

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v3 3/8] qemu: add /dev/mshv to default cgroup acl
Posted by Praveen K Paladugu 3 months, 3 weeks ago

On 10/1/2025 4:52 AM, Daniel P. Berrangé wrote:
> On Fri, Aug 29, 2025 at 03:28:33PM -0500, Praveen K Paladugu wrote:
>> Add /dev/mshv to default set of devices allowed into domain's cgroup.
>>
>> Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com>
>> ---
>>   src/qemu/qemu.conf.in              | 2 +-
>>   src/qemu/qemu_cgroup.c             | 2 +-
>>   src/qemu/test_libvirtd_qemu.aug.in | 3 ++-
>>   3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
>> index fc91ba8f08..1b5b96c37f 100644
>> --- a/src/qemu/qemu.conf.in
>> +++ b/src/qemu/qemu.conf.in
>> @@ -618,7 +618,7 @@
>>   #cgroup_device_acl = [
>>   #    "/dev/null", "/dev/full", "/dev/zero",
>>   #    "/dev/random", "/dev/urandom",
>> -#    "/dev/ptmx", "/dev/kvm",
>> +#    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
>>   #    "/dev/userfaultfd"
>>   #]
>>   #
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index f10976c2b0..b9b68d38d5 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -41,7 +41,7 @@ VIR_LOG_INIT("qemu.qemu_cgroup");
>>   const char *const defaultDeviceACL[] = {
>>       "/dev/null", "/dev/full", "/dev/zero",
>>       "/dev/random", "/dev/urandom",
>> -    "/dev/ptmx", "/dev/kvm",
>> +    "/dev/ptmx", "/dev/kvm", "/dev/mshv",
>>       "/dev/userfaultfd",
>>       NULL,
>>   };
> 
> This is not your fault, but now we're adding MSHV support the
> pre-existing mistake is more obvious. We ought not hardcode
> /dev/kvm or /dev/mshv in this global ACL, as when using TCG
> emulation the QEMU process should not have access to either
> of these devices.
> 
> IMHO we should change our code to add /dev/kvm or /dev/mshv
> to the allow list dynamically based on whether the guest is
> configured to use KVM or MSHV.
> 
> So could you drop this patch and instead create two other
> patches:
> 
>   1. Drop /dev/kvm from the defaultDevice ACL & config,
>      while modifying qemuCgroupAllowDevicesPaths to dynamically
>      grant /dev/kvm if vm->def->virtType == VIR_DOMAIN_VIRT_KVM
> 
>   2. Grant /dev/mshv if vm->def->virtType = VIR_DOMAIN_VIRT_HYPERV

As the commands to query acclerators is being refactored, I sent a 
self-contained patch to address this feedback.

"qemu: Drop /dev/kvm from default device ACL" is the patch I sent to the
mailing list

> 
>> diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
>> index 90012b3f52..0deae04b83 100644
>> --- a/src/qemu/test_libvirtd_qemu.aug.in
>> +++ b/src/qemu/test_libvirtd_qemu.aug.in
>> @@ -77,7 +77,8 @@ module Test_libvirtd_qemu =
>>       { "5" = "/dev/urandom" }
>>       { "6" = "/dev/ptmx" }
>>       { "7" = "/dev/kvm" }
>> -    { "8" = "/dev/userfaultfd" }
>> +    { "8" = "/dev/mshv" }
>> +    { "9" = "/dev/userfaultfd" }
>>   }
>>   { "save_image_format" = "raw" }
>>   { "dump_image_format" = "raw" }
> 
> With regards,
> Daniel

-- 
Regards,
Praveen K Paladugu