Received-SPF: pass (google.com: domain of linux-kernel+bounces-38877-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240124-alice-mm-v1-0-d1abcec83c44@google.com>
 <20240124-alice-mm-v1-2-d1abcec83c44@google.com> <070574cb-8d7f-4fe7-9826-cec6110168ff@app.fastmail.com>
 <CAH5fLgi8D18ufma0X49nWhXpnz47t-C=OAtM+wwnYu78hEnwhA@mail.gmail.com> <e69f9b43-39cd-469d-94db-140504df0833@app.fastmail.com>
In-Reply-To: <e69f9b43-39cd-469d-94db-140504df0833@app.fastmail.com>
From: Alice Ryhl <aliceryhl@google.com>
Date: Thu, 25 Jan 2024 17:15:13 +0100
Message-ID: <CAH5fLghn2=Y-M-M7-17PhNvKN3qVTsa6wh=Df770kHj1Oxh8hQ@mail.gmail.com>
Subject: Re: [PATCH 2/3] rust: add typed accessors for userspace pointers
To: Arnd Bergmann <arnd@arndb.de>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, 
	Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, 
	=?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Benno Lossin <benno.lossin@proton.me>, Andreas Hindborg <a.hindborg@samsung.com>, 
	Kees Cook <keescook@chromium.org>, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Andrew Morton <akpm@linux-foundation.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	=?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>, 
	Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>, 
	Joel Fernandes <joel@joelfernandes.org>, Carlos Llamas <cmllamas@google.com>, 
	Suren Baghdasaryan <surenb@google.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	rust-for-linux@vger.kernel.org, Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 25, 2024 at 5:00=E2=80=AFPM Arnd Bergmann <arnd@arndb.de> wrote=
:
>
> On Thu, Jan 25, 2024, at 13:37, Alice Ryhl wrote:
> > On Thu, Jan 25, 2024 at 1:27=E2=80=AFPM Arnd Bergmann <arnd@arndb.de> w=
rote:
> >> On Wed, Jan 24, 2024, at 12:20, Alice Ryhl wrote:
>
> >> > +EXPORT_SYMBOL_GPL(rust_helper_copy_to_user_unsafe_skip_check_object=
_size);
> >>
> >> These functions are almost identical to the ones in
> >> lib/usercopy.c for !defined(INLINE_COPY_TO_USER).
> >>
> >> That version has an extra memset() after a partial
> >> copy_from_user(), and you probably want to have the
> >> same thing here for consistency.
> >>
> >> I think ideally we should only have one out-of-line copy
> >> of these two functions and have that one shared between
> >> rust and architectures that want the C version out of line
> >> as well.
> >
> > I had a bit of trouble figuring out all of the copy_[to/from]_user
> > methods that are available. I was hoping that a better solution would
> > be available, and it sounds like one is. Is _copy_from_user always
> > available as an exported symbol? If it's always available and skips
> > the check, then I can just use that. I don't think the memset matters
> > for my case.
>
> At the moment, it appears that it's available on the few architectures
> that don't #define INLINE_COPY_FROM_USER: alpha, csky, powerpc,
> riscv and x86. On the other architectures it is always an inline
> function.
>
> > Otherwise, I can add a helper in rust/helpers.c that wraps
> > _copy_from_user only when INLINE_COPY_FROM_USER is defined, and call
> > the helper in those cases, and otherwise call the exported symbol
> > directly. (I need an exported symbol to call into C from Rust.)
> >
> > Would that make sense?
>
> I don't think we can have a perfect abstraction here, but rather
> than putting knowledge of INLINE_COPY_FROM_USER into the rust
> wrapper, I would suggest putting a bit of information about
> rust into lib/usercopy.c.
>
> I've tried to come up with an idea below, see if that works
> for you.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 3064314f4832..835aa175d0ee 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -138,13 +138,18 @@ __copy_to_user(void __user *to, const void *from, u=
nsigned long n)
>         return raw_copy_to_user(to, from, n);
>  }
>
> -#ifdef INLINE_COPY_FROM_USER
>  static inline __must_check unsigned long
> -_copy_from_user(void *to, const void __user *from, unsigned long n)
> +_inline_copy_from_user(void *to, const void __user *from, unsigned long =
n)
>  {
>         unsigned long res =3D n;
>         might_fault();
>         if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> +               /*
> +                * Ensure that bad access_ok() speculation will not
> +                * lead to nasty side effects *after* the copy is
> +                * finished:
> +                */
> +               barrier_nospec();
>                 instrument_copy_from_user_before(to, from, n);
>                 res =3D raw_copy_from_user(to, from, n);
>                 instrument_copy_from_user_after(to, from, n, res);
> @@ -153,14 +158,11 @@ _copy_from_user(void *to, const void __user *from, =
unsigned long n)
>                 memset(to + (n - res), 0, res);
>         return res;
>  }
> -#else
>  extern __must_check unsigned long
>  _copy_from_user(void *, const void __user *, unsigned long);
> -#endif
>
> -#ifdef INLINE_COPY_TO_USER
>  static inline __must_check unsigned long
> -_copy_to_user(void __user *to, const void *from, unsigned long n)
> +_inline_copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
>         might_fault();
>         if (should_fail_usercopy())
> @@ -171,25 +173,32 @@ _copy_to_user(void __user *to, const void *from, un=
signed long n)
>         }
>         return n;
>  }
> -#else
>  extern __must_check unsigned long
>  _copy_to_user(void __user *, const void *, unsigned long);
> -#endif
>
>  static __always_inline unsigned long __must_check
>  copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> -       if (check_copy_size(to, n, false))
> -               n =3D _copy_from_user(to, from, n);
> -       return n;
> +       if (!check_copy_size(to, n, false))
> +               return n;
> +#ifdef INLINE_COPY_FROM_USER
> +       return _inline_copy_from_user(to, from, n);
> +#else
> +       return _copy_from_user(to, from, n);
> +#endif
>  }
>
>  static __always_inline unsigned long __must_check
>  copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> -       if (check_copy_size(from, n, true))
> -               n =3D _copy_to_user(to, from, n);
> -       return n;
> +       if (!check_copy_size(from, n, true))
> +               return n;
> +
> +#ifdef INLINE_COPY_TO_USER
> +       return _inline_copy_to_user(to, from, n);
> +#else
> +       return _copy_to_user(to, from, n);
> +#endif
>  }
>
>  #ifndef copy_mc_to_kernel
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index d29fe29c6849..503a064d79e2 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -7,40 +7,18 @@
>
>  /* out-of-line parts */
>
> -#ifndef INLINE_COPY_FROM_USER
> +#if !defined(INLINE_COPY_FROM_USER) || defined(CONFIG_RUST)
>  unsigned long _copy_from_user(void *to, const void __user *from, unsigne=
d long n)
>  {
> -       unsigned long res =3D n;
> -       might_fault();
> -       if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> -               /*
> -                * Ensure that bad access_ok() speculation will not
> -                * lead to nasty side effects *after* the copy is
> -                * finished:
> -                */
> -               barrier_nospec();
> -               instrument_copy_from_user_before(to, from, n);
> -               res =3D raw_copy_from_user(to, from, n);
> -               instrument_copy_from_user_after(to, from, n, res);
> -       }
> -       if (unlikely(res))
> -               memset(to + (n - res), 0, res);
> -       return res;
> +       return _inline_copy_from_user(to, from, n);
>  }
>  EXPORT_SYMBOL(_copy_from_user);
>  #endif
>
> -#ifndef INLINE_COPY_TO_USER
> +#if !defined(INLINE_COPY_TO_USER) || defined(CONFIG_RUST)
>  unsigned long _copy_to_user(void __user *to, const void *from, unsigned =
long n)
>  {
> -       might_fault();
> -       if (should_fail_usercopy())
> -               return n;
> -       if (likely(access_ok(to, n))) {
> -               instrument_copy_to_user(to, from, n);
> -               n =3D raw_copy_to_user(to, from, n);
> -       }
> -       return n;
> +       return _inline_copy_to_user(to, from, n);
>  }
>  EXPORT_SYMBOL(_copy_to_user);
>  #endif

Sure, if that's okay with you, then I'm happy to do it that way in v2.

Thank you!
Alice