Skip to content

Conversation

@yafu-1
Copy link
Contributor

@yafu-1 yafu-1 commented Sep 11, 2025

Guest will crash if booting guest with interface multiqueue enabled and virtqemud in foregound mode since running daemons from cli is considered unsupported by selinux-policy.

It reports avc dened error without the fix:
time->Thu Sep 11 01:39:57 2025
type=PROCTITLE msg=audit(1757569197.527:38921): proctitle=2F7573722F6C6962657865632F71656D752D6B766D002D6E616D650067756573743D61766F6361646F2D76742D766D312C64656275672D746872656164733D6F6E002D53002D6F626A656374007B22716F6D2D74797065223A22736563726574222C226964223A226D61737465724B657930222C22666F726D6174223A227261
type=SYSCALL msg=audit(1757569197.527:38921): arch=c000003e syscall=16 success=no exit=-13 a0=1a a1=400454d9 a2=7fa6f1836d70 a3=0 items=0 ppid=1 pid=550563 auid=0 uid=107 gid=107 euid=107 suid=107 fsuid=107 egid=107 sgid=107 fsgid=107 tty=(none) ses=8 comm=43505520302F4B564D exe="/usr/libexec/qemu-kvm" subj=unconfined_u:unconfined_r:svirt_t:s0:c790,c975 key=(null)
type=AVC msg=audit(1757569197.527:38921): avc: denied { attach_queue } for pid=550563 comm=43505520302F4B564D scontext=unconfined_u:unconfined_r:svirt_t:s0:c790,c975 tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tclass=tun_socket permissive=0

After the fix, the case pass and no avc dened error:
$ avocado run daemon.crash_regression.shutdown_console --vt-type libvirt
JOB LOG : /var/log/avocado/job-results/job-2025-09-11T03.42-0320716/job.log
(1/1) type_specific.io-github-autotest-libvirt.daemon.crash_regression.shutdown_console: STARTED
(1/1) type_specific.io-github-autotest-libvirt.daemon.crash_regression.shutdown_console: PASS (134.41 s)
RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /var/log/avocado/job-results/job-2025-09-11T03.42-0320716/results.html
JOB TIME : 136.40 s

$ ausearch -m avc --start recent

Summary by CodeRabbit

  • Tests
    • Enhanced VM lifecycle test to temporarily adjust the first network interface’s queue settings for more deterministic execution during shutdown/start sequences.
    • Introduced automatic backup and full restoration of the VM configuration within the test to prevent persistent changes and ensure environment cleanliness.
    • Applied changes dynamically during the test run and guaranteed cleanup in all outcomes for improved test reliability and isolation.

@coderabbitai
Copy link

coderabbitai bot commented Sep 11, 2025

Walkthrough

The test modifies the first network interface’s driver queues to 1 by editing VM XML, applies it using virsh.update_device, starts and shuts down the VM, and restores the original XML in a finally block using a backup.

Changes

Cohort / File(s) Change Summary
Crash regression test adjustment
libvirt/tests/src/daemon/crash_regression.py
Adds logic in run_shutdown_console to: backup VM XML; set first NIC driver queues=1 via VMXML; apply via virsh.update_device; start and shut down VM; restore original XML in finally by syncing backup.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant T as Test (run_shutdown_console)
    participant X as VMXML
    participant V as virsh.update_device
    participant H as Hypervisor/VM

    rect rgb(235,245,255)
    note over T: Prepare and modify VM NIC driver queues=1
    T->>X: Backup current VM XML
    T->>X: Edit first NIC driver (queues=1)
    T->>V: Apply device update
    V-->>T: Update result
    end

    rect rgb(235,255,235)
    note over T,H: Execute lifecycle
    T->>H: Start VM
    H-->>T: VM running
    T->>H: Shutdown VM
    H-->>T: VM stopped
    end

    rect rgb(255,245,235)
    note over T,X: Cleanup
    T->>X: Restore original XML from backup
    X-->>T: Sync complete
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely summarizes the primary change: it states that interface multiqueue is disabled when starting a guest with virtqemud in foreground mode, which matches the PR description and the code change that forces queue=1 on the VM interface; it is specific, relevant, and free of noisy details.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Poem

I nudge the NIC to queue as one,
Start, then yawn—the VM is done.
Backups tucked in burrow tight,
Restore the XML by night.
Whiskers twitch—no crashes near,
Hippity hop, the logs are clear! 🐇

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
libvirt/tests/src/daemon/crash_regression.py (1)

46-46: Guard for missing interfaces.

If the guest has no interfaces, indexing [0] would raise. Add a presence check (and log) before modifying.

