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

update_flags does not add flags to parent page tables #534

Open
ChocolateLoverRaj opened this issue Feb 5, 2025 · 7 comments
Open

update_flags does not add flags to parent page tables #534

ChocolateLoverRaj opened this issue Feb 5, 2025 · 7 comments

Comments

@ChocolateLoverRaj
Copy link

Steps to reproduce:

  • Create a mapping without USER_ACCESSIBLE
  • Update the flags to include USER_ACCESSIBLE

Expected:
The page can be accessed by user mode.

Actual:
The page can't be accessed by user mode because even though the L1 entry has USER_ACCESSIBLE, the L2, L3, L4 entries may not be user accessible.

@Freax13
Copy link
Member

Freax13 commented Feb 11, 2025

Note that this isn't unique to USER_ACCESSIBLE, the same should be true for other bits e.g. WRITE and NO_EXECUTE.

I think we should be careful changing about update_flags's behaviors around this, it's possible that some users depend on the current behavior, so I'd rather not change it. That said I don't think we currently offer a good solution to this, so I'm open to suggestions :)

@ChocolateLoverRaj
Copy link
Author

One idea is to set all the flags for all L4-L2 entries and only use L1 flags to control permissions.

@Freax13
Copy link
Member

Freax13 commented Feb 12, 2025

I don't think we should change the behavior of existing methods in such a drastic way. There might be people that depend on the current behavior.

@ChocolateLoverRaj
Copy link
Author

What about if update_flags was left as is and there was a update_flags_2 function with the new behavior?

@Freax13
Copy link
Member

Freax13 commented Feb 12, 2025

I'm open to that idea (although I don't like that method name).

Another idea might be to improve TranslateResult to also contain the page table flags for the upper levels. With this information, a user can call set_flags_p{2,3,4}_entry as required. Changing TranslateResult would be a breaking change though...

@phil-opp
Copy link
Member

In the Mapper::map_to function, we automatically set the PRESENT, WRITABLE and USER_ACCESSIBLE flags for the parent table if required. We also have Mapper::map_to_with_table_flags with an explicit parent_table_flags argument. It probably makes sense to provide this functionality also for flag updates.

I'm also against changing the behavior of update_flags directly since this would be a very subtle breaking change, but a new method with a different name sounds good to me. Maybe something like update_flags_recursive or update_flags_with_parents?

For completeness, we could also add a method like update_table_flags(page, flags, parent_table_flags) that allows you to specify the flags for parent table entries manually.


Including the parent table flags in TranslateResult sounds like a good idea too! So maybe we just do both?

@ChocolateLoverRaj
Copy link
Author

I like the idea of a new function called update_flags_with_parents. I don't think I will be using the parent table flags so that change doesn't make a difference for my use.

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

No branches or pull requests

3 participants