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

PageTable: Handling of pages without the present flag set #481

Closed
Wasabi375 opened this issue Apr 10, 2024 · 2 comments · Fixed by #484
Closed

PageTable: Handling of pages without the present flag set #481

Wasabi375 opened this issue Apr 10, 2024 · 2 comments · Fixed by #484

Comments

@Wasabi375
Copy link
Contributor

I'm playing around with adding guard pages to my kernel. The idea is that I add a guard page, before and after used pages to catch buffer overflows, etc.
I'd like to use Mapper::update_flags but the issue is, that I don't want to actually map the page to a PhysFrame so I can't set the present flag.

Is it possible to add something like update_flags_ignore_present as well as versions for the p4, p3, p2 entries? I'm willing to create a PR, just want to see if there is interest in this, or if I should just add a custom implementation in my kernel.

This would probably also require an extension to CleanUp. Something like CleanUp::clean_up_keep_with_flags.

@Wasabi375
Copy link
Contributor Author

Ok, turns out I was wrong here on a few counts when I originally wrote this. I only started looking into this more thoroughly in the last few days.

Mapper::update_flags works fine on pages that don't have the present flag set as long as the page entry is not 0. This means that either the frame has to be set or any flag. For some reason I was under the impression that update_flags checked for the present flag and not just that the entry is not 0.
I haven't spent much time looking at the cleanup function, but I think it also checks for not 0, so I don't think that needs to be updated. I just threw that in there originally as I thought that it would be related.

That said, there are still issues with pages without the present flag, mainly unmapping them.

Some pseudo example code:

let fake_frame = PhysFram::from_start_address(0);
let page = allocate_free_page();

// This works fine. There is no check that the present flag is set
unsafe { page_table.map_to(page, fake_frame, PageTableFlags::BIT_9, frame_allocator) }
            .map(|flusher| flusher.flush())
            .unwrap();

// This also works, the only check is that the page_entry for the page is not 0
unsafe { page_table.update_flags(page, PageTableFlags::BIT_10) }
            .map(|flusher| flusher.flush())
            .unwrap();

// This fails, with page not mapped error.
page_table.unmap(page).unwrap();

One option would be to just use update_flags to zero out the flags. Assuming that the fake frame uses the frame at address 0 this would be the same as unmapping the page. However I don't think we can assume that this is always the case, so there should be some option of actually clearing a page, even if the present flag is not set.

There are 2 solutions to this I can think of:

  1. update unmap to allow unmapping of pages without the present flag.
  2. add a new function unmap_ignore_present

I prefer option 2 as option 1 would require the return type to be updated so that we can communicate whether or not the page was present. Also as far as I understand it there is no reason to flush the tlb when the page wasn't present so no need to return a flusher.

Right now my suggestion for an api would be

enum UnmapResult {
     Present (frame, flusher),
     NotPresent(page_table_entry),
     Error(UnmapError)
}
fn unmap_ignore_present(&mut self, page: Page) -> UnmapResult;

From what I have seen so far, there is no consistent concept for when the page-table implementation checks for the present flag or when it is ok that the entry is just unused. If you want me to I don't mind spending some additional time, looking into all other cases where this is done. Hopefully the way it is currently checked is good enough. With that in mind, what are any policies that you have in regards to breaking changes in terms of functionality? Is this something that could even be changed?

@Wasabi375 Wasabi375 changed the title PageTable flags for unmaped pages PageTable: Handling of pages without the present flag set May 16, 2024
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 19, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 19, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 20, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 20, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 23, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
Wasabi375 added a commit to Wasabi375/x86_64 that referenced this issue May 24, 2024
Add a way to clear page table entries that don't have the present flag
set.

fixes: rust-osdev#481
@Freax13
Copy link
Contributor

Freax13 commented May 24, 2024

Closed by #484

@Freax13 Freax13 closed this as completed May 24, 2024
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 a pull request may close this issue.

2 participants