rust: kernel: add missing safety comments #382
Conversation
This comment has been minimized.
This comment has been minimized.
| /// # Safety | ||
| /// | ||
| /// The returned value of `A::convert` must be a valid non-null pointer and | ||
| /// `T:open` must return a valid non-null pointer on an `Ok` result. |
There was a problem hiding this comment.
Was going to add comments and docs to all the helpers, but wasn't sure if this was something needed/wanted. If this is helpful, I can update with the rest documented.
There was a problem hiding this comment.
Hi @dlrobertson this should ideally be reviewed by @ojeda, our resident // SAFETY expert. That said, I'll try to review using the limited knowledge I have.
First, you've chosen a section of the code (SafeOpenAdapter) that still appears to be a work-in-progress. Both implementations of that trait contain TODO annotations wrt safety comments. And as far as I can see, the missing annotations here are non-trivial, and require deep understanding of the relevant code around it.
Second, the aim of the safety comments is to document exactly why we guarantee that the code inside the unsafe {} block will not result in Undefined Behaviour. Which means we usually document the inputs to the unsafe block only, not the outputs.
I have placed a comment on two of your annotations with the questions which the // SAFETY annotation will need to answer.
| let arg = unsafe { A::convert(inode, file) }; | ||
| // SAFETY: `arg` was returned by `A::convert` and must be a valid | ||
| // non-null pointer | ||
| let ptr = T::open(unsafe { &*arg })?.into_pointer(); |
There was a problem hiding this comment.
Only the unsafe { &*arg } block needs the // SAFETY comment. To arrive at a correct comment here, ask yourself how you can prove that it is always safe to convert the raw pointer arg to a shared reference, which lives long enough to satisfy the requirements of FileOpener::open().
(note that I do not know the answer to that question myself, this is intended to show what questions to ask and answer)
There was a problem hiding this comment.
Got it. I wrote it this way because I'm assuming the pointer returned by convert is going to point to something in the inode or file which will definitely live beyond open callback and will live beyond T::open. I'll add that to the comment.
| let ptr = T::open(unsafe { &*arg })?.into_pointer(); | ||
| // SAFETY: `file` was returned by `T::open` and must be a valid | ||
| // non-null pointer. | ||
| unsafe { (*file).private_data = ptr as *mut c_types::c_void }; |
There was a problem hiding this comment.
To arrive at a correct comment here, ask yourself how you can prove that it is always safe to dereference the raw pointer called file. Is that pointer guaranteed to be valid and non-null? If so, why?
(note that I do not know the answer to that question myself, this is intended to show what questions to ask and answer)
There was a problem hiding this comment.
Updated this one a bit. I whiffed on the first draft.
- Add safety comment to usage of get_unused_fd_flags - Add documentation and safety comments to internal open_callback helper function - Add additional safety documentation to FileOpenAdapter::convert Signed-off-by: Dan Robertson <[email protected]>
dlrobertson
force-pushed
the
safety-comment-file
branch
from
55530e7 to
53252c0
Compare
This comment has been minimized.
This comment has been minimized.
dlrobertson
force-pushed
the
safety-comment-file
branch
from
53252c0 to
2900f16
Compare
|
Review of
|
helper function
Related To: #351
Signed-off-by: Dan Robertson [email protected]