-        iface = vm_xml.get_devices(device_type="interface")[0]
+        if not vm_xml.get_devices(device_type="interface"):
+            logging.warning("No interface found; skipping multiqueue disable.")
+            return
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ae482a9 and 5156d3b.

📒 Files selected for processing (1)
  • libvirt/tests/src/daemon/crash_regression.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/daemon/crash_regression.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_change_media.py (1)
  • update_device (113-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.8
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.12

Comment on lines +46 to 55
iface = vm_xml.get_devices(device_type="interface")[0]
driver_dict = dict(iface.driver.driver_attr)
driver_dict['queues'] = '1'
iface.driver = iface.new_driver(driver_attr=driver_dict)
virsh.update_device(vm.name, iface.xml, debug=True)
vm.start(autoconsole=True)
vm.shutdown()
finally:
vm_xml_backup.sync()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix: update persistent XML instead of calling virsh.update_device with raw XML (and missing --config).

virsh.update_device expects a file path and, for an inactive VM, requires --config. As written, this likely no-ops or fails silently, so multiqueue may remain enabled and the crash can persist.

Apply this diff to persist the change before boot and wait for shutdown:

-        iface = vm_xml.get_devices(device_type="interface")[0]
-        driver_dict = dict(iface.driver.driver_attr)
-        driver_dict['queues'] = '1'
-        iface.driver = iface.new_driver(driver_attr=driver_dict)
-        virsh.update_device(vm.name, iface.xml, debug=True)
-        vm.start(autoconsole=True)
-        vm.shutdown()
+        # Disable NIC multiqueue persistently to avoid AVC attach_queue when qemu runs in foreground.
+        devices = vm_xml.devices
+        for dev in devices:
+            if getattr(dev, "device_tag", "") == "interface":
+                driver_attr = dict(getattr(getattr(dev, "driver", None), "driver_attr", {}) or {})
+                driver_attr["queues"] = "1"
+                dev.driver = dev.new_driver(driver_attr=driver_attr)
+                break
+        vm_xml.devices = devices
+        vm_xml.sync()
+        vm.start(autoconsole=True)
+        vm.shutdown()
+        vm.wait_for_shutdown()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
iface = vm_xml.get_devices(device_type="interface")[0]
driver_dict = dict(iface.driver.driver_attr)
driver_dict['queues'] = '1'
iface.driver = iface.new_driver(driver_attr=driver_dict)
virsh.update_device(vm.name, iface.xml, debug=True)
vm.start(autoconsole=True)
vm.shutdown()
finally:
vm_xml_backup.sync()
# Disable NIC multiqueue persistently to avoid AVC attach_queue when qemu runs in foreground.
devices = vm_xml.devices
for dev in devices:
if getattr(dev, "device_tag", "") == "interface":
driver_attr = dict(getattr(getattr(dev, "driver", None), "driver_attr", {}) or {})
driver_attr["queues"] = "1"
dev.driver = dev.new_driver(driver_attr=driver_attr)
break
vm_xml.devices = devices
vm_xml.sync()
vm.start(autoconsole=True)
vm.shutdown()
vm.wait_for_shutdown()
finally:
vm_xml_backup.sync()
🤖 Prompt for AI Agents
libvirt/tests/src/daemon/crash_regression.py lines 46-55: the test modifies the
interface driver XML then calls virsh.update_device with raw XML (and omits
--config), which does not persist the change for the inactive VM; instead write
the updated device XML into the VM's persistent/domain XML (or call virsh
update-device with the --config flag and a file path), ensure you apply the
updated XML to the domain configuration before starting the VM, and wait for the
VM to shutdown to confirm persistence before restoring the backup.

Comment on lines +46 to +50
iface = vm_xml.get_devices(device_type="interface")[0]
driver_dict = dict(iface.driver.driver_attr)
driver_dict['queues'] = '1'
iface.driver = iface.new_driver(driver_attr=driver_dict)
virsh.update_device(vm.name, iface.xml, debug=True)
Copy link
Contributor

@chloerh chloerh Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


        # Workaround bug: Remove multi-queue setting
        vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name)
        libvirt_vmxml.modify_vm_device(vmxml, 'interface', {'driver': None})

We used to have this workaround for managed save tests. Would you like to consider it ?
I also want to make sure if the current fix is a workaround, would it be removed someday ? If so, maybe we could consider adding a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not a workaround since selinux-policy does not support running daemons from cli.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I mean we could replace the 5 lines with libvirt_vmxml.modify_vm_device if it's simply modifying vmxml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants