Skip to content
This repository was archived by the owner on May 20, 2025. It is now read-only.

Fix Command::DmaMap#34

Merged
likebreath merged 2 commits intorust-vmm:mainfrom
blitz:fixes
Apr 30, 2025
Merged

Fix Command::DmaMap#34
likebreath merged 2 commits intorust-vmm:mainfrom
blitz:fixes

Conversation

@blitz
Copy link
Contributor

@blitz blitz commented Apr 23, 2025

Summary of the PR

This PR fixes handling the DmaMap command. There are two issues:

  1. When a DmaMap message arrives with one file descriptor, the backend still receives None in its dma_map call, because for fds.len() == 1 the code passes None instead of fds[0].
  2. If the callee wants to hang on to the File object it gets via backend.dma_map(), they have a hard time, because we currently only pass a &File. Change this to File to give the callee more options to hang on to the File without resorting to unsafe libc::dup().

Technically only 1 is strictly necessary to get DMA mapping working:

2025-04-23T15:50:37.487297Z  INFO usbvfiod: dma_map flags = READ_ONLY | WRITE_ONLY | READ_WRITE offset = 0 address = 4294967296 size = 1073741824 fd = Some(File { fd: 6, path: "/memfd:ch_ram (deleted)", read: true, write: true })

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@blitz blitz changed the title vfio-user: fix an off-by-one in DmaMap Fix Command::DmaMap Apr 23, 2025
@rbradford
Copy link
Collaborator

@likebreath Please review

Copy link

@snue snue left a comment

Choose a reason for hiding this comment

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

The API change to drop the reference and use Option<File> looks like a good simplification to me.

Thanks for fixing the off by one.

Copy link
Collaborator

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

@blitz Thanks for the contributions. Changes are looking good to me.

Would you please update the commit message with prefix server instead of vfio_user? This would be very helpful to clarify the scope of changes, say consumers of client APIs are not impacted (such as Cloud Hypervisor).

blitz added 2 commits April 29, 2025 08:55
When a DmaMap message arrives with one file descriptor, the backend
still receives None in its dma_map call, because the for fds.len() the
code passes None instead of fds[0].

Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de>
If the backend wants to hang on to the file descriptor, it now has to
resort to unsafe code to dup() the File object.

Since we don't intend to hold on to the file object, we can just pass
it on by value and let the callee do with it as they please.

While we are here, also reject malformed input with too many file
descriptors.

Signed-off-by: Julian Stecklina <julian.stecklina@cyberus-technology.de>
@blitz
Copy link
Contributor Author

blitz commented Apr 29, 2025

Would you please update the commit message with prefix server instead of vfio_user? This would be very helpful to clarify the scope of changes, say consumers of client APIs are not impacted (such as Cloud Hypervisor).

Good point. Done!

Copy link
Collaborator

@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.

Thanks! Still lgtm.

@likebreath likebreath merged commit 4663a55 into rust-vmm:main Apr 30, 2025
6 checks passed
@blitz blitz deleted the fixes branch May 6, 2025 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants