Skip to content

Conversation

@ljrcore
Copy link

@ljrcore ljrcore commented Jun 10, 2025

When starting a VM with a disk serial specified, for example:

--disk path=focal-server-cloudimg-amd64-custom.raw,serial=aabbccfjdlsdkjfkldsjlkfjdsklfjdslkjflsdk

The VM fails to boot and the following log is observed:

cloud-hypervisor: 1.041609s: <_disk0_q0> ERROR:virtio-devices/src/thread_helper.rs:53 -- Error running worker: HandleEvent(Failed to process queue (submit): RequestExecuting(BadRequest(InvalidOffset)))

This patch adds support for checking and truncating disk custom serial to avoid VM startup failures.

@ljrcore ljrcore requested a review from a team as a code owner June 10, 2025 13:36
let mut s = Vec::from(s);
if s.len() > VIRTIO_BLK_ID_BYTES as usize {
s.truncate(VIRTIO_BLK_ID_BYTES as usize);
warn!("Serial truncated to {} bytes", VIRTIO_BLK_ID_BYTES);
Copy link
Member

@rbradford rbradford Jun 10, 2025

Choose a reason for hiding this comment

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

Please clarify that it's the block device serial - warn!("Block device serial truncated to {} bytes", VIRTIO_BLK_ID_BYTES)

Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks.

@ljrcore ljrcore force-pushed the fix_blk_serial_len branch from 80c03ff to 48139a0 Compare June 11, 2025 02:17
let serial = serial
.map(Vec::from)
.unwrap_or_else(|| build_serial(&disk_path));
let serial = if let Some(s) = serial {
Copy link
Member

@rbradford rbradford Jun 11, 2025

Choose a reason for hiding this comment

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

Looking at this PR again - I think the commit message is a bit confusing. Something like:

virtio-devices: block: Truncate device serial to maximum supported length

Truncate the device serial to the maximum supported length to ensure that it is correctly matched during guest boot disk discovery.

@rbradford
Copy link
Member

The other option would be to refuse to create the block device if the serial is too long?

@ljrcore ljrcore force-pushed the fix_blk_serial_len branch from 48139a0 to e1d3a5b Compare June 11, 2025 12:52
@ljrcore ljrcore closed this Jun 11, 2025
@ljrcore ljrcore reopened this Jun 11, 2025
@ljrcore
Copy link
Author

ljrcore commented Jun 11, 2025

The other option would be to refuse to create the block device if the serial is too long?

If we decide to refuse to create the block device if the serial is too long, would it be better to do the check in DiskConfig?

@rbradford
Copy link
Member

The other option would be to refuse to create the block device if the serial is too long?

If we decide to refuse to create the block device if the serial is too long, would it be better to do the check in DiskConfig?

Yes - do you want to give it a go?

.unwrap_or_else(|| build_serial(&disk_path));
let serial = if let Some(s) = serial {
let mut s = Vec::from(s);
if s.len() > VIRTIO_BLK_ID_BYTES as usize {
Copy link
Contributor

@russell-islam russell-islam Jun 12, 2025

Choose a reason for hiding this comment

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

May be we should not let user create the device, we are modifying the content provided by the user, Who knows user might use this value to track something?

@ljrcore
Copy link
Author

ljrcore commented Jun 12, 2025

The other option would be to refuse to create the block device if the serial is too long?

If we decide to refuse to create the block device if the serial is too long, would it be better to do the check in DiskConfig?

Yes - do you want to give it a go?

Yes, I would love to and will update this PR.

@ljrcore ljrcore force-pushed the fix_blk_serial_len branch from e1d3a5b to c635c01 Compare June 12, 2025 12:33
@ljrcore ljrcore force-pushed the fix_blk_serial_len branch from c635c01 to 7f49963 Compare June 12, 2025 12:45
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Very happy with this version.

@rbradford rbradford changed the title virtio-devices: Add check for specified virtio block device serial vmm: config: Add DiskConfig check for device serial length Jun 12, 2025
@rbradford rbradford added this pull request to the merge queue Jun 12, 2025
Merged via the queue into cloud-hypervisor:main with commit f151fdb Jun 12, 2025
40 of 41 checks passed
@likebreath likebreath moved this from 🆕 New to ✅ Done in Cloud Hypervisor Roadmap Jul 16, 2025
@likebreath likebreath added the bug-fix Bug fix to include in release notes label Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix Bug fix to include in release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants