Skip to content

Fix implicit_provenance_casts warnings on Xous#159174

Open
bjorn3 wants to merge 1 commit into
rust-lang:mainfrom
bjorn3:fix_xous_warnings
Open

Fix implicit_provenance_casts warnings on Xous#159174
bjorn3 wants to merge 1 commit into
rust-lang:mainfrom
bjorn3:fix_xous_warnings

Conversation

@bjorn3

@bjorn3 bjorn3 commented Jul 12, 2026

Copy link
Copy Markdown
Member

No description provided.

@bjorn3 bjorn3 added the O-xous OS: A microkernel OS for privacy in computing label Jul 12, 2026
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 12, 2026
@rustbot

rustbot commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ChrisDenton, libs
  • @ChrisDenton, libs expanded to 13 candidates
  • Random selection from Darksonn, JohnTitor, Mark-Simulacrum, clarfonthey, jhpratt

@bjorn3

bjorn3 commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

cc @xobs

@xobs

xobs commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Thank you for addressing these.

/// pointer uniqueness guarantee.
pub(crate) unsafe fn map_memory<T>(
phys: Option<core::ptr::NonNull<T>>,
phys: Option<core::num::NonZeroUsize>,

@bjorn3 bjorn3 Jul 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I changed this away from a pointer type as pointer provenance covers virtual memory, not physical memory.

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That seems worth a comment (and same for the expose_provenance calls, basically embedding in the code what you explained in the PR).

@RalfJung

Copy link
Copy Markdown
Member

The point of these warnings is to actually audit whether exposing is necessary or whether strict provenance could be used instead. Did you actually check this or just replace is everywhere?

@bjorn3

bjorn3 commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

For lend_mut_impl and lend_impl the kernel will read through the passed in pointer. It might be possible to directly pass the pointer to the as block instead. For map_memory the kernel will replace the memory allocation at the given address if there was one. And unmap_memory will deallocate the allocation. And finally create_thread will invoke the function pointer and use the passed in stack for the new thread.

In Thread::new, guard_page_pre and data are both read/deallocated by the thread_start. And tls_table_slow also need the pointer to be exposed for TLS accesses to be allowed.

@xobs

xobs commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

The documentation isn't very clear. It seems that for strict provenance, you use .with_addr(), but to do that you already need to have a pointer.

The problem is that these addresses come from the kernel via a syscall, so they're just a bare usize when the functions return. What's the correct way to go from a syscall return value to a pointer without using .exposed_provenance()?

@RalfJung

RalfJung commented Jul 12, 2026

Copy link
Copy Markdown
Member

@xobs there is no exposed_provenance. There is expose_provenance which goes from a pointer to an integer, which is the opposite direction of what you are talking about. So I can't quite follow your question.

It seems that @bjorn3 is saying that the kernel APIs here are all designed such that they take integers when they actually should take pointers (i.e. this is about inputs to syscalls, not return values)? Yeah if APIs don't use pointer types properly then expose_provenance is what one has to do. Might be worth a few comments.

@clarfonthey

Copy link
Copy Markdown
Contributor

There's with_exposed_provenance, but based upon what the docs say, it's not possible to start from exposed provenance, only return to it. But maybe this is the right choice for this kind of API.

I know that the provenance of external addresses is something that has been discussed a lot and not really resolved. From a libs perspective, the code looks okay (r=me), but I'm going to defer to opsem for review since it feels more important that this is semantically correct rather than just passing a lint.

r? RalfJung since you've already taken a look, but feel free to reroll if you'd rather not review.

@rustbot rustbot assigned RalfJung and unassigned clarfonthey Jul 12, 2026
@rustbot

rustbot commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

@RalfJung

Copy link
Copy Markdown
Member

There's with_exposed_provenance, but based upon what the docs say, it's not possible to start from exposed provenance, only return to it. But maybe this is the right choice for this kind of API.

As long as they have been exposed "on the other side" of the FFI (which we can generally assume for cases like this), that's fine.

But this PR doesn't even call with_exposed_provenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-xous OS: A microkernel OS for privacy in computing S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants