Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device: Fix TPM name errors #13671

Merged
merged 5 commits into from
Jul 3, 2024
Merged

Conversation

hamistao
Copy link
Contributor

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

@hamistao hamistao force-pushed the fix-tpm-name-errors branch 3 times, most recently from 598026f to b064a7f Compare June 26, 2024 19:22
@tomponline
Copy link
Member

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 hamistao force-pushed the fix-tpm-name-errors branch 2 times, most recently from 610fe75 to b707985 Compare June 26, 2024 21:30
@hamistao
Copy link
Contributor Author

hamistao commented Jun 26, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

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

…ameOrID`

Use a more general function so it is possible to reuse it for TPM devices that have different prefixes. The name change and the additional lenght parameter were introduced so it could also be used for device names.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
…nt tags

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
… IDs

This contains the actual fix for the bug by avoiding trimming the device ID and resulting in mismatches between the device ID and references to it.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

hamistao commented Jun 27, 2024

@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

@tomponline tomponline changed the title Fix TPM name errors Device: Fix TPM name errors Jun 28, 2024
@tomponline tomponline merged commit 2c97e1e into canonical:main Jul 3, 2024
29 checks passed
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