Skip to content

Add UserSlicePtrWriter::clear. #222

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Add UserSlicePtrWriter::clear. #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rust/helpers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ unsigned long rust_helper_copy_to_user(void __user *to, const void *from, unsign
return copy_to_user(to, from, n);
}

unsigned long rust_helper_clear_user(void __user *to, unsigned long n)
{
return clear_user(to, n);
}

void rust_helper_spin_lock_init(spinlock_t *lock, const char *name,
struct lock_class_key *key)
{
Expand Down
30 changes: 30 additions & 0 deletions rust/kernel/user_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ extern "C" {
from: *const c_types::c_void,
n: c_types::c_ulong,
) -> c_types::c_ulong;

fn rust_helper_clear_user(to: *mut c_types::c_void, n: c_types::c_ulong) -> c_types::c_ulong;
}

/// Specifies that a type is safely readable from byte slices.
Expand Down Expand Up @@ -242,6 +244,34 @@ impl UserSlicePtrWriter {
self.len() == 0
}

/// Writes zeroes to the user slice.
///
/// Differently from the other write functions, `clear` will zero as much as it can and update
/// the writer internal state to reflect this. It will, however, return an error if it cannot
/// clear `len` bytes.
///
/// For example, if a caller requests that 100 bytes be cleared but a segfault happens after
/// 20 bytes, then EFAULT is returned and the writer is advanced by 20 bytes.
pub fn clear(&mut self, mut len: usize) -> KernelResult {
let mut ret = Ok(());
if len > self.1 {
ret = Err(Error::EFAULT);
len = self.1;
}
Comment on lines +257 to +260
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the documentation should explain the behavior better, i.e. that it will clear no matter what, even if the length is bigger and that the fields will be updated in all cases.

In any case, isn't a bit confusing to do that given write_raw() will return without performing any action if len is out of bounds? (Which, by the way, should likely be a EINVAL rather than a EFAULT, no?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the documentation should explain the behavior better, i.e. that it will clear no matter what, even if the length is bigger and that the fields will be updated in all cases.

Sure. I updated the documentation.

In any case, isn't a bit confusing to do that given write_raw() will return without performing any action if len is out of bounds?

Since write_raw doesn't update its internal state to reflect partial writes, it not so problematic at the moment.

For clear we need to report how much was actually cleared because we want to mimic the behaviour of the C implementation.

I agree that it isn't ideal, but I think the solution here is really to update write_raw to reflect what was written in its internal state. But since this PR is about clear, I think this should be updated in another PR.

(Which, by the way, should likely be a EINVAL rather than a EFAULT, no?)

Perhaps. But I think EINVAL, if bubbled up to userspace, is too generic. EFAULT is a better indication that the problem is in some address. Also, this is the existing behaviour, I'm just trying to add clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It is clear now.


// SAFETY: The buffer will be validated by `clear_user`. We ensure that `len` is within
// bounds in the check above.
let left = unsafe { rust_helper_clear_user(self.0, len as _) } as usize;
if left != 0 {
ret = Err(Error::EFAULT);
len -= left;
}

self.0 = self.0.wrapping_add(len);
self.1 -= len;
ret
}

/// Writes a byte slice to the user slice.
///
/// Returns `EFAULT` if the byte slice is bigger than the remaining size
Expand Down