Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature #607

nicholasbishop · 2022-12-11T19:50:57Z

Our protocols are all marked !Send and !Sync using the unstable negative_impls feature. There's another way to mark things !Send and !Sync though: add a field that itself is !Send and !Sync, for example a raw pointer. We don't want to actually change the struct layout of course, but we can use the PhantomData type for this.

The new unsafe_protocol macro implements this idea; it's an attribute macro that combines the functionality of the unsafe_guid attribute macro and the Protocol derive macro, as well as adding a hidden core::marker::PhantomData<*const u8> field for !Send and !Sync.

The real purpose of this change is to remove use of the unstable negative_impls feature towards our goal of building on the stable channel. But it also ends up with a slightly nicer API as well.

Short log:

uefi-macros: Refactor GUID tests
uefi: Remove non-protocol uses of unsafe_guid
uefi-macros: Add unsafe_protocol macro
uefi: Use unsafe_protocol! for all protocol impls
uefi-macros: Drop the unsafe_guid and Protocol macros
Drop use of unstable negative_impls feature
Update changelog

Tracking issue for unstable features: #452

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

phip1611 · 2022-12-20T15:13:47Z

uefi-test-runner/src/boot/misc.rs

@@ -80,8 +80,7 @@ fn test_watchdog(bt: &BootServices) {
 }

 /// Dummy protocol for tests
-#[unsafe_guid("1a972918-3f69-4b5d-8cb4-cece2309c7f5")]
-#[derive(Protocol)]
+#[unsafe_protocol("1a972918-3f69-4b5d-8cb4-cece2309c7f5")]


general question: why does it need to be called unsafe, tho? Why can't the attribute be called uefi_protocol("...")? I do not see a benefit of the prefix unsafe here and it only adds confusion.

What do you think?

@nicholasbishop

If it's called unsafe_protocol my question would be what makes it unsafe / how do I use it safely?

It's prefixed with unsafe_ because passing in the wrong GUID can easily lead to unsoundness.

Here's a quick example of how bad things can happen:

The definition of EFI_DISK_IO_PROTOCOL from the spec is:

#define EFI_DISK_IO_PROTOCOL_GUID \ {0xCE345171,0xBA0B,0x11d2,\ {0x8e,0x4F,0x00,0xa0,0xc9,0x69,0x72,0x3b}} typedef struct _EFI_DISK_IO_PROTOCOL { UINT64 Revision; EFI_DISK_READ ReadDisk; EFI_DISK_WRITE WriteDisk; } EFI_DISK_IO_PROTOCOL;

Now consider this Rust code:

#[repr(C)] // Bad! #[unsafe_protocol("ce345171-ba0b-11d2-8e4f-00a0c969723b")] pub struct MyCoolType { do_it: extern "efiapi" fn(), } let handle = bt.get_handle_for_protocol::<MyCoolType>().unwrap(); let c = bt.open_protocol_exclusive::<MyCoolType>(handle).unwrap(); (c.do_it)();

I've defined a protocol called MyCoolType with the same GUID as EFI_DISK_IO_PROTOCOL. When I call get_handle_for_protocol and open_protocol_exclusive, UEFI will think it's looking for a disk handle and opening the disk IO protocol. So those calls will succeed, and although c will be a ScopedProtocol<MyCoolType>, it will contain completely invalid data.

The docstring has a # Safety section: https://github.com/rust-osdev/uefi-rs/pull/607/files#diff-ff7f936d7319fc633630baca626659855f359b7185c91237bc9d68bd798256aeR33. There's not much too it; you have to make sure the type and GUID are correct and match up. Definitely open to improving or expanding that paragraph if you have suggestions though.

Alright, thanks for the clarification.

phip1611 · 2022-12-20T15:15:26Z

uefi/src/proto/media/file/info.rs

