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

Make IoctlFlags robust to unknown flags #62

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

kawasin73
Copy link
Collaborator

@kawasin73 kawasin73 commented Jan 10, 2024

This is the mitigation for #61.

Returning Error::UnrecognizedIoctls is inconvinient for developers because the only way to solve it is modifying this userfaultfd-rs crate implementation.

For example, Linux v6.6 adds UFFDIO_POISON bit to
uffdio_register.iotcls. With that change, Uffd::register() always fails even if the user don't care about the new UFFDIO_POISON feature.

We need to guard this change behind the ignore_unknown_ioctlflags feature because Error::UnrecognizedIoctls is already published and some user may depend on the error.

Using an unnamed flag for externally defined flag is originally recommended.
https://docs.rs/bitflags/latest/bitflags/#externally-defined-flags

@kawasin73 kawasin73 requested a review from pchickey January 10, 2024 04:22
@kawasin73 kawasin73 self-assigned this Jan 10, 2024
@kawasin73 kawasin73 requested a review from keiichiw January 10, 2024 04:24
@keiichiw
Copy link
Collaborator

It looks good to me, and I guess new users of this crate would want to enable it by default.
But, I can understand you want to keep the public behavior unchanged.

So, can you add a doc comment to the feature to recommend this feature enabled at least?
https://doc.rust-lang.org/cargo/reference/features.html#feature-documentation-and-discovery

@pchickey
Copy link
Collaborator

I would prefer to increment the version to 0.8.0, which semver indicates is a breaking change, rather than gate this behind a feature that might be difficult to discover, since I expect that users want this change and, as you said, the previous error is impossible to fix without changing the -sys source.

Returning Error::UnrecognizedIoctls is not helpful for developers
because the only way to solve it is modifying this userfaultfd-rs crate
implementation.

For example, Linux v6.6 adds UFFDIO_POISON bit to
uffdio_register.iotcls. With that change, Uffd::register() always fails
even if the user don't care about the new UFFDIO_POISON feature.

Using an unnamed flag for externally defined flag is originally
recommended.
https://docs.rs/bitflags/latest/bitflags/#externally-defined-flags

Since IoctlFlags accepts any unknown flags, Error::UnrecognizedIoctls is
needless and removed.
@kawasin73
Copy link
Collaborator Author

Thank you for reviewing. Updated the commit. It also removes Error::UnrecognizedIoctls.

Copy link
Collaborator

@keiichiw keiichiw left a comment

Choose a reason for hiding this comment

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

LGTM.
Although it'd be optional, isn't it a good timing to clean up and update CHANGELOG.md to mention this breaking change?

@kawasin73
Copy link
Collaborator Author

I will update the CHANGELOG in another PR.

@kawasin73 kawasin73 merged commit 7f1385f into bytecodealliance:main Jan 12, 2024
3 checks passed
@kawasin73
Copy link
Collaborator Author

Created #63 for changelog update.

@kawasin73
Copy link
Collaborator Author

Published 0.8.0

vadimberezniker added a commit to vadimberezniker/firecracker that referenced this pull request Apr 19, 2024
UFFD support is not compatible with newer kernels as there was a new ioctl option added that the older userfaultfd version does not recognize:
```
[PUT /snapshot/load][400] loadSnapshotBadRequest  &{FaultMessage:Load snapshot error: Failed to restore from snapshot: Failed to load guest memory: Error creating guest memory from uffd: Failed to register memory address range with the userfaultfd object: Unrecognized ioctl flags: 284}
```

This was reported in bytecodealliance/userfaultfd-rs#61 and fixed in bytecodealliance/userfaultfd-rs#62
vadimberezniker added a commit to vadimberezniker/firecracker that referenced this pull request Apr 19, 2024
UFFD support is not compatible with newer kernels as there was a new ioctl option added that the older userfaultfd version does not recognize:
```
[PUT /snapshot/load][400] loadSnapshotBadRequest  &{FaultMessage:Load snapshot error: Failed to restore from snapshot: Failed to load guest memory: Error creating guest memory from uffd: Failed to register memory address range with the userfaultfd object: Unrecognized ioctl flags: 284}
```

This was reported in bytecodealliance/userfaultfd-rs#61 and fixed in bytecodealliance/userfaultfd-rs#62

Signed-off-by: Vadim Berezniker <vadim@buildbuddy.io>
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.

3 participants