| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y/uiAfZnfbbMrQuD@boqun-archlinux>
Date: Sun, 26 Feb 2023 10:16:33 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Gary Guo <gary@...yguo.net>
Cc: Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Sven Van Asbroeck <thesven73@...il.com>,
Fox Chen <foxhlchen@...il.com>, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH 5/5] rust: error: Add from_kernel_result!() macro
On Sun, Feb 26, 2023 at 01:36:06PM +0000, Gary Guo wrote:
> On Sat, 25 Feb 2023 18:22:11 -0800
> Boqun Feng <boqun.feng@...il.com> wrote:
>
> > On Sat, Feb 25, 2023 at 10:23:40PM +0000, Gary Guo wrote:
> > > On Fri, 24 Feb 2023 15:56:05 -0800
> > > Boqun Feng <boqun.feng@...il.com> wrote:
> > >
> > > > On Fri, Feb 24, 2023 at 05:50:23PM +0900, Asahi Lina wrote:
> > > > > From: Wedson Almeida Filho <wedsonaf@...il.com>
> > > > >
> > > > > Add a helper macro to easily return C result codes from a Rust function
> > > > > that calls functions which return a Result<T>.
> > > > >
> > > > > Lina: Imported from rust-for-linux/rust, originally developed by Wedson
> > > > > as part of file_operations.rs. Added the allow() flags since there is no
> > > > > user in the kernel crate yet and fixed a typo in a comment.
> > > > >
> > > > > Co-developed-by: Fox Chen <foxhlchen@...il.com>
> > > > > Signed-off-by: Fox Chen <foxhlchen@...il.com>
> > > > > Co-developed-by: Miguel Ojeda <ojeda@...nel.org>
> > > > > Signed-off-by: Miguel Ojeda <ojeda@...nel.org>
> > > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > > > > Signed-off-by: Asahi Lina <lina@...hilina.net>
> > > > > ---
> > > > > rust/kernel/error.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 52 insertions(+)
> > > > >
> > > > > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > > > > index cf3d089477d2..8a9222595cd1 100644
> > > > > --- a/rust/kernel/error.rs
> > > > > +++ b/rust/kernel/error.rs
> > > > > @@ -226,3 +226,55 @@ pub(crate) fn from_kernel_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {
> > > > > }
> > > > > Ok(ptr)
> > > > > }
> > > > > +
> > > > > +// TODO: Remove `dead_code` marker once an in-kernel client is available.
> > > > > +#[allow(dead_code)]
> > > > > +pub(crate) fn from_kernel_result_helper<T>(r: Result<T>) -> T
> > > > > +where
> > > > > + T: From<i16>,
> > > > > +{
> > > > > + match r {
> > > > > + Ok(v) => v,
> > > > > + // NO-OVERFLOW: negative `errno`s are no smaller than `-bindings::MAX_ERRNO`,
> > > > > + // `-bindings::MAX_ERRNO` fits in an `i16` as per invariant above,
> > > > > + // therefore a negative `errno` always fits in an `i16` and will not overflow.
> > > > > + Err(e) => T::from(e.to_kernel_errno() as i16),
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +/// Transforms a [`crate::error::Result<T>`] to a kernel C integer result.
> > > > > +///
> > > > > +/// This is useful when calling Rust functions that return [`crate::error::Result<T>`]
> > > > > +/// from inside `extern "C"` functions that need to return an integer
> > > > > +/// error result.
> > > > > +///
> > > > > +/// `T` should be convertible from an `i16` via `From<i16>`.
> > > > > +///
> > > > > +/// # Examples
> > > > > +///
> > > > > +/// ```ignore
> > > > > +/// # use kernel::from_kernel_result;
> > > > > +/// # use kernel::bindings;
> > > > > +/// unsafe extern "C" fn probe_callback(
> > > > > +/// pdev: *mut bindings::platform_device,
> > > > > +/// ) -> core::ffi::c_int {
> > > > > +/// from_kernel_result! {
> > > > > +/// let ptr = devm_alloc(pdev)?;
> > > > > +/// bindings::platform_set_drvdata(pdev, ptr);
> > > > > +/// Ok(0)
> > > > > +/// }
> > > > > +/// }
> > > > > +/// ```
> > > > > +// TODO: Remove `unused_macros` marker once an in-kernel client is available.
> > > > > +#[allow(unused_macros)]
> > > > > +macro_rules! from_kernel_result {
> > > >
> > > > This actually doesn't need to be a macro, right? The following function
> > > > version:
> > > >
> > > > pub fn from_kernel_result<T, F>(f: F) -> T
> > > > where
> > > > T: From<i16>,
> > > > F: FnOnce() -> Result<T>;
> > > >
> > > > is not bad, the above case then can be written as:
> > > >
> > > > unsafe extern "C" fn probe_callback(
> > > > pdev: *mut bindings::platform_device,
> > > > ) -> core::ffi::c_int {
> > > > from_kernel_result(|| {
> > > > let ptr = devm_alloc(pdev)?;
> > > > bindings::platform_set_drvdata(pdev, ptr);
> > > > Ok(0)
> > > > })
> > > > }
> > > >
> > > > less magical, but the control flow is more clear.
> > > >
> > > > Thoughts?
> > >
> > > I don't think even the closure is necessary? Why not just
> > >
> > > pub fn from_kernel_result<T: From<i16>>(r: Result<T>) -> T
> > >
> > > and
> > >
> > > unsafe extern "C" fn probe_callback(
> > > pdev: *mut bindings::platform_device,
> > > ) -> core::ffi::c_int {
> > > from_kernel_result({
> > > let ptr = devm_alloc(pdev)?;
> >
> > Hmm.. looks like the `?` only "propagating them (the errors) to the
> > calling *function*":
> >
> > https://doc.rust-lang.org/reference/expressions/operator-expr.html#the-question-mark-operator
> >
> > , so this doesn't work as we expect:
> >
> > https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=64c9039e1da2c436f9f4f5ea46e97e90
>
> Oh, you're absolutely right, I guess I shouldn't be doing code review
> in the middle of the night...
>
;-)
> However, if we have the try blocks then we should be able to just
> rewrite it as
>
> from_kernel_result(try {
> ...
> })
>
Thank you and Miguel for the references on try blocks, I vaguely
remember I heard of them, but never take a close look.
> I guess in this sense it might worth keeping this as a macro so we can
> tweak the implementation from closure to try blocks without having to
> edit all use sites?
>
My preference to function instead of macro here is because I want to
avoid the extra level of abstraction and make things explict, so that
users and reviewers can understand the API behavior solely based on
Rust's types, functions and closures: they are simpler than macros, at
least to me ;-)
Speak of future changes in the implementation:
First, I think the macro version here is just a poor-man's try block, in
other words, I'd expect explicit use of try blocks intead of
`from_kernel_result` when the feature is ready. If that's the case, we
need to change the use sites anyway.
Second, I think the semantics is a little different between try block
implementation and closure implemention? Consider the following case:
// the outer function return a Result<i32>
let a = from_kernel_result!({
...
return Some(0); // x is i32
..
});
return Some(a + 1);
Do both implementation share the same behavior?
No one can predict the future, but being simple at the beginning sounds
a good strategy to me.
Regards,
Boqun
> Best,
> Gary
Powered by blists - more mailing lists