[libvirt PATCH] vircgroupv2: fix virCgroupV2DenyDevice

Pavel Hrdina posted 1 patch 3 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/6ece0beb74c0ca4bf649d4d12714224320ea894c.1606720919.git.phrdina@redhat.com
src/util/vircgroupv2.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[libvirt PATCH] vircgroupv2: fix virCgroupV2DenyDevice
Posted by Pavel Hrdina 3 years, 5 months ago
The original logic is incorrect. We would delete the device entry
from eBPF map only if the newval would be same as current val in the
map. In case that the device was allowed only as read-only but later
we remove all permissions for that device it would remain in the table
with empty values.

The old code would still deny the device but it's not working as
intended. Instead we will update the value in advance. If the updated
value is 0 it means that we are removing all permissions so it should
be removed from the map, otherwise we will update the value in map.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1810356

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/util/vircgroupv2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 2b32f614e4..40cfa20376 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -1796,7 +1796,9 @@ virCgroupV2DenyDevice(virCgroupPtr group,
         return 0;
     }
 
-    if (newval == val) {
+    val = val & ~newval;
+
+    if (val == 0) {
         if (virBPFDeleteElem(group->unified.devices.mapfd, &key) < 0) {
             virReportSystemError(errno, "%s",
                                  _("failed to remove device from BPF cgroup map"));
@@ -1804,7 +1806,6 @@ virCgroupV2DenyDevice(virCgroupPtr group,
         }
         group->unified.devices.count--;
     } else {
-        val ^= val & newval;
         if (virBPFUpdateElem(group->unified.devices.mapfd, &key, &val) < 0) {
             virReportSystemError(errno, "%s",
                                  _("failed to update device in BPF cgroup map"));
-- 
2.28.0

Re: [libvirt PATCH] vircgroupv2: fix virCgroupV2DenyDevice
Posted by Michal Privoznik 3 years, 5 months ago
On 11/30/20 8:22 AM, Pavel Hrdina wrote:
> The original logic is incorrect. We would delete the device entry
> from eBPF map only if the newval would be same as current val in the
> map. In case that the device was allowed only as read-only but later
> we remove all permissions for that device it would remain in the table
> with empty values.
> 
> The old code would still deny the device but it's not working as
> intended. Instead we will update the value in advance. If the updated
> value is 0 it means that we are removing all permissions so it should
> be removed from the map, otherwise we will update the value in map.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1810356
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>   src/util/vircgroupv2.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal