Make new tables take new flags

[mark-i-m]

When using RecursivePageTables::map_to(...), if new page tables are created, they do not use the page table flags of the new page. This means that e.g. if we map something as USER_ACCESSIBLE and we create a new page table, then counter-intuitively (and tediously), we have to up the page table hierarchy and figure out which entries should be made more permissive.

This PR means that instead when new page tables are created they get the same permissions as the new page, while old page table entries remain untouched.

This is a breaking change.

[phil-opp]

Thanks a lot for reporting this issue!

I'm not sure if inheriting the flags of the new page is the best solution, though. The problem I see is that adding a single mapping in for the same level 4 entry affects all following mappings to that entry. For example, you might break your code by reordering the map_to operations for a memory region if they use different flags. Thus, local reasoning about the effects of a mapping is not possible anymore.

I think a better solution would be to automatically add the required flags to parent page tables when creating a mapping. This would mean that when we map a page as user accessible, the map_to function would add the flag to the page tables of all levels, even if they already exist.

[phil-opp]

To implement this, we could OR the flags of existing tables with the flags of the page in the else block here:

x86_64/src/structures/paging/mapper/recursive_page_table.rs

Line 119 in 69e392a

created = false;

[phil-opp]

Also, we would need to update the code for MappedPageTable too, in order to keep the behavior consistent between different implementations of the Mapper trait. The required changes should be very similar since we also have a create_next_table function for MappedPageTable.

[phil-opp]

Given that this changes the behavior of an existing function, this is a breaking change. I updated the description of this pull request to ensure that I don't forget about this when releasing a new version.

[mark-i-m]

Thanks for the response! I agree with the issue you raised but I would point out that it is already true today: you can't reason locally because you need to know what had happened before (eg in my case, I need to know that I should enable all user-access bits up to the root somehow.

I think what we really want is some sort of Visitor and MutVisitor traits that allow traversing and/or modifying page table sets in different ways (eg pruning away unneeded tables, changing flags, adding new entries and tables, etc). That would solve a lot of problems, I think.

However, I don't think I have time to implement such a thing.

[phil-opp]

I think what we really want is some sort of Visitor and MutVisitor traits that allow traversing and/or modifying page table sets in different ways (eg pruning away unneeded tables, changing flags, adding new entries and tables, etc). That would solve a lot of problems, I think.

Interesting idea, but I don't have the time to experiment with something like this either.

I agree with the issue you raised but I would point out that it is already true today: you can't readon locally because you need to know what had happened before (eg in my case, I need to know that I should enable all user-access bits up to the root somehow.

Yes, but at least it does not matter which mapping is created first.

---

What do you think about my idea to automatically add the required flags mentioned above:

I think a better solution would be to automatically add the required flags to parent page tables when creating a mapping. This would mean that when we map a page as user accessible, the map_to function would add the flag to the page tables of all levels, even if they already exist.

?

With this approach we would automatically add the USER_ACCESSIBLE flag to all parent page tables when a mapping with that flag is created. This way, you would never need to set those flags on higher level tables manually.

[mark-i-m]

With this approach we would automatically add the USER_ACCESSIBLE flag to all parent page tables when a mapping with that flag is created. This way, you would never need to set those flags on higher level tables manually.

I guess it could be a reasonable default for now, but I'm generally against changing the flags at any level without the user being aware... If we do this, we should make sure it is in the documentation.

Also, would we do this for only USER_ACCESSIBLE or also for other flags (e.g. GLOBAL, PRESENT, etc)?

[mark-i-m]

Closing in favor of #114.

[phil-opp]

Also, would we do this for only USER_ACCESSIBLE or also for other flags (e.g. GLOBAL, PRESENT, etc)?

The idea was to do this for all flags that have the same behavior at a higher level table, such as PRESENT or WRITABLE.

I'm generally against changing the flags at any level without the user being aware... If we do this, we should make sure it is in the documentation.

I completely agree that this should be mentioned prominently in the documentation.