rust: kernel: add missing safety comments#450
rust: kernel: add missing safety comments#450Ayush1325 wants to merge 1 commit intoRust-for-Linux:rustfrom
Conversation
LeSeulArtichaut
left a comment
LeSeulArtichaut
left a comment
There was a problem hiding this comment.
The goal of SAFETY comments is to explain why the unsafe operations performed in an unsafe block are safe. I'm not familiar with kernel APIs but I tried to write two examples, I hope they are useful.
| // Safety: Returns a raw Pointer. Will return NULL in case of error. | ||
| // `krealloc()` is used instead of `kmalloc()` because the latter is | ||
| // an inline function and cannot be bound to as a result. |
There was a problem hiding this comment.
I don't think malloc has particular safety requirements (docs), so this can simply be
| // Safety: Returns a raw Pointer. Will return NULL in case of error. | |
| // `krealloc()` is used instead of `kmalloc()` because the latter is | |
| // an inline function and cannot be bound to as a result. | |
| // `krealloc()` is used instead of `kmalloc()` because the latter is | |
| // an inline function and cannot be bound to as a result. | |
| // SAFETY: FFI call. |
There was a problem hiding this comment.
I also mentioned the C function name, `krealloc' in the new commit only FFI call might be confusing in places where the rust function is named differently.
| } | ||
|
|
||
| unsafe fn dealloc(&self, ptr: *mut u8, _layout: Layout) { | ||
| // Safety: Freeing memory not allocated with kmalloc() can cause undefined behavior. |
There was a problem hiding this comment.
As you noted kfree should only be called on memory allocated with kmalloc. However, as stated in the GlobalAlloc::dealloc docs the caller has to guarantee that:
ptrmust denote a block of memory currently allocated via this allocator,layoutmust be the same layout that was used to allocate that block of memory.
So the SAFETY comment could look like:
| // Safety: Freeing memory not allocated with kmalloc() can cause undefined behavior. | |
| // SAFETY: the caller must guarantee that `ptr` and `layout` denote memory | |
| // allocated by this allocator, so allocated with `kmalloc`. |
There was a problem hiding this comment.
I have made the edits as suggested.
| unsafe fn alloc(&self, layout: Layout) -> *mut u8 { | ||
| // `krealloc()` is used instead of `kmalloc()` because the latter is | ||
| // an inline function and cannot be bound to as a result. | ||
| // SAFETY: FFI call to krealloc. |
There was a problem hiding this comment.
Elsewhere we do not mention the function being called:
| // SAFETY: FFI call to krealloc. | |
| // SAFETY: FFI call. |
There was a problem hiding this comment.
Ok. Removed mention of the function being called.
|
Please rebase and reword the commit message (take a look at |
Ayush1325
force-pushed
the
allocator-safety
branch
from
bd47ed7 to
0c3aab6
Compare
I did the rebase and modeled my latest commit message after a few other safety commits I could find. Do I need to add the signed-off-by line in my previous commits too? |
|
The kernel does not take patches (commits) that fix patches from the same series (PR). In other words, this should be a single commit. |
Ayush1325
force-pushed
the
allocator-safety
branch
from
0c3aab6 to
61898ef
Compare
- Added safety comments for rust/kernel/allocator.rs Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
Ayush1325
force-pushed
the
allocator-safety
branch
from
61898ef to
557dd52
Compare
I squashed the commits to a single commit. I am assuming that's how it is supposed to be done? I haven't combined commits before. |
3 participants
related to #351
Signed-off-by: Ayush Singh ayushsingh1325@gmail.com