@@ -142,7 +142,6 @@ pub enum FileInfoCreationError {
 ///   attribute. Other changes must be carried out in a separate transaction.
 #[derive(Debug, Eq, PartialEq)]
 #[repr(C)]
-#[unsafe_guid("09576e92-6d3f-11d2-8e39-00a0c969723b")]


good catch! thanks for fixing this

uefi/src/proto/console/serial.rs

uefi-test-runner/src/main.rs

phip1611 · 2022-12-20T15:30:44Z

Awesome work @nicholasbishop, thanks!

Switch the tests from using `unsafe_guid!` to `guid!`. Both use the same internal code, so we're testing more or less the same thing, but `guid!` is a little more direct. Also, as we previously did with the entry macro, split the GUID tests into multiple files to make it easier to visually inspect the error output. The "good" test case has been removed since that's already covered in plenty of places elsewhere.

The `unsafe_guid` macro is almost always used for creating protocols. There are three other uses though, for identifying file info types. Replace these with explicit impls of the `Identify` trait. Now that we have a `guid!` macro, this doesn't reduce the clarity of the code at all.

This attribute macro combines the functionality of the `unsafe_guid` attribute macro and the `Protocol` derive macro. It impls the `Protocol` and `Identify` traits, and it also marks the type as `!Send` and `!Sync`. This provides a slightly nicer API for protocols (`unsafe_protocol` is a clearer name than `unsafe_guid`, and only one macro is needed instead of two), but more importantly it does the `!Send` and `!Sync` part via a hidden `PhantomData` field rather than relying on the unstable `negative_impls` feature.

Switch all uses of `unsafe_guid!` + `derive(Protocol)` to using `unsafe_protocol!`. Also update the docstrings for the `Identify` and `Protocol` traits.

These are no longer used anywhere.

This is no longer used anywhere.

phip1611 requested changes Dec 20, 2022

View reviewed changes

phip1611 reviewed Dec 20, 2022

View reviewed changes

uefi/src/proto/console/serial.rs Show resolved Hide resolved

phip1611 reviewed Dec 20, 2022

View reviewed changes

uefi-test-runner/src/main.rs Show resolved Hide resolved

nicholasbishop added 7 commits December 26, 2022 17:29

uefi: Use unsafe_protocol! for all protocol impls

be7a20c

Switch all uses of `unsafe_guid!` + `derive(Protocol)` to using `unsafe_protocol!`. Also update the docstrings for the `Identify` and `Protocol` traits.

uefi-macros: Drop the unsafe_guid and Protocol macros

c2de574

These are no longer used anywhere.

Drop use of unstable negative_impls feature

d2cf762

This is no longer used anywhere.

Update changelog

19e29dc

nicholasbishop force-pushed the bishop-protocol-macro-3 branch from d51b573 to 19e29dc Compare December 26, 2022 23:10

phip1611 approved these changes Dec 27, 2022

View reviewed changes

phip1611 merged commit 93699b8 into rust-osdev:main Dec 27, 2022

nicholasbishop deleted the bishop-protocol-macro-3 branch December 28, 2022 04:58

nicholasbishop mentioned this pull request Dec 30, 2022

Tracking issue for use of unstable features #452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature #607

Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature #607

nicholasbishop commented Dec 11, 2022

phip1611 Dec 20, 2022 •

edited

Loading

blitz Dec 20, 2022

nicholasbishop Dec 20, 2022

phip1611 Dec 27, 2022

blitz Jan 2, 2023

phip1611 Dec 20, 2022

phip1611 commented Dec 20, 2022 •

edited

Loading

Add unsafe_protocol macro and drop use of the unstable negative_impls feature #607

Add unsafe_protocol macro and drop use of the unstable negative_impls feature #607

Conversation

nicholasbishop commented Dec 11, 2022

Checklist

phip1611 Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

blitz Dec 20, 2022

Choose a reason for hiding this comment

nicholasbishop Dec 20, 2022

Choose a reason for hiding this comment

phip1611 Dec 27, 2022

Choose a reason for hiding this comment

blitz Jan 2, 2023

Choose a reason for hiding this comment

phip1611 Dec 20, 2022

Choose a reason for hiding this comment

phip1611 commented Dec 20, 2022 • edited Loading

Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature #607

Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature #607

phip1611 Dec 20, 2022 •

edited

Loading

phip1611 commented Dec 20, 2022 •

edited

Loading