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

Reasoning safety of self.get_ctx() in KernelRef::kernel_trap #517

Open
Medowhill opened this issue Apr 30, 2021 · 2 comments
Open

Reasoning safety of self.get_ctx() in KernelRef::kernel_trap #517

Medowhill opened this issue Apr 30, 2021 · 2 comments
Assignees
Labels

Comments

@Medowhill
Copy link
Collaborator

rv6/kernel-rs/src/trap.rs

Lines 200 to 201 in b946a94

// TODO: safety?
if let Some(ctx) = unsafe { self.get_ctx() } {

@jeehoonkang
Copy link
Member

음... 전 사실 이게 왜 safe한지 모르겠습니다. ctx 잡고있는데 kernel trap 밟아서 다시 ctx 생성될 수 있는거같아서요. 재민님, 혹시 왜 safe한지 아시나요?

@Medowhill
Copy link
Collaborator Author

KernelCtx를 두 개 만들면 안 되는 이유는, 그 자체로 위험해서가 아니라, KernelCtx가 두 개이면 하나의 ProcData를 가리키는 두 개의 &mut을 만들 수 있기 때문입니다. kernel_trap에서는 KernelCtx를 만들며, cpu_yield를 호출하는데, cpu_yield 안에서 ProcData 내부의 Context에 접근하기 위해 &mut ProcData를 얻습니다. 그래서 이론상으로는 하나의 Context에 대한 두 &mut Context가 만들어질 수 있습니다. 그러나 아마도 실제로는 Context가 필요한 경우가 context switch를 할 때뿐이기 때문에, kernel trap이 발생하기 전에 커널 모드에서 user trap을 처리하고 있을 때는 Context에 접근하고 있지 않았을 것 같습니다. 그래서 kernel_trap에서 &mut Context에 접근하더라도 실질적으로는 unique access가 아닌가 합니다. Safety를 따지기기 상당히 복잡한 것 같습니다..

그리고 바로 이어지는 코드에도 unsafe block이 있는데, safety가 주석으로 써 있기는 하지만 설명이 잘 이해되지는 않습니다.

rv6/kernel-rs/src/trap.rs

Lines 200 to 207 in 9f0a531

// TODO(https://github.com/kaist-cp/rv6/issues/517): safety?
if let Some(ctx) = unsafe { self.get_ctx() } {
// SAFETY:
// Reading state without lock is safe because `proc_yield` and `sched`
// is called after we check if current process is `RUNNING`.
if unsafe { (*ctx.proc().info.get_mut_raw()).state } == Procstate::RUNNING {
ctx.yield_cpu();
}

아래의 논의에서 나온 내용을 주석으로 쓴 것 같은데 왜 안전한 것인지 잘 모르겠습니다.

#401 (comment)

일단 CurrentProc의 invariant를 정의할 때 state가 Running이라는 내용을 넣었기 때문에, 해당 invariant가 틀린 것이 아니라면 굳이 state가 Running인지 검사할 필요가 없습니다. 지금 해당 검사를 지우고 무조건 cpu_yield를 호출하게 수정해 보았는데, 테스트는 무사히 통과하는 것 같습니다. 해당 검사는 xv6에부터 있던 것인데, xv6가 불필요한 검사를 넣은 것일 수도 있을 것 같습니다.

@Medowhill Medowhill added D-hard and removed D-mid labels May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants