On Tue, 13 Feb 2024 at 15:05, David Hildenbrand [off-list ref] wrote:

On 13.02.24 15:02, Ryan Roberts wrote:

quoted

On 13/02/2024 13:45, David Hildenbrand wrote:

quoted

On 13.02.24 14:33, Ard Biesheuvel wrote:

quoted

On Tue, 13 Feb 2024 at 14:21, Ryan Roberts [off-list ref] wrote:

quoted

On 13/02/2024 13:13, David Hildenbrand wrote:

...

quoted

quoted

quoted

quoted

quoted

Just a thought, you could have a is_efi_mm() function that abstracts all that.

diff --git a/include/linux/efi.h b/include/linux/efi.h
index c74f47711f0b..152f5fa66a2a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h

@@ -692,6 +692,15 @@ extern struct efi {

   extern struct mm_struct efi_mm;

+static inline void is_efi_mm(struct mm_struct *mm)
+{
+#ifdef CONFIG_EFI
+       return mm == &efi_mm;
+#else
+       return false;
+#endif
+}
+
   static inline int
   efi_guidcmp (efi_guid_t left, efi_guid_t right)
   {

That would definitely work, but in that case, I might as well just check for it
in mm_is_user() (and personally I would change the name to mm_is_efi()):


static inline bool mm_is_user(struct mm_struct *mm)
{
          return mm != &init_mm && !mm_is_efi(mm);
}

Any objections?

Any reason not to use IS_ENABLED(CONFIG_EFI) in the above? The extern
declaration is visible to the compiler, and any references should
disappear before the linker could notice that efi_mm does not exist.

Sure, as long as the linker is happy why not. I'll let Ryan mess with that :)

I'm not sure if you are suggesting dropping the mm_is_efi() helper and just use
IS_ENABLED(CONFIG_EFI) in mm_is_user() to guard efi_mm, or if you are suggesting
using IS_ENABLED(CONFIG_EFI) in mm_is_efi() instead of the ifdefery?

The former was what I did initially; It works great, but I didn't like that I
was introducing a new code dependecy between efi and arm64 (nothing else outside
of efi references efi_mm).

So then concluded that it is safe to not worry about efi_mm (thanks for your
confirmation). But then David wanted a VM_WARN check, which reintroduces the
code dependency. So he suggested the mm_is_efi() helper to hide that... This is
all starting to feel circular...

I think Ard meant that inside mm_is_efi(), we could avoid the #ifdef and
simply use IS_ENABLED().

Yes.

static inline void mm_is_efi(struct mm_struct *mm)
{
    return IS_ENABLED(CONFIG_EFI) && mm == &efi_mm;
}