search

canonical / lxd

Powerful system container and virtual machine manager
https://canonical.com/lxd
GNU Affero General Public License v3.0
4.33k stars 929 forks source link

Device: Fix TPM name errors #13671

Closed hamistao closed 2 months ago

hamistao commented 3 months ago

This fixes TPM failing due to the device ID being trimmed if too long, that caused any references to that device to be incorrect after trimming. To reproduce the issue: 1.LONG_DEVICE_NAME="device-with-very-long-name-and---4-qemu-property-handling-test_" 2.lxc init ubuntu:n vm --vm 3.lxc config device add vm "${LONG_DEVICE_NAME}" tpm 4.lxc start vm This also allows using / in TPM names

tomponline commented 3 months ago

device ID being trimmed if too long, that caused any references to that device to be incorrect after trimming.

What was doing the trimming?

Also does this change the disk names, mount tags or device ids in anyway?

hamistao commented 3 months ago

@tomponline QEMU internally truncates section IDs (in this case chardev and tpmdev IDs) to 63 characters, the same process is mentioned in #13597, #13596 and #13672. Since the device name was previously included without escaping and now it is, TPM IDs that only after escaping - to -- become longer than the limit of 63 are being changed by this PR, but this is such a specific case thay may not be a problem. Aside from that case, the only properties that this PR changes are TPM section IDs that are longer than 63 characters and thus did not previously work at all. Mount tags, device node names and other device IDs are generated in the same manner as before.

hamistao commented 3 months ago

@tomponline Also, please correct me if I am wrong. I believe this does not require any work done on the LXD agent side because TPMs can only be added on stopped VMs.

tomponline commented 3 months ago

Also, please correct me if I am wrong. I believe this does not require any work done on the LXD agent side because TPMs can only be added on stopped VMs.

You are correct, the tpm side im not so worried about, but the changes you've made in this PR could also affect disk passthrough devices, so I am seeking confirmation from you that you have considered both directory and block disk passthrough names inside the VM have remained unchanged. Ta

hamistao commented 3 months ago

I ran some tests comparing qemu.conf files and block node names before and after the changes and did not find any inconsistencies. The VMs used included devices with different name lenghts. If you have any more ideas of tests that I should try please let me know.

tomponline commented 3 months ago

I ran some tests comparing qemu.conf files and block node names before and after the changes and did not find any inconsistencies. The VMs used included devices with different name lenghts. If you have any more ideas of tests that I should try please let me know.

Worth checking hotplugging (as that doesnt use qemu.conf but qmp) and then check the end result in the guest (i.e /dev/disk/by-id) and the mount tag.

hamistao commented 3 months ago

@tomponline This is safe to merge. Both entries for the device on /dev/disk/by-id are generated from the serial name (and not the device ID, name or block node name), so this could not affect them. This can be seen here