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

exception/interrupt handlers shouldn't access this_cpu() #478

Open
Freax13 opened this issue Oct 2, 2024 · 4 comments
Open

exception/interrupt handlers shouldn't access this_cpu() #478

Freax13 opened this issue Oct 2, 2024 · 4 comments

Comments

@Freax13
Copy link
Contributor

Freax13 commented Oct 2, 2024

Exception/interrupt handlers (or more commonly signal handlers in non-kernel code) are considered separate threads within the abstract machine. One consequence of this is that they shouldn't access this_cpu() because this_cpu() is not Sync. There's currently nothing to prevent this from happening.

Code accessing !Sync data can usually assume that its accesses are never interrupted, but that's not the case if an exception handler preempts the code and accesses the data as well. The soundness of the Cell and RefCell types hinges on being able to do uninterrupted accesses. Consider the following example: Some code wants to write an enum with data into a Cell in its PerCpu structure. Rust enums have a tag that decides which variant is active. If the code trying to write the enum is interrupted halfway through (e.g. by the hypervisor injecting #HV), it may have only written the tag, but not all the other data contained in the enum. Unlike with lock structures, there's nothing that prevents the exception handler from accessing the data as well. When the exception handler reads the Cell it may see the new enum tag but with some of the old enum data. If the tag doesn't match the data, that's obviously very bad.

@p4zuu
Copy link
Collaborator

p4zuu commented Oct 11, 2024

It seems to me that we can hardly remove all Cell, RefCell and OnceCell from PerCpu to have it Sync (I'm not even sure if doing this is sufficient to have this_cpu() Sync).
It also seems to me that we can hardly handle #PF without accessing the page tables, so I guess here we either need to move the page tables into a Sync version (eg. moving the page tables without using RefCell into PerCpuShared, right?), or to add a lock around the page tables ref, but it seems this is a bad idea

@Freax13
Copy link
Contributor Author

Freax13 commented Oct 11, 2024

Sorry, I think the title is a bit misleading: What I was trying to say was the "with the current PerCpu implementation, exception/interrupt handlers must not access this_cpu()". I don't know of a good solution to this, but it might involve making PerCpu safe to access from multiple threads so that exception/interrupt handles may access this_cpu().
I don't know of a good way to solve this, we might have to get rid of the *Cell types, we might have to introduce locks, maybe we should split up PerCpu into several levels of access from different contexts, I don't know.

@00xc
Copy link
Member

00xc commented Oct 16, 2024

For RefCells we could perhaps introduce a wrapper type that disables interrupts while the data is borrowed.

There would still be the possibility of restoring interrupts manually while the borrow is alive but it would at least be better than what we have now.

@Freax13
Copy link
Contributor Author

Freax13 commented Oct 17, 2024

For RefCells we could perhaps introduce a wrapper type that disables interrupts while the data is borrowed.

This won't work for exception handlers though. The host can pretty much always trigger a #VC (e.g. by unmapping memory) even with restricted injection.

There would still be the possibility of restoring interrupts manually while the borrow is alive but it would at least be better than what we have now.

We already support nested disabling/enabling of interrupts.

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