Re: apparmor NULL pointer dereference on resume

Re: apparmor NULL pointer dereference on resume

[Ryan Lee]

On Wed, Mar 5, 2025 at 1:47 PM Malte Schröder <malte.schroeder@tnxip.de> wrote:
>
> On 05/03/2025 20:22, Ryan Lee wrote:
> > On Wed, Mar 5, 2025 at 11:11 AM Malte Schröder <malte.schroeder@tnxip.de> wrote:
> >> Hi,
> >>
> >> I hope this is the right place to report this. Since 6.14-rc1 ff. resume
> >> from hibernate does not work anymore. Now I finally managed to get dmesg
> >> from when this happens (Console is frozen, but managed to login via
> >> network). If I read that trace correctly there seems to be some
> >> interaction with apparmor. I retried with apparmor disabled and the
> >> issue didn't trigger.
> > Also CC'ing the AppArmor-specific mailing list in this reply.
> >
> >> I am happy to provide more data if required.
> > Could you try to reproduce this NULL pointer dereference with a clean
> > kernel with debug info (that I'd be able to access the source code of)
> > and send a symbolized stacktrace processed with
> > scripts/decode_stacktrace.sh?
>
> Sure. Result using plain v6.14-rc5:
>
> [  142.014428] BUG: kernel NULL pointer dereference, address:
> 0000000000000018
> [  142.014429] #PF: supervisor read access in kernel mode
> [  142.014431] #PF: error_code(0x0000) - not-present page
> [  142.014432] PGD 0 P4D 0
> [  142.014433] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> [  142.014436] CPU: 4 UID: 0 PID: 6833 Comm: systemd-sleep Not tainted
> 6.14.0-rc5 #1
> [  142.014437] Hardware name: To Be Filled By O.E.M. X570 Extreme4/X570
> Extreme4, BIOS P5.60 01/18/2024
> [  142.014439] RIP: 0010:apparmor_file_open (./include/linux/mount.h:78
> (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> security/apparmor/lsm.c:483 (discriminator 2))
> [ 142.014442] Code: c5 00 08 00 00 0f 85 4b 01 00 00 4c 89 e9 31 c0 f6
> c1 02 0f 85 fd 00 00 00 48 8b 87 88 00 00 00 4c 8d b7 88 00 00 00 48 89
> fd <48> 8b 40 18 48 8b 4f 70 0f b7 11 48 89 c7 66 89 54 24 04 48 8b 51
> All code
> ========
>    0:    c5 00 08                 (bad)
>    3:    00 00                    add    %al,(%rax)
>    5:    0f 85 4b 01 00 00        jne    0x156
>    b:    4c 89 e9                 mov    %r13,%rcx
>    e:    31 c0                    xor    %eax,%eax
>   10:    f6 c1 02                 test   $0x2,%cl
>   13:    0f 85 fd 00 00 00        jne    0x116
>   19:    48 8b 87 88 00 00 00     mov    0x88(%rdi),%rax
>   20:    4c 8d b7 88 00 00 00     lea    0x88(%rdi),%r14
>   27:    48 89 fd                 mov    %rdi,%rbp
>   2a:*    48 8b 40 18              mov    0x18(%rax),%rax        <--
> trapping instruction
>   2e:    48 8b 4f 70              mov    0x70(%rdi),%rcx
>   32:    0f b7 11                 movzwl (%rcx),%edx
>   35:    48 89 c7                 mov    %rax,%rdi
>   38:    66 89 54 24 04           mov    %dx,0x4(%rsp)
>   3d:    48                       rex.W
>   3e:    8b                       .byte 0x8b
>   3f:    51                       push   %rcx
>
> Code starting with the faulting instruction
> ===========================================
>    0:    48 8b 40 18              mov    0x18(%rax),%rax
>    4:    48 8b 4f 70              mov    0x70(%rdi),%rcx
>    8:    0f b7 11                 movzwl (%rcx),%edx
>    b:    48 89 c7                 mov    %rax,%rdi
>    e:    66 89 54 24 04           mov    %dx,0x4(%rsp)
>   13:    48                       rex.W
>   14:    8b                       .byte 0x8b
>   15:    51                       push   %rcx
> [  142.014443] RSP: 0018:ffffb9ef7189bc50 EFLAGS: 00010246
> [  142.014445] RAX: 0000000000000000 RBX: ffff95eb5e555b00 RCX:
> 0000000000000300
> [  142.014446] RDX: ffff95f838227538 RSI: 00000000002ba677 RDI:
> ffff95e992be2a00
> [  142.014447] RBP: ffff95e992be2a00 R08: ffff95f838227520 R09:
> 0000000000000002
> [  142.014447] R10: ffff95ea72241d00 R11: 0000000000000001 R12:
> 0000000000000010
> [  142.014448] R13: 0000000000000300 R14: ffff95e992be2a88 R15:
> ffff95e95a6034e0
> [  142.014449] FS:  00007f74ab6cf880(0000) GS:ffff95f838200000(0000)
> knlGS:0000000000000000
> [  142.014450] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  142.014451] CR2: 0000000000000018 CR3: 00000002473b6000 CR4:
> 0000000000f50ef0
> [  142.014452] PKRU: 55555554
> [  142.014453] Call Trace:
> [  142.014454]  <TASK>
> [  142.014456] ? __die_body (arch/x86/kernel/dumpstack.c:421)
> [  142.014459] ? page_fault_oops (arch/x86/mm/fault.c:710)
> [  142.014460] ? __lock_acquire (kernel/locking/lockdep.c:?
> kernel/locking/lockdep.c:5174)
> [  142.014462] ? local_lock_acquire
> (./include/linux/local_lock_internal.h:29 (discriminator 1))
> [  142.014465] ? do_user_addr_fault (arch/x86/mm/fault.c:?)
> [  142.014467] ? exc_page_fault (./arch/x86/include/asm/irqflags.h:37
> ./arch/x86/include/asm/irqflags.h:92 arch/x86/mm/fault.c:1488
> arch/x86/mm/fault.c:1538)
> [  142.014468] ? asm_exc_page_fault (./arch/x86/include/asm/idtentry.h:623)
> [  142.014471] ? apparmor_file_open (./include/linux/mount.h:78
> (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> security/apparmor/lsm.c:483 (discriminator 2))
> [  142.014472] security_file_open (security/security.c:?)
> [  142.014474] do_dentry_open (fs/open.c:934)
> [  142.014476] kernel_file_open (fs/open.c:1201)
> [  142.014477] efivarfs_pm_notify (fs/efivarfs/super.c:505)

I traced the NULL dereference down to efivarfs_pm_notify creating a
struct path with a NULL .mnt pointer which is then passed into
kernel_file_open, which then invokes the LSM file_open security hook,
where AppArmor is not expecting a path that has a NULL .mnt pointer.
The code in question was introduced in b5d1e6ee761a (efivarfs: add
variable resync after hibernation).

I have sent in a patch to the AppArmor mailing list at
https://lists.ubuntu.com/archives/apparmor/2025-March/013545.html
which should give improved diagnostics for this case happening again.
My understanding is that path .mnt pointers generally should not be
NULL, but I do not know what an appropriate (non-NULL) value for that
pointer should be, as I am not familiar with the efivarfs subsystem.

> [  142.014479] ? __cfi_efivarfs_actor (fs/efivarfs/super.c:404)
> [  142.014480] ? fw_pm_notify (./include/linux/workqueue.h:?
> drivers/base/firmware_loader/main.c:1585
> drivers/base/firmware_loader/main.c:1615)
> [  142.014482] notifier_call_chain.llvm.227730641462406920
> (kernel/notifier.c:?)
> [  142.014485] blocking_notifier_call_chain (kernel/notifier.c:381)
> [  142.014486] hibernate (kernel/power/hibernate.c:850)
> [  142.014488] state_store (kernel/power/main.c:?)
> [  142.014490] kernfs_fop_write_iter.llvm.8178385953439778559
> (fs/kernfs/file.c:334)
> [  142.014492] vfs_write (fs/read_write.c:587 (discriminator 1))
> [  142.014494] ksys_write (fs/read_write.c:732)
> [  142.014496] do_syscall_64 (arch/x86/entry/common.c:?)
> [  142.014498] ? irqentry_exit_to_user_mode
> (./arch/x86/include/asm/processor.h:722
> ./arch/x86/include/asm/entry-common.h:100
> ./include/linux/entry-common.h:365 kernel/entry/common.c:233)
> [  142.014499] entry_SYSCALL_64_after_hwframe
> (arch/x86/entry/entry_64.S:130)
> [  142.014501] RIP: 0033:0x7f74ab09fe56
> [ 142.014503] Code: 89 df e8 7d bd 00 00 8b 93 08 03 00 00 59 5e 48 83
> f8 fc 75 15 83 e2 39 83 fa 08 75 0d e8 32 ff ff ff 66 90 48 8b 45 10 0f
> 05 <48> 8b 5d f8 c9 c3 0f 1f 40 00 f3 0f 1e fa 55 48 89 e5 48 83 ec 08
> All code
> ========
>    0:    89 df                    mov    %ebx,%edi
>    2:    e8 7d bd 00 00           call   0xbd84
>    7:    8b 93 08 03 00 00        mov    0x308(%rbx),%edx
>    d:    59                       pop    %rcx
>    e:    5e                       pop    %rsi
>    f:    48 83 f8 fc              cmp    $0xfffffffffffffffc,%rax
>   13:    75 15                    jne    0x2a
>   15:    83 e2 39                 and    $0x39,%edx
>   18:    83 fa 08                 cmp    $0x8,%edx
>   1b:    75 0d                    jne    0x2a
>   1d:    e8 32 ff ff ff           call   0xffffffffffffff54
>   22:    66 90                    xchg   %ax,%ax
>   24:    48 8b 45 10              mov    0x10(%rbp),%rax
>   28:    0f 05                    syscall
>   2a:*    48 8b 5d f8              mov    -0x8(%rbp),%rbx        <--
> trapping instruction
>   2e:    c9                       leave
>   2f:    c3                       ret
>   30:    0f 1f 40 00              nopl   0x0(%rax)
>   34:    f3 0f 1e fa              endbr64
>   38:    55                       push   %rbp
>   39:    48 89 e5                 mov    %rsp,%rbp
>   3c:    48 83 ec 08              sub    $0x8,%rsp
>
> Code starting with the faulting instruction
> ===========================================
>    0:    48 8b 5d f8              mov    -0x8(%rbp),%rbx
>    4:    c9                       leave
>    5:    c3                       ret
>    6:    0f 1f 40 00              nopl   0x0(%rax)
>    a:    f3 0f 1e fa              endbr64
>    e:    55                       push   %rbp
>    f:    48 89 e5                 mov    %rsp,%rbp
>   12:    48 83 ec 08              sub    $0x8,%rsp
> [  142.014504] RSP: 002b:00007ffcbaa60170 EFLAGS: 00000202 ORIG_RAX:
> 0000000000000001
> [  142.014505] RAX: ffffffffffffffda RBX: 00007f74ab6cf880 RCX:
> 00007f74ab09fe56
> [  142.014506] RDX: 0000000000000005 RSI: 000056105b899c30 RDI:
> 0000000000000007
> [  142.014507] RBP: 00007ffcbaa60180 R08: 0000000000000000 R09:
> 0000000000000000
> [  142.014507] R10: 0000000000000000 R11: 0000000000000202 R12:
> 0000000000000005
> [  142.014508] R13: 000056105b899c30 R14: 000056105b88e2a0 R15:
> 00007f74ab1f4ea0
> [  142.014510]  </TASK>
> [  142.014510] Modules linked in: rfcomm bnep snd_seq_dummy snd_hrtimer
> snd_seq nct6775 hwmon_vid nct6775_core btusb btrtl btmtk btbcm btintel
> bluetooth snd_usb_audio snd_ump snd_usbmidi_lib snd_rawmidi
> snd_seq_device snd_hda_codec_realtek snd_hda_scodec_component
> snd_hda_codec_generic kvm_amd snd_hda_codec_hdmi snd_hda_intel
> snd_intel_dspcfg snd_hda_codec snd_hwdep cfg80211 kvm wmi_bmof rfkill
> snd_pcsp snd_hda_core snd_ctxfi nfnetlink ip_tables x_tables
> hid_roccat_kone hid_roccat hid_logitech_hidpp hid_logitech_dj amdgpu
> drm_panel_backlight_quirks cec mfd_core drm_buddy drm_suballoc_helper
> video drm_exec drm_display_helper gpu_sched amdxcp drm_client_lib
> drm_ttm_helper drm_kms_helper ttm wmi uhid snd_aloop snd_pcm snd_timer
> snd soundcore videodev mc
> [  142.014533] CR2: 0000000000000018
> [  142.014534] ---[ end trace 0000000000000000 ]---
> [  142.023833] mlx4_en: 0000:04:00.0: Port 1: Using 32 TX rings
> [  142.023836] mlx4_en: 0000:04:00.0: Port 1: Using 16 RX rings
> [  142.031513] mlx4_en: 0000:04:00.0: Port 1: Initializing port
> [  142.031932] mlx4_en 0000:04:00.0: registered PHC clock
> [  142.078775] mlx4_en: eth0: Link Up
> [  142.157707] RIP: 0010:apparmor_file_open (./include/linux/mount.h:78
> (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> security/apparmor/lsm.c:483 (discriminator 2))
> [ 142.157711] Code: c5 00 08 00 00 0f 85 4b 01 00 00 4c 89 e9 31 c0 f6
> c1 02 0f 85 fd 00 00 00 48 8b 87 88 00 00 00 4c 8d b7 88 00 00 00 48 89
> fd <48> 8b 40 18 48 8b 4f 70 0f b7 11 48 89 c7 66 89 54 24 04 48 8b 51
> All code
> ========
>    0:    c5 00 08                 (bad)
>    3:    00 00                    add    %al,(%rax)
>    5:    0f 85 4b 01 00 00        jne    0x156
>    b:    4c 89 e9                 mov    %r13,%rcx
>    e:    31 c0                    xor    %eax,%eax
>   10:    f6 c1 02                 test   $0x2,%cl
>   13:    0f 85 fd 00 00 00        jne    0x116
>   19:    48 8b 87 88 00 00 00     mov    0x88(%rdi),%rax
>   20:    4c 8d b7 88 00 00 00     lea    0x88(%rdi),%r14
>   27:    48 89 fd                 mov    %rdi,%rbp
>   2a:*    48 8b 40 18              mov    0x18(%rax),%rax        <--
> trapping instruction
>   2e:    48 8b 4f 70              mov    0x70(%rdi),%rcx
>   32:    0f b7 11                 movzwl (%rcx),%edx
>   35:    48 89 c7                 mov    %rax,%rdi
>   38:    66 89 54 24 04           mov    %dx,0x4(%rsp)
>   3d:    48                       rex.W
>   3e:    8b                       .byte 0x8b
>   3f:    51                       push   %rcx
>
> Code starting with the faulting instruction
> ===========================================
>    0:    48 8b 40 18              mov    0x18(%rax),%rax
>    4:    48 8b 4f 70              mov    0x70(%rdi),%rcx
>    8:    0f b7 11                 movzwl (%rcx),%edx
>    b:    48 89 c7                 mov    %rax,%rdi
>    e:    66 89 54 24 04           mov    %dx,0x4(%rsp)
>   13:    48                       rex.W
>   14:    8b                       .byte 0x8b
>   15:    51                       push   %rcx
> [  142.157713] RSP: 0018:ffffb9ef7189bc50 EFLAGS: 00010246
> [  142.157714] RAX: 0000000000000000 RBX: ffff95eb5e555b00 RCX:
> 0000000000000300
> [  142.157715] RDX: ffff95f838227538 RSI: 00000000002ba677 RDI:
> ffff95e992be2a00
> [  142.157716] RBP: ffff95e992be2a00 R08: ffff95f838227520 R09:
> 0000000000000002
> [  142.157717] R10: ffff95ea72241d00 R11: 0000000000000001 R12:
> 0000000000000010
> [  142.157718] R13: 0000000000000300 R14: ffff95e992be2a88 R15:
> ffff95e95a6034e0
> [  142.157719] FS:  00007f74ab6cf880(0000) GS:ffff95f838200000(0000)
> knlGS:0000000000000000
> [  142.157720] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  142.157720] CR2: 0000000000000018 CR3: 00000002473b6000 CR4:
> 0000000000f50ef0
> [  142.157721] PKRU: 55555554
>
> >
> >> Regards
> >>
> >> /Malte
> >>
> >> ...
> >> <snip old backtrace>
> >> ...
> >>
> >>
> > Ryan

CC'ing in the Linux EFI mailing list, John Johansen (AppArmor
maintainer), James Bottomley (original author of the patch that
introduced the code in question), and Jeremy Kerr+Ard Biesheuvel
(efivarfs maintainers).

Ryan

Re: apparmor NULL pointer dereference on resume [efivarfs]

[James Bottomley]

On Mon, 2025-03-10 at 12:57 -0700, Ryan Lee wrote:
> On Wed, Mar 5, 2025 at 1:47 PM Malte Schröder
> <malte.schroeder@tnxip.de> wrote:
> > 
> > On 05/03/2025 20:22, Ryan Lee wrote:
> > > On Wed, Mar 5, 2025 at 11:11 AM Malte Schröder
> > > <malte.schroeder@tnxip.de> wrote:
> > > > Hi,
> > > > 
> > > > I hope this is the right place to report this. Since 6.14-rc1
> > > > ff. resume
> > > > from hibernate does not work anymore. Now I finally managed to
> > > > get dmesg
> > > > from when this happens (Console is frozen, but managed to login
> > > > via
> > > > network). If I read that trace correctly there seems to be some
> > > > interaction with apparmor. I retried with apparmor disabled and
> > > > the
> > > > issue didn't trigger.
> > > Also CC'ing the AppArmor-specific mailing list in this reply.
> > > 
> > > > I am happy to provide more data if required.
> > > Could you try to reproduce this NULL pointer dereference with a
> > > clean
> > > kernel with debug info (that I'd be able to access the source
> > > code of)
> > > and send a symbolized stacktrace processed with
> > > scripts/decode_stacktrace.sh?
> > 
> > Sure. Result using plain v6.14-rc5:
> > 
> > [  142.014428] BUG: kernel NULL pointer dereference, address:
> > 0000000000000018
> > [  142.014429] #PF: supervisor read access in kernel mode
> > [  142.014431] #PF: error_code(0x0000) - not-present page
> > [  142.014432] PGD 0 P4D 0
> > [  142.014433] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [  142.014436] CPU: 4 UID: 0 PID: 6833 Comm: systemd-sleep Not
> > tainted
> > 6.14.0-rc5 #1
> > [  142.014437] Hardware name: To Be Filled By O.E.M. X570
> > Extreme4/X570
> > Extreme4, BIOS P5.60 01/18/2024
> > [  142.014439] RIP: 0010:apparmor_file_open
> > (./include/linux/mount.h:78
> > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > security/apparmor/lsm.c:483 (discriminator 2))
> > [ 142.014442] Code: c5 00 08 00 00 0f 85 4b 01 00 00 4c 89 e9 31 c0
> > f6
> > c1 02 0f 85 fd 00 00 00 48 8b 87 88 00 00 00 4c 8d b7 88 00 00 00
> > 48 89
> > fd <48> 8b 40 18 48 8b 4f 70 0f b7 11 48 89 c7 66 89 54 24 04 48 8b
> > 51
> > All code
> > ========
> >    0:    c5 00 08                 (bad)
> >    3:    00 00                    add    %al,(%rax)
> >    5:    0f 85 4b 01 00 00        jne    0x156
> >    b:    4c 89 e9                 mov    %r13,%rcx
> >    e:    31 c0                    xor    %eax,%eax
> >   10:    f6 c1 02                 test   $0x2,%cl
> >   13:    0f 85 fd 00 00 00        jne    0x116
> >   19:    48 8b 87 88 00 00 00     mov    0x88(%rdi),%rax
> >   20:    4c 8d b7 88 00 00 00     lea    0x88(%rdi),%r14
> >   27:    48 89 fd                 mov    %rdi,%rbp
> >   2a:*    48 8b 40 18              mov    0x18(%rax),%rax        <-
> > -
> > trapping instruction
> >   2e:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> >   32:    0f b7 11                 movzwl (%rcx),%edx
> >   35:    48 89 c7                 mov    %rax,%rdi
> >   38:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> >   3d:    48                       rex.W
> >   3e:    8b                       .byte 0x8b
> >   3f:    51                       push   %rcx
> > 
> > Code starting with the faulting instruction
> > ===========================================
> >    0:    48 8b 40 18              mov    0x18(%rax),%rax
> >    4:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> >    8:    0f b7 11                 movzwl (%rcx),%edx
> >    b:    48 89 c7                 mov    %rax,%rdi
> >    e:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> >   13:    48                       rex.W
> >   14:    8b                       .byte 0x8b
> >   15:    51                       push   %rcx
> > [  142.014443] RSP: 0018:ffffb9ef7189bc50 EFLAGS: 00010246
> > [  142.014445] RAX: 0000000000000000 RBX: ffff95eb5e555b00 RCX:
> > 0000000000000300
> > [  142.014446] RDX: ffff95f838227538 RSI: 00000000002ba677 RDI:
> > ffff95e992be2a00
> > [  142.014447] RBP: ffff95e992be2a00 R08: ffff95f838227520 R09:
> > 0000000000000002
> > [  142.014447] R10: ffff95ea72241d00 R11: 0000000000000001 R12:
> > 0000000000000010
> > [  142.014448] R13: 0000000000000300 R14: ffff95e992be2a88 R15:
> > ffff95e95a6034e0
> > [  142.014449] FS:  00007f74ab6cf880(0000)
> > GS:ffff95f838200000(0000)
> > knlGS:0000000000000000
> > [  142.014450] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  142.014451] CR2: 0000000000000018 CR3: 00000002473b6000 CR4:
> > 0000000000f50ef0
> > [  142.014452] PKRU: 55555554
> > [  142.014453] Call Trace:
> > [  142.014454]  <TASK>
> > [  142.014456] ? __die_body (arch/x86/kernel/dumpstack.c:421)
> > [  142.014459] ? page_fault_oops (arch/x86/mm/fault.c:710)
> > [  142.014460] ? __lock_acquire (kernel/locking/lockdep.c:?
> > kernel/locking/lockdep.c:5174)
> > [  142.014462] ? local_lock_acquire
> > (./include/linux/local_lock_internal.h:29 (discriminator 1))
> > [  142.014465] ? do_user_addr_fault (arch/x86/mm/fault.c:?)
> > [  142.014467] ? exc_page_fault
> > (./arch/x86/include/asm/irqflags.h:37
> > ./arch/x86/include/asm/irqflags.h:92 arch/x86/mm/fault.c:1488
> > arch/x86/mm/fault.c:1538)
> > [  142.014468] ? asm_exc_page_fault
> > (./arch/x86/include/asm/idtentry.h:623)
> > [  142.014471] ? apparmor_file_open (./include/linux/mount.h:78
> > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > security/apparmor/lsm.c:483 (discriminator 2))
> > [  142.014472] security_file_open (security/security.c:?)
> > [  142.014474] do_dentry_open (fs/open.c:934)
> > [  142.014476] kernel_file_open (fs/open.c:1201)
> > [  142.014477] efivarfs_pm_notify (fs/efivarfs/super.c:505)
> 
> I traced the NULL dereference down to efivarfs_pm_notify creating a
> struct path with a NULL .mnt pointer which is then passed into
> kernel_file_open, which then invokes the LSM file_open security hook,
> where AppArmor is not expecting a path that has a NULL .mnt pointer.
> The code in question was introduced in b5d1e6ee761a (efivarfs: add
> variable resync after hibernation).
> 
> I have sent in a patch to the AppArmor mailing list at
> https://lists.ubuntu.com/archives/apparmor/2025-March/013545.html
> which should give improved diagnostics for this case happening again.
> My understanding is that path .mnt pointers generally should not be
> NULL, but I do not know what an appropriate (non-NULL) value for that
> pointer should be, as I am not familiar with the efivarfs subsystem.

The problem comes down to the superblock functions not being able to
get the struct vfsmount for the superblock (because it isn't even
allocated until after they've all been called).  The assumption I was
operating under was that provided I added O_NOATIME to prevent the
parent directory being updated, passing in a NULL mnt for the purposes
of iterating the directory dentry was safe.  What apparmour is trying
to do is look up the idmap for the mount point to do one of its checks.

There are two ways of fixing this that I can think of.  One would be
exporting a function that lets me dig the vfsmount out of s_mounts and
use that (it's well hidden in the internals of fs/mount.h, so I suspect
this might not be very acceptable) or to get mnt_idmap to return
&nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards the
latter, but I'm cc'ing fsdevel to see what others think.

Regards,

James

Re: apparmor NULL pointer dereference on resume [efivarfs]

[Ard Biesheuvel]

(cc Al Viro)

On Mon, 10 Mar 2025 at 22:49, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Mon, 2025-03-10 at 12:57 -0700, Ryan Lee wrote:
> > On Wed, Mar 5, 2025 at 1:47 PM Malte Schröder
> > <malte.schroeder@tnxip.de> wrote:
> > >
> > > On 05/03/2025 20:22, Ryan Lee wrote:
> > > > On Wed, Mar 5, 2025 at 11:11 AM Malte Schröder
> > > > <malte.schroeder@tnxip.de> wrote:
> > > > > Hi,
> > > > >
> > > > > I hope this is the right place to report this. Since 6.14-rc1
> > > > > ff. resume
> > > > > from hibernate does not work anymore. Now I finally managed to
> > > > > get dmesg
> > > > > from when this happens (Console is frozen, but managed to login
> > > > > via
> > > > > network). If I read that trace correctly there seems to be some
> > > > > interaction with apparmor. I retried with apparmor disabled and
> > > > > the
> > > > > issue didn't trigger.
> > > > Also CC'ing the AppArmor-specific mailing list in this reply.
> > > >
> > > > > I am happy to provide more data if required.
> > > > Could you try to reproduce this NULL pointer dereference with a
> > > > clean
> > > > kernel with debug info (that I'd be able to access the source
> > > > code of)
> > > > and send a symbolized stacktrace processed with
> > > > scripts/decode_stacktrace.sh?
> > >
> > > Sure. Result using plain v6.14-rc5:
> > >
> > > [  142.014428] BUG: kernel NULL pointer dereference, address:
> > > 0000000000000018
> > > [  142.014429] #PF: supervisor read access in kernel mode
> > > [  142.014431] #PF: error_code(0x0000) - not-present page
> > > [  142.014432] PGD 0 P4D 0
> > > [  142.014433] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > [  142.014436] CPU: 4 UID: 0 PID: 6833 Comm: systemd-sleep Not
> > > tainted
> > > 6.14.0-rc5 #1
> > > [  142.014437] Hardware name: To Be Filled By O.E.M. X570
> > > Extreme4/X570
> > > Extreme4, BIOS P5.60 01/18/2024
> > > [  142.014439] RIP: 0010:apparmor_file_open
> > > (./include/linux/mount.h:78
> > > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > > security/apparmor/lsm.c:483 (discriminator 2))
> > > [ 142.014442] Code: c5 00 08 00 00 0f 85 4b 01 00 00 4c 89 e9 31 c0
> > > f6
> > > c1 02 0f 85 fd 00 00 00 48 8b 87 88 00 00 00 4c 8d b7 88 00 00 00
> > > 48 89
> > > fd <48> 8b 40 18 48 8b 4f 70 0f b7 11 48 89 c7 66 89 54 24 04 48 8b
> > > 51
> > > All code
> > > ========
> > >    0:    c5 00 08                 (bad)
> > >    3:    00 00                    add    %al,(%rax)
> > >    5:    0f 85 4b 01 00 00        jne    0x156
> > >    b:    4c 89 e9                 mov    %r13,%rcx
> > >    e:    31 c0                    xor    %eax,%eax
> > >   10:    f6 c1 02                 test   $0x2,%cl
> > >   13:    0f 85 fd 00 00 00        jne    0x116
> > >   19:    48 8b 87 88 00 00 00     mov    0x88(%rdi),%rax
> > >   20:    4c 8d b7 88 00 00 00     lea    0x88(%rdi),%r14
> > >   27:    48 89 fd                 mov    %rdi,%rbp
> > >   2a:*    48 8b 40 18              mov    0x18(%rax),%rax        <-
> > > -
> > > trapping instruction
> > >   2e:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> > >   32:    0f b7 11                 movzwl (%rcx),%edx
> > >   35:    48 89 c7                 mov    %rax,%rdi
> > >   38:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> > >   3d:    48                       rex.W
> > >   3e:    8b                       .byte 0x8b
> > >   3f:    51                       push   %rcx
> > >
> > > Code starting with the faulting instruction
> > > ===========================================
> > >    0:    48 8b 40 18              mov    0x18(%rax),%rax
> > >    4:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> > >    8:    0f b7 11                 movzwl (%rcx),%edx
> > >    b:    48 89 c7                 mov    %rax,%rdi
> > >    e:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> > >   13:    48                       rex.W
> > >   14:    8b                       .byte 0x8b
> > >   15:    51                       push   %rcx
> > > [  142.014443] RSP: 0018:ffffb9ef7189bc50 EFLAGS: 00010246
> > > [  142.014445] RAX: 0000000000000000 RBX: ffff95eb5e555b00 RCX:
> > > 0000000000000300
> > > [  142.014446] RDX: ffff95f838227538 RSI: 00000000002ba677 RDI:
> > > ffff95e992be2a00
> > > [  142.014447] RBP: ffff95e992be2a00 R08: ffff95f838227520 R09:
> > > 0000000000000002
> > > [  142.014447] R10: ffff95ea72241d00 R11: 0000000000000001 R12:
> > > 0000000000000010
> > > [  142.014448] R13: 0000000000000300 R14: ffff95e992be2a88 R15:
> > > ffff95e95a6034e0
> > > [  142.014449] FS:  00007f74ab6cf880(0000)
> > > GS:ffff95f838200000(0000)
> > > knlGS:0000000000000000
> > > [  142.014450] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [  142.014451] CR2: 0000000000000018 CR3: 00000002473b6000 CR4:
> > > 0000000000f50ef0
> > > [  142.014452] PKRU: 55555554
> > > [  142.014453] Call Trace:
> > > [  142.014454]  <TASK>
> > > [  142.014456] ? __die_body (arch/x86/kernel/dumpstack.c:421)
> > > [  142.014459] ? page_fault_oops (arch/x86/mm/fault.c:710)
> > > [  142.014460] ? __lock_acquire (kernel/locking/lockdep.c:?
> > > kernel/locking/lockdep.c:5174)
> > > [  142.014462] ? local_lock_acquire
> > > (./include/linux/local_lock_internal.h:29 (discriminator 1))
> > > [  142.014465] ? do_user_addr_fault (arch/x86/mm/fault.c:?)
> > > [  142.014467] ? exc_page_fault
> > > (./arch/x86/include/asm/irqflags.h:37
> > > ./arch/x86/include/asm/irqflags.h:92 arch/x86/mm/fault.c:1488
> > > arch/x86/mm/fault.c:1538)
> > > [  142.014468] ? asm_exc_page_fault
> > > (./arch/x86/include/asm/idtentry.h:623)
> > > [  142.014471] ? apparmor_file_open (./include/linux/mount.h:78
> > > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > > security/apparmor/lsm.c:483 (discriminator 2))
> > > [  142.014472] security_file_open (security/security.c:?)
> > > [  142.014474] do_dentry_open (fs/open.c:934)
> > > [  142.014476] kernel_file_open (fs/open.c:1201)
> > > [  142.014477] efivarfs_pm_notify (fs/efivarfs/super.c:505)
> >
> > I traced the NULL dereference down to efivarfs_pm_notify creating a
> > struct path with a NULL .mnt pointer which is then passed into
> > kernel_file_open, which then invokes the LSM file_open security hook,
> > where AppArmor is not expecting a path that has a NULL .mnt pointer.
> > The code in question was introduced in b5d1e6ee761a (efivarfs: add
> > variable resync after hibernation).
> >
> > I have sent in a patch to the AppArmor mailing list at
> > https://lists.ubuntu.com/archives/apparmor/2025-March/013545.html
> > which should give improved diagnostics for this case happening again.
> > My understanding is that path .mnt pointers generally should not be
> > NULL, but I do not know what an appropriate (non-NULL) value for that
> > pointer should be, as I am not familiar with the efivarfs subsystem.
>
> The problem comes down to the superblock functions not being able to
> get the struct vfsmount for the superblock (because it isn't even
> allocated until after they've all been called).  The assumption I was
> operating under was that provided I added O_NOATIME to prevent the
> parent directory being updated, passing in a NULL mnt for the purposes
> of iterating the directory dentry was safe.  What apparmour is trying
> to do is look up the idmap for the mount point to do one of its checks.
>
> There are two ways of fixing this that I can think of.  One would be
> exporting a function that lets me dig the vfsmount out of s_mounts and
> use that (it's well hidden in the internals of fs/mount.h, so I suspect
> this might not be very acceptable) or to get mnt_idmap to return
> &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards the
> latter, but I'm cc'ing fsdevel to see what others think.
>


Al spotted the same issue based on a syzbot report [0]

[0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/

Re: apparmor NULL pointer dereference on resume [efivarfs]

[Christian Brauner]

On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:
> (cc Al Viro)
> 
> On Mon, 10 Mar 2025 at 22:49, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > On Mon, 2025-03-10 at 12:57 -0700, Ryan Lee wrote:
> > > On Wed, Mar 5, 2025 at 1:47 PM Malte Schröder
> > > <malte.schroeder@tnxip.de> wrote:
> > > >
> > > > On 05/03/2025 20:22, Ryan Lee wrote:
> > > > > On Wed, Mar 5, 2025 at 11:11 AM Malte Schröder
> > > > > <malte.schroeder@tnxip.de> wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I hope this is the right place to report this. Since 6.14-rc1
> > > > > > ff. resume
> > > > > > from hibernate does not work anymore. Now I finally managed to
> > > > > > get dmesg
> > > > > > from when this happens (Console is frozen, but managed to login
> > > > > > via
> > > > > > network). If I read that trace correctly there seems to be some
> > > > > > interaction with apparmor. I retried with apparmor disabled and
> > > > > > the
> > > > > > issue didn't trigger.
> > > > > Also CC'ing the AppArmor-specific mailing list in this reply.
> > > > >
> > > > > > I am happy to provide more data if required.
> > > > > Could you try to reproduce this NULL pointer dereference with a
> > > > > clean
> > > > > kernel with debug info (that I'd be able to access the source
> > > > > code of)
> > > > > and send a symbolized stacktrace processed with
> > > > > scripts/decode_stacktrace.sh?
> > > >
> > > > Sure. Result using plain v6.14-rc5:
> > > >
> > > > [  142.014428] BUG: kernel NULL pointer dereference, address:
> > > > 0000000000000018
> > > > [  142.014429] #PF: supervisor read access in kernel mode
> > > > [  142.014431] #PF: error_code(0x0000) - not-present page
> > > > [  142.014432] PGD 0 P4D 0
> > > > [  142.014433] Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > > [  142.014436] CPU: 4 UID: 0 PID: 6833 Comm: systemd-sleep Not
> > > > tainted
> > > > 6.14.0-rc5 #1
> > > > [  142.014437] Hardware name: To Be Filled By O.E.M. X570
> > > > Extreme4/X570
> > > > Extreme4, BIOS P5.60 01/18/2024
> > > > [  142.014439] RIP: 0010:apparmor_file_open
> > > > (./include/linux/mount.h:78
> > > > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > > > security/apparmor/lsm.c:483 (discriminator 2))
> > > > [ 142.014442] Code: c5 00 08 00 00 0f 85 4b 01 00 00 4c 89 e9 31 c0
> > > > f6
> > > > c1 02 0f 85 fd 00 00 00 48 8b 87 88 00 00 00 4c 8d b7 88 00 00 00
> > > > 48 89
> > > > fd <48> 8b 40 18 48 8b 4f 70 0f b7 11 48 89 c7 66 89 54 24 04 48 8b
> > > > 51
> > > > All code
> > > > ========
> > > >    0:    c5 00 08                 (bad)
> > > >    3:    00 00                    add    %al,(%rax)
> > > >    5:    0f 85 4b 01 00 00        jne    0x156
> > > >    b:    4c 89 e9                 mov    %r13,%rcx
> > > >    e:    31 c0                    xor    %eax,%eax
> > > >   10:    f6 c1 02                 test   $0x2,%cl
> > > >   13:    0f 85 fd 00 00 00        jne    0x116
> > > >   19:    48 8b 87 88 00 00 00     mov    0x88(%rdi),%rax
> > > >   20:    4c 8d b7 88 00 00 00     lea    0x88(%rdi),%r14
> > > >   27:    48 89 fd                 mov    %rdi,%rbp
> > > >   2a:*    48 8b 40 18              mov    0x18(%rax),%rax        <-
> > > > -
> > > > trapping instruction
> > > >   2e:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> > > >   32:    0f b7 11                 movzwl (%rcx),%edx
> > > >   35:    48 89 c7                 mov    %rax,%rdi
> > > >   38:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> > > >   3d:    48                       rex.W
> > > >   3e:    8b                       .byte 0x8b
> > > >   3f:    51                       push   %rcx
> > > >
> > > > Code starting with the faulting instruction
> > > > ===========================================
> > > >    0:    48 8b 40 18              mov    0x18(%rax),%rax
> > > >    4:    48 8b 4f 70              mov    0x70(%rdi),%rcx
> > > >    8:    0f b7 11                 movzwl (%rcx),%edx
> > > >    b:    48 89 c7                 mov    %rax,%rdi
> > > >    e:    66 89 54 24 04           mov    %dx,0x4(%rsp)
> > > >   13:    48                       rex.W
> > > >   14:    8b                       .byte 0x8b
> > > >   15:    51                       push   %rcx
> > > > [  142.014443] RSP: 0018:ffffb9ef7189bc50 EFLAGS: 00010246
> > > > [  142.014445] RAX: 0000000000000000 RBX: ffff95eb5e555b00 RCX:
> > > > 0000000000000300
> > > > [  142.014446] RDX: ffff95f838227538 RSI: 00000000002ba677 RDI:
> > > > ffff95e992be2a00
> > > > [  142.014447] RBP: ffff95e992be2a00 R08: ffff95f838227520 R09:
> > > > 0000000000000002
> > > > [  142.014447] R10: ffff95ea72241d00 R11: 0000000000000001 R12:
> > > > 0000000000000010
> > > > [  142.014448] R13: 0000000000000300 R14: ffff95e992be2a88 R15:
> > > > ffff95e95a6034e0
> > > > [  142.014449] FS:  00007f74ab6cf880(0000)
> > > > GS:ffff95f838200000(0000)
> > > > knlGS:0000000000000000
> > > > [  142.014450] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [  142.014451] CR2: 0000000000000018 CR3: 00000002473b6000 CR4:
> > > > 0000000000f50ef0
> > > > [  142.014452] PKRU: 55555554
> > > > [  142.014453] Call Trace:
> > > > [  142.014454]  <TASK>
> > > > [  142.014456] ? __die_body (arch/x86/kernel/dumpstack.c:421)
> > > > [  142.014459] ? page_fault_oops (arch/x86/mm/fault.c:710)
> > > > [  142.014460] ? __lock_acquire (kernel/locking/lockdep.c:?
> > > > kernel/locking/lockdep.c:5174)
> > > > [  142.014462] ? local_lock_acquire
> > > > (./include/linux/local_lock_internal.h:29 (discriminator 1))
> > > > [  142.014465] ? do_user_addr_fault (arch/x86/mm/fault.c:?)
> > > > [  142.014467] ? exc_page_fault
> > > > (./arch/x86/include/asm/irqflags.h:37
> > > > ./arch/x86/include/asm/irqflags.h:92 arch/x86/mm/fault.c:1488
> > > > arch/x86/mm/fault.c:1538)
> > > > [  142.014468] ? asm_exc_page_fault
> > > > (./arch/x86/include/asm/idtentry.h:623)
> > > > [  142.014471] ? apparmor_file_open (./include/linux/mount.h:78
> > > > (discriminator 2) ./include/linux/fs.h:2781 (discriminator 2)
> > > > security/apparmor/lsm.c:483 (discriminator 2))
> > > > [  142.014472] security_file_open (security/security.c:?)
> > > > [  142.014474] do_dentry_open (fs/open.c:934)
> > > > [  142.014476] kernel_file_open (fs/open.c:1201)
> > > > [  142.014477] efivarfs_pm_notify (fs/efivarfs/super.c:505)
> > >
> > > I traced the NULL dereference down to efivarfs_pm_notify creating a
> > > struct path with a NULL .mnt pointer which is then passed into
> > > kernel_file_open, which then invokes the LSM file_open security hook,
> > > where AppArmor is not expecting a path that has a NULL .mnt pointer.
> > > The code in question was introduced in b5d1e6ee761a (efivarfs: add
> > > variable resync after hibernation).
> > >
> > > I have sent in a patch to the AppArmor mailing list at
> > > https://lists.ubuntu.com/archives/apparmor/2025-March/013545.html
> > > which should give improved diagnostics for this case happening again.
> > > My understanding is that path .mnt pointers generally should not be
> > > NULL, but I do not know what an appropriate (non-NULL) value for that
> > > pointer should be, as I am not familiar with the efivarfs subsystem.
> >
> > The problem comes down to the superblock functions not being able to
> > get the struct vfsmount for the superblock (because it isn't even
> > allocated until after they've all been called).  The assumption I was
> > operating under was that provided I added O_NOATIME to prevent the
> > parent directory being updated, passing in a NULL mnt for the purposes
> > of iterating the directory dentry was safe.  What apparmour is trying
> > to do is look up the idmap for the mount point to do one of its checks.
> >
> > There are two ways of fixing this that I can think of.  One would be
> > exporting a function that lets me dig the vfsmount out of s_mounts and
> > use that (it's well hidden in the internals of fs/mount.h, so I suspect
> > this might not be very acceptable) or to get mnt_idmap to return

Nope, please don't.

> > &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards the
> > latter, but I'm cc'ing fsdevel to see what others think.

A struct path with mount NULL and dentry != NULL is guaranteed to bit us
in the ass in other places. That's the bug.

> >
> 
> 
> Al spotted the same issue based on a syzbot report [0]
> 
> [0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/

efivars as written only has a single global superblock and it doesn't
support idmapped mounts and I don't see why it ever would.

But since efivars does only ever have a single global superblock, one
possibility is to an internal superblock that always exits and is
resurfaced whenever userspace mounts efivarfs. That's essentially the
devtmpfs model.

Then you can stash:

static struct vfsmount *efivarfs_mnt;

globally and use that in efivarfs_pm_notify() to fill in struct path.

Re: apparmor NULL pointer dereference on resume [efivarfs]

[James Bottomley]

On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
> On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:
> > (cc Al Viro)
> > 
> > On Mon, 10 Mar 2025 at 22:49, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
[...]
> > > The problem comes down to the superblock functions not being able
> > > to get the struct vfsmount for the superblock (because it isn't
> > > even allocated until after they've all been called).  The
> > > assumption I was operating under was that provided I added
> > > O_NOATIME to prevent the parent directory being updated, passing
> > > in a NULL mnt for the purposes of iterating the directory dentry
> > > was safe.  What apparmour is trying to do is look up the idmap
> > > for the mount point to do one of its checks.
> > > 
> > > There are two ways of fixing this that I can think of.  One would
> > > be exporting a function that lets me dig the vfsmount out of
> > > s_mounts and use that (it's well hidden in the internals of
> > > fs/mount.h, so I suspect this might not be very acceptable) or to
> > > get mnt_idmap to return
> 
> Nope, please don't.
> 
> > > &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards
> > > the latter, but I'm cc'ing fsdevel to see what others think.
> 
> A struct path with mount NULL and dentry != NULL is guaranteed to bit
> us in the ass in other places. That's the bug.
> 
> > > 
> > 
> > 
> > Al spotted the same issue based on a syzbot report [0]
> > 
> > [0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/
> 
> efivars as written only has a single global superblock and it doesn't
> support idmapped mounts and I don't see why it ever would.

So that's not quite true: efivarfs currently supports uid and gid
mapping as mount options, which certainly looks like they were designed
to allow a second mount in a user directory.  I've no idea what the
actual use case for this is, but if I go for a single superblock, any
reconfigure with new uid/gid would become globally effective (change
every current mount) because they're stored in the superblock
information.

So what is the use case for this uid/gid parameter?  If no-one can
remember and no-one actually uses it, perhaps the whole config path can
be simplified by getting rid of the options?  Even if there is a use
case, if it's single mount only then we can still go with a global
superblock.

> But since efivars does only ever have a single global superblock, one
> possibility is to an internal superblock that always exits and is
> resurfaced whenever userspace mounts efivarfs. That's essentially the
> devtmpfs model.
> 
> Then you can stash:
> 
> static struct vfsmount *efivarfs_mnt;
> 
> globally and use that in efivarfs_pm_notify() to fill in struct path.

I didn't see devtmpfs when looking for examples, since it's hiding
outside of the fs/ directory.  However, it does seem to be a bit legacy
nasty as an example to copy.  However, I get the basics: we'd
instantiate the mnt and superblock on init (stashing mnt in the sfi so
the notifier gets it).  Then we can do the variable population on
reconfigure, just in case an EFI system doesn't want to mount efivarfs
to save memory.

I can code that up if I can get an answer to the uid/gid parameter
question above.

Regards,

James

Re: apparmor NULL pointer dereference on resume [efivarfs]

[Christian Brauner]

On Tue, Mar 11, 2025 at 09:01:36AM -0400, James Bottomley wrote:
> On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
> > On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:
> > > (cc Al Viro)
> > > 
> > > On Mon, 10 Mar 2025 at 22:49, James Bottomley
> > > <James.Bottomley@hansenpartnership.com> wrote:
> [...]
> > > > The problem comes down to the superblock functions not being able
> > > > to get the struct vfsmount for the superblock (because it isn't
> > > > even allocated until after they've all been called).  The
> > > > assumption I was operating under was that provided I added
> > > > O_NOATIME to prevent the parent directory being updated, passing
> > > > in a NULL mnt for the purposes of iterating the directory dentry
> > > > was safe.  What apparmour is trying to do is look up the idmap
> > > > for the mount point to do one of its checks.
> > > > 
> > > > There are two ways of fixing this that I can think of.  One would
> > > > be exporting a function that lets me dig the vfsmount out of
> > > > s_mounts and use that (it's well hidden in the internals of
> > > > fs/mount.h, so I suspect this might not be very acceptable) or to
> > > > get mnt_idmap to return
> > 
> > Nope, please don't.
> > 
> > > > &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards
> > > > the latter, but I'm cc'ing fsdevel to see what others think.
> > 
> > A struct path with mount NULL and dentry != NULL is guaranteed to bit
> > us in the ass in other places. That's the bug.
> > 
> > > > 
> > > 
> > > 
> > > Al spotted the same issue based on a syzbot report [0]
> > > 
> > > [0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/
> > 
> > efivars as written only has a single global superblock and it doesn't
> > support idmapped mounts and I don't see why it ever would.
> 
> So that's not quite true: efivarfs currently supports uid and gid
> mapping as mount options, which certainly looks like they were designed
> to allow a second mount in a user directory.  I've no idea what the
> actual use case for this is, but if I go for a single superblock, any
> reconfigure with new uid/gid would become globally effective (change
> every current mount) because they're stored in the superblock
> information.
> 
> So what is the use case for this uid/gid parameter?  If no-one can
> remember and no-one actually uses it, perhaps the whole config path can
> be simplified by getting rid of the options?  Even if there is a use
> case, if it's single mount only then we can still go with a global
> superblock.

So efivarfs uses get_tree_single(). That means that only a single
superblock of the filesystem type efivarfs can ever exist on the system.

If efivars is mounted multiple times it will be the exact same
superblock that's used. IOW, mounting efivars multiple times is akin to
a bind-mount. It would be a bit ugly but it could be done by making sure
that any uid/gid changes are reflected. But see below.

> 
> > But since efivars does only ever have a single global superblock, one
> > possibility is to an internal superblock that always exits and is
> > resurfaced whenever userspace mounts efivarfs. That's essentially the
> > devtmpfs model.
> > 
> > Then you can stash:
> > 
> > static struct vfsmount *efivarfs_mnt;
> > 
> > globally and use that in efivarfs_pm_notify() to fill in struct path.
> 
> I didn't see devtmpfs when looking for examples, since it's hiding
> outside of the fs/ directory.  However, it does seem to be a bit legacy
> nasty as an example to copy.  However, I get the basics: we'd
> instantiate the mnt and superblock on init (stashing mnt in the sfi so
> the notifier gets it).  Then we can do the variable population on
> reconfigure, just in case an EFI system doesn't want to mount efivarfs
> to save memory.
> 
> I can code that up if I can get an answer to the uid/gid parameter
> question above.

I have some questions. efivarfs registers efivarfs_pm_notify even before
a superblock exists in efivarfs_init_fs_context(). That's called during
fd_context = fsopen("efivarfs") before a superblock even exists:

(1) Is it guaranteed that efivarfs_pm_notify() is only called once a
    superblock exists?

(2) Is it guaranteed that efivarfs_pm_notify() is only called when and
    while a mount for the superblock exists?

If the question to either one of those is "no" then the global
vfsmount hack will not help.

From reading efivarfs_pm_notify() it looks like the answer to (1) is
"yes" because you're dereferencing sfi->sb->s_root in
efivarfs_pm_notify().

But I'm not at all certain that (2) isn't a "no" and that
efivarfs_pm_notify() can be called before a mount exists. IOW, once
fsconfig(FSCONFIG_CMD_CREATE) is called the notifier seems ready and
registered but userspace isn't forced to call fsmount(fd_fs) at all.

They could just not to do it for whatever reason but the notifier should
already be able to run.

Another question is whether the superblock can be freed while
efivarfs_pm_notify() is running? I think that can't happen because
blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will block
in efivarfs_kill_sb() until all outstanding calls to
efivarfs_pm_notify() are finished?

If (2) isn't guranteed then efivarfs_pm_notify() needs to be rewritten
without relying on files because there's no guarantee that a mount
exists at all.

Re: apparmor NULL pointer dereference on resume [efivarfs]

[Christian Brauner]

On Tue, Mar 11, 2025 at 04:55:16PM +0100, Christian Brauner wrote:
> On Tue, Mar 11, 2025 at 09:01:36AM -0400, James Bottomley wrote:
> > On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
> > > On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:
> > > > (cc Al Viro)
> > > > 
> > > > On Mon, 10 Mar 2025 at 22:49, James Bottomley
> > > > <James.Bottomley@hansenpartnership.com> wrote:
> > [...]
> > > > > The problem comes down to the superblock functions not being able
> > > > > to get the struct vfsmount for the superblock (because it isn't
> > > > > even allocated until after they've all been called).  The
> > > > > assumption I was operating under was that provided I added
> > > > > O_NOATIME to prevent the parent directory being updated, passing
> > > > > in a NULL mnt for the purposes of iterating the directory dentry
> > > > > was safe.  What apparmour is trying to do is look up the idmap
> > > > > for the mount point to do one of its checks.
> > > > > 
> > > > > There are two ways of fixing this that I can think of.  One would
> > > > > be exporting a function that lets me dig the vfsmount out of
> > > > > s_mounts and use that (it's well hidden in the internals of
> > > > > fs/mount.h, so I suspect this might not be very acceptable) or to
> > > > > get mnt_idmap to return
> > > 
> > > Nope, please don't.
> > > 
> > > > > &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards
> > > > > the latter, but I'm cc'ing fsdevel to see what others think.
> > > 
> > > A struct path with mount NULL and dentry != NULL is guaranteed to bit
> > > us in the ass in other places. That's the bug.
> > > 
> > > > > 
> > > > 
> > > > 
> > > > Al spotted the same issue based on a syzbot report [0]
> > > > 
> > > > [0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/
> > > 
> > > efivars as written only has a single global superblock and it doesn't
> > > support idmapped mounts and I don't see why it ever would.
> > 
> > So that's not quite true: efivarfs currently supports uid and gid
> > mapping as mount options, which certainly looks like they were designed
> > to allow a second mount in a user directory.  I've no idea what the
> > actual use case for this is, but if I go for a single superblock, any
> > reconfigure with new uid/gid would become globally effective (change
> > every current mount) because they're stored in the superblock
> > information.
> > 
> > So what is the use case for this uid/gid parameter?  If no-one can
> > remember and no-one actually uses it, perhaps the whole config path can
> > be simplified by getting rid of the options?  Even if there is a use
> > case, if it's single mount only then we can still go with a global
> > superblock.
> 
> So efivarfs uses get_tree_single(). That means that only a single
> superblock of the filesystem type efivarfs can ever exist on the system.
> 
> If efivars is mounted multiple times it will be the exact same
> superblock that's used. IOW, mounting efivars multiple times is akin to
> a bind-mount. It would be a bit ugly but it could be done by making sure
> that any uid/gid changes are reflected. But see below.
> 
> > 
> > > But since efivars does only ever have a single global superblock, one
> > > possibility is to an internal superblock that always exits and is
> > > resurfaced whenever userspace mounts efivarfs. That's essentially the
> > > devtmpfs model.
> > > 
> > > Then you can stash:
> > > 
> > > static struct vfsmount *efivarfs_mnt;
> > > 
> > > globally and use that in efivarfs_pm_notify() to fill in struct path.
> > 
> > I didn't see devtmpfs when looking for examples, since it's hiding
> > outside of the fs/ directory.  However, it does seem to be a bit legacy
> > nasty as an example to copy.  However, I get the basics: we'd
> > instantiate the mnt and superblock on init (stashing mnt in the sfi so
> > the notifier gets it).  Then we can do the variable population on
> > reconfigure, just in case an EFI system doesn't want to mount efivarfs
> > to save memory.
> > 
> > I can code that up if I can get an answer to the uid/gid parameter
> > question above.
> 
> I have some questions. efivarfs registers efivarfs_pm_notify even before
> a superblock exists in efivarfs_init_fs_context(). That's called during
> fd_context = fsopen("efivarfs") before a superblock even exists:
> 
> (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
>     superblock exists?
> 
> (2) Is it guaranteed that efivarfs_pm_notify() is only called when and
>     while a mount for the superblock exists?
> 
> If the question to either one of those is "no" then the global
> vfsmount hack will not help.
> 
> >From reading efivarfs_pm_notify() it looks like the answer to (1) is
> "yes" because you're dereferencing sfi->sb->s_root in
> efivarfs_pm_notify().
> 
> But I'm not at all certain that (2) isn't a "no" and that
> efivarfs_pm_notify() can be called before a mount exists. IOW, once
> fsconfig(FSCONFIG_CMD_CREATE) is called the notifier seems ready and
> registered but userspace isn't forced to call fsmount(fd_fs) at all.
> 
> They could just not to do it for whatever reason but the notifier should
> already be able to run.
> 
> Another question is whether the superblock can be freed while
> efivarfs_pm_notify() is running? I think that can't happen because
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will block
> in efivarfs_kill_sb() until all outstanding calls to
> efivarfs_pm_notify() are finished?
> 
> If (2) isn't guranteed then efivarfs_pm_notify() needs to be rewritten
> without relying on files because there's no guarantee that a mount
> exists at all.

Btw, sorry but I'm going to be able to (guarantee) to respond Wed - Fri
because I'm on kid-watch-duty.

Re: apparmor NULL pointer dereference on resume [efivarfs]

[James Bottomley]

On Tue, 2025-03-11 at 16:55 +0100, Christian Brauner wrote:
> On Tue, Mar 11, 2025 at 09:01:36AM -0400, James Bottomley wrote:
> > On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
[...]
> > > efivars as written only has a single global superblock and it
> > > doesn't support idmapped mounts and I don't see why it ever
> > > would.
> > 
> > So that's not quite true: efivarfs currently supports uid and gid
> > mapping as mount options, which certainly looks like they were
> > designed to allow a second mount in a user directory.  I've no idea
> > what the actual use case for this is, but if I go for a single
> > superblock, any reconfigure with new uid/gid would become globally
> > effective (change every current mount) because they're stored in
> > the superblock information.
> > 
> > So what is the use case for this uid/gid parameter?  If no-one can
> > remember and no-one actually uses it, perhaps the whole config path
> > can be simplified by getting rid of the options?  Even if there is
> > a use case, if it's single mount only then we can still go with a
> > global superblock.
> 
> So efivarfs uses get_tree_single(). That means that only a single
> superblock of the filesystem type efivarfs can ever exist on the
> system.
> 
> If efivars is mounted multiple times it will be the exact same
> superblock that's used. IOW, mounting efivars multiple times is akin
> to a bind-mount. It would be a bit ugly but it could be done by
> making sure that any uid/gid changes are reflected. But see below.

OK, so it's a fair bet that either the uid/gid option is never used or
only used once globally.

> > > But since efivars does only ever have a single global superblock,
> > > one possibility is to an internal superblock that always exits
> > > and is resurfaced whenever userspace mounts efivarfs. That's
> > > essentially the devtmpfs model.
> > > 
> > > Then you can stash:
> > > 
> > > static struct vfsmount *efivarfs_mnt;
> > > 
> > > globally and use that in efivarfs_pm_notify() to fill in struct
> > > path.
> > 
> > I didn't see devtmpfs when looking for examples, since it's hiding
> > outside of the fs/ directory.  However, it does seem to be a bit
> > legacy nasty as an example to copy.  However, I get the basics:
> > we'd instantiate the mnt and superblock on init (stashing mnt in
> > the sfi so the notifier gets it).  Then we can do the variable
> > population on reconfigure, just in case an EFI system doesn't want
> > to mount efivarfs to save memory.
> > 
> > I can code that up if I can get an answer to the uid/gid parameter
> > question above.
> 
> I have some questions. efivarfs registers efivarfs_pm_notify even
> before a superblock exists in efivarfs_init_fs_context(). That's
> called during
> fd_context = fsopen("efivarfs") before a superblock even exists:
> 
> (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
>     superblock exists?

Yes (there was a fix that ensured it)

> 
> (2) Is it guaranteed that efivarfs_pm_notify() is only called when  
> and while a mount for the superblock exists?

That's the intention, but I thought the last unmount would trigger the
superblock kill.  However, I was thinking I'd need a new mechanism
based on reconfiguration to do the dentries only if the filesystem got
mounted, so I think I can cope with that.

> If the question to either one of those is "no" then the global
> vfsmount hack will not help.
> 
> From reading efivarfs_pm_notify() it looks like the answer to (1) is
> "yes" because you're dereferencing sfi->sb->s_root in
> efivarfs_pm_notify().
> 
> But I'm not at all certain that (2) isn't a "no" and that
> efivarfs_pm_notify() can be called before a mount exists. IOW, once
> fsconfig(FSCONFIG_CMD_CREATE) is called the notifier seems ready and
> registered but userspace isn't forced to call fsmount(fd_fs) at all.

I'd like to have efivarfs so that if no-one's actually mounted anything
then the reflection of the variables isn't taking up any space, so I
was planning a new mechanism for that anyway.

> They could just not to do it for whatever reason but the notifier
> should already be able to run.
> 
> Another question is whether the superblock can be freed while
> efivarfs_pm_notify() is running? I think that can't happen because
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will
> block in efivarfs_kill_sb() until all outstanding calls to
> efivarfs_pm_notify() are finished?

That's the way it's supposed to work, yes.  However, if we move to an
always persistent superblock and mnt, I was thinking there'd have to be
an indicator in the sfi about whether the variables were reflected or
not.

Regards,

James

> If (2) isn't guranteed then efivarfs_pm_notify() needs to be
> rewritten without relying on files because there's no guarantee that
> a mount exists at all.
> 

Re: apparmor NULL pointer dereference on resume [efivarfs]

[Al Viro]

On Tue, Mar 11, 2025 at 12:20:05PM -0400, James Bottomley wrote:

> That's the way it's supposed to work, yes.  However, if we move to an
> always persistent superblock and mnt, I was thinking there'd have to be
> an indicator in the sfi about whether the variables were reflected or
> not.

Just have a pointer to superblock set at ->get_tree() and cleared at
the very beginning of ->kill_sb(), then have notifier bugger off if
that thing's NULL or if atomic_inc_not_zero(sb->s_active) fails
(rcu_read_lock() is sufficient for memory safety of that).  And
do deactivate_super() when you are done.

That'll give you exclusion with umount.  As for the rest of that...
fuck it, just have kern_mount()/kern_unmount() inside that.
How hot do you expect that notifier chain to be?

Or screw playing with open/iterate_dir, but that'll need some thinking -
theoretically everything you need is already accessible, but direct
access to ->d_lock/->d_sib in there is almost certainly not the right
level of abstraction.  We already have similar bits and pieces
in autofs and ceph, and it's just a matter of figuring out a good
set of primitives.

Re: apparmor NULL pointer dereference on resume [efivarfs]

[James Bottomley]

On Tue, 2025-03-11 at 17:15 +0000, Al Viro wrote:
> On Tue, Mar 11, 2025 at 12:20:05PM -0400, James Bottomley wrote:
> 
> > That's the way it's supposed to work, yes.  However, if we move to
> > an always persistent superblock and mnt, I was thinking there'd
> > have to be an indicator in the sfi about whether the variables were
> > reflected or not.
> 
> Just have a pointer to superblock set at ->get_tree() and cleared at
> the very beginning of ->kill_sb(), then have notifier bugger off if
> that thing's NULL or if atomic_inc_not_zero(sb->s_active) fails
> (rcu_read_lock() is sufficient for memory safety of that).  And
> do deactivate_super() when you are done.

OK that works, thanks!

> That'll give you exclusion with umount.  As for the rest of that...
> fuck it, just have kern_mount()/kern_unmount() inside that.
> How hot do you expect that notifier chain to be?

The notifier chain itself is somewhat active, but the notifications we
care about occur once on resume from hibernate (so very few, if any).

> Or screw playing with open/iterate_dir, but that'll need some
> thinking -
> theoretically everything you need is already accessible, but direct
> access to ->d_lock/->d_sib in there is almost certainly not the right
> level of abstraction.  We already have similar bits and pieces
> in autofs and ceph, and it's just a matter of figuring out a good
> set of primitives.

I'll probably stick to getting it to work for now ... the refactoring
can come later.

Regards,

James

[RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[James Bottomley]

On Tue, 2025-03-11 at 09:01 -0400, James Bottomley wrote:
> On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
[...]
> > But since efivars does only ever have a single global superblock,
> > one possibility is to an internal superblock that always exits and
> > is resurfaced whenever userspace mounts efivarfs. That's
> > essentially the devtmpfs model.
> > 
> > Then you can stash:
> > 
> > static struct vfsmount *efivarfs_mnt;
> > 
> > globally and use that in efivarfs_pm_notify() to fill in struct
> > path.
> 
> I didn't see devtmpfs when looking for examples, since it's hiding
> outside of the fs/ directory.  However, it does seem to be a bit
> legacy nasty as an example to copy.  However, I get the basics: we'd
> instantiate the mnt and superblock on init (stashing mnt in the sfi
> so the notifier gets it).  Then we can do the variable population on
> reconfigure, just in case an EFI system doesn't want to mount
> efivarfs to save memory.
> 
> I can code that up if I can get an answer to the uid/gid parameter
> question above.

I coded up the naive implementation and it definitely works, but it
suffers from the problem that everything that pins in the module init
routine (like configfs) does in that once inserted the module can never
be removed.  Plus, for efivarfs, we would allocate all resources on
module insertion not on first mount.  The final problem we'd have is
that the uid/gid parameters for variable creation would be taken from
the kernel internal mount, so if they got specified on a user mount,
they'd be ignored (because the variable inodes are already created).

To answer some of your other questions:

> (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
>     superblock exists?

Yes, as you realized.

> (2) Is it guaranteed that efivarfs_pm_notify() is only called when
>     and while a mount for the superblock exists?

No, but the behaviour is correct because the notifier needs to update
the variable list and we create the variable list in
efivarfs_fill_super.  Now you can argue this is suboptimal because if
userspace didn't ever mount, we'd simply destroy it all again on last
put of the superblock so it's wasted effort, but its function is
correct.

> Another question is whether the superblock can be freed while
> efivarfs_pm_notify() is running? I think that can't happen because
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will
> block in efivarfs_kill_sb() until all outstanding calls to
> efivarfs_pm_notify() are finished?

That's right: a blocking notifier is called under the notifier list
rwsem.  It's taken read for calls but write for register/unregister, so
efivarfs_kill_sb would block in the unregister until the call chain was
executed.

Taking into account the module removal issue, the simplest way I found
to fix the issue was to call vfs_kern_mount() from the notifier to get
a struct vfsmount before opening the path.  We ensure it's gone by
calling mntput immediately after open, but, by that time, the open file
is pinning the vfsmnt if the open was successful.

If this looks OK to everyone I'll code it up as a fix which can be cc'd
to stable.

Regards,

James

---

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6eae8cf655c1..e2e6575b5abf 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -474,12 +474,14 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
 	return err;
 }
 
+static struct file_system_type efivarfs_type;
+
 static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
 			      void *ptr)
 {
 	struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info,
 						    pm_nb);
-	struct path path = { .mnt = NULL, .dentry = sfi->sb->s_root, };
+	struct path path;
 	struct efivarfs_ctx ectx = {
 		.ctx = {
 			.actor	= efivarfs_actor,
@@ -501,9 +503,17 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
 
 	pr_info("efivarfs: resyncing variable state\n");
 
-	/* O_NOATIME is required to prevent oops on NULL mnt */
+	path.dentry = sfi->sb->s_root;
+	path.mnt = vfs_kern_mount(&efivarfs_type, SB_KERNMOUNT,
+				  efivarfs_type.name, NULL);
+	if (IS_ERR(path.mnt)) {
+		pr_err("efivarfs: internal mount failed\n");
+		return PTR_ERR(path.mnt);
+	}
+
 	file = kernel_file_open(&path, O_RDONLY | O_DIRECTORY | O_NOATIME,
 				current_cred());
+	mntput(path.mnt);
 	if (IS_ERR(file))
 		return NOTIFY_DONE;
 

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[Christian Brauner]

On Fri, Mar 14, 2025 at 10:59:14AM -0400, James Bottomley wrote:
> On Tue, 2025-03-11 at 09:01 -0400, James Bottomley wrote:
> > On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
> [...]
> > > But since efivars does only ever have a single global superblock,
> > > one possibility is to an internal superblock that always exits and
> > > is resurfaced whenever userspace mounts efivarfs. That's
> > > essentially the devtmpfs model.
> > > 
> > > Then you can stash:
> > > 
> > > static struct vfsmount *efivarfs_mnt;
> > > 
> > > globally and use that in efivarfs_pm_notify() to fill in struct
> > > path.
> > 
> > I didn't see devtmpfs when looking for examples, since it's hiding
> > outside of the fs/ directory.  However, it does seem to be a bit
> > legacy nasty as an example to copy.  However, I get the basics: we'd
> > instantiate the mnt and superblock on init (stashing mnt in the sfi
> > so the notifier gets it).  Then we can do the variable population on
> > reconfigure, just in case an EFI system doesn't want to mount
> > efivarfs to save memory.
> > 
> > I can code that up if I can get an answer to the uid/gid parameter
> > question above.
> 
> I coded up the naive implementation and it definitely works, but it
> suffers from the problem that everything that pins in the module init
> routine (like configfs) does in that once inserted the module can never
> be removed.  Plus, for efivarfs, we would allocate all resources on
> module insertion not on first mount.  The final problem we'd have is
> that the uid/gid parameters for variable creation would be taken from
> the kernel internal mount, so if they got specified on a user mount,
> they'd be ignored (because the variable inodes are already created).
> 
> To answer some of your other questions:
> 
> > (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
> >     superblock exists?
> 
> Yes, as you realized.
> 
> > (2) Is it guaranteed that efivarfs_pm_notify() is only called when
> >     and while a mount for the superblock exists?
> 
> No, but the behaviour is correct because the notifier needs to update
> the variable list and we create the variable list in
> efivarfs_fill_super.  Now you can argue this is suboptimal because if
> userspace didn't ever mount, we'd simply destroy it all again on last
> put of the superblock so it's wasted effort, but its function is
> correct.
> 
> > Another question is whether the superblock can be freed while
> > efivarfs_pm_notify() is running? I think that can't happen because
> > blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will
> > block in efivarfs_kill_sb() until all outstanding calls to
> > efivarfs_pm_notify() are finished?
> 
> That's right: a blocking notifier is called under the notifier list
> rwsem.  It's taken read for calls but write for register/unregister, so
> efivarfs_kill_sb would block in the unregister until the call chain was
> executed.
> 
> Taking into account the module removal issue, the simplest way I found
> to fix the issue was to call vfs_kern_mount() from the notifier to get

Yeah, Al had already mentioned that. I initially had the same idea but
since I didn't know enough about the notifier block stuff I wasn't sure
whether there's some odd deadlock that could be caused by this.

> a struct vfsmount before opening the path.  We ensure it's gone by
> calling mntput immediately after open, but, by that time, the open file
> is pinning the vfsmnt if the open was successful.
> 
> If this looks OK to everyone I'll code it up as a fix which can be cc'd
> to stable.
> 
> Regards,
> 
> James
> 
> ---
> 
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 6eae8cf655c1..e2e6575b5abf 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -474,12 +474,14 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
>  	return err;
>  }
>  
> +static struct file_system_type efivarfs_type;
> +
>  static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
>  			      void *ptr)
>  {
>  	struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info,
>  						    pm_nb);
> -	struct path path = { .mnt = NULL, .dentry = sfi->sb->s_root, };
> +	struct path path;
>  	struct efivarfs_ctx ectx = {
>  		.ctx = {
>  			.actor	= efivarfs_actor,
> @@ -501,9 +503,17 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
>  
>  	pr_info("efivarfs: resyncing variable state\n");
>  
> -	/* O_NOATIME is required to prevent oops on NULL mnt */
> +	path.dentry = sfi->sb->s_root;
> +	path.mnt = vfs_kern_mount(&efivarfs_type, SB_KERNMOUNT,
> +				  efivarfs_type.name, NULL);
> +	if (IS_ERR(path.mnt)) {
> +		pr_err("efivarfs: internal mount failed\n");
> +		return PTR_ERR(path.mnt);
> +	}

I see some issues with this. A umount by another task could already hit:

sb->kill_sb == efivarfs_kill_super()

which means the superblock is already marked as dying.

By calling vfs_kern_mount() unconditionally you end up calling
vfs_get_tree() and then get_tree_single() again. That would mean
efivarfs_pm_notify() now waits for the old superblock to be dead.

But the old superblock waits in efivarfs_kill_sb() for
efivarfs_pm_notify() to finish before actually killing the old
superblock.

So this would deadlock.

So you need to make sure that the superbock a) isn't dead and b) doesn't
go away behind your back:

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6eae8cf655c1..6a4f95c27697 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -474,6 +474,8 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
        return err;
 }

+static struct file_system_type efivarfs_type;
+
 static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
                              void *ptr)
 {
@@ -499,6 +501,31 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
        if (rescan_done)
                return NOTIFY_DONE;

+       /*
+        * Ensure that efivarfs is still alive and cannot go away behind
+        * our back.
+        */
+       if (!atomic_inc_not_zero(&sfi->sb->s_active))
+               return NOTIFY_DONE;
+
+       path.mnt = vfs_kern_mount(&efivarfs_type, SB_KERNMOUNT,
+                                 efivarfs_type.name, NULL);

Since efivars uses a single global superblock and we know that sfi->sb
is still alive (After all we've just pinned it above.) vfs_kern_mount()
will reuse the same superblock.

There's two cases to consider:

(1) vfs_kern_mount() was successful. In this case path->mnt will hold an
    active superblock reference that will be released asynchronously via
    __fput(). That is safe and correct.

(2) vfs_kern_mount() fails. That's an issue because you need to call
    deactivate_super() which will have a similar deadlock problem.

    If efivarfs_pm_notify() now holds the last reference to the
    superblock then deactivate_super() super will put that last
    reference and call efivarfs_kill_super() which in turn will wait for
    efivarfs_pm_notify() to finish. => deadlock

So in the error case you need to offload the call to deactivate_super()
to a workqueue.

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6eae8cf655c1..288c1dd8622b 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -474,6 +474,8 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
        return err;
 }

+static struct file_system_type efivarfs_type;
+
 static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
                              void *ptr)
 {
@@ -499,6 +501,39 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
        if (rescan_done)
                return NOTIFY_DONE;

+       /*
+        * Ensure that efivarfs is still alive and cannot go away behind
+        * our back.
+        */
+       if (!atomic_inc_not_zero(&sfi->sb->s_active))
+               return NOTIFY_DONE;
+
+       path.mnt = vfs_kern_mount(&efivarfs_type, SB_KERNMOUNT,
+                                 efivarfs_type.name, NULL);
+       /*
+        * Since efivars uses a single global superblock and we know
+        * that sfi->sb is still alive (After all we've just pinned it
+        * above.) vfs_kern_mount() will reuse the same superblock.
+        *
+        * If vfs_kern_mount() was successful path->mnt will hold an
+        * active superblock reference that will be released
+        * asynchronously via __fput().
+        *
+        * If vfs_kern_mount() fails we might be the ones to hold the
+        * last reference now so we need to call deactivate_super(). But
+        * we need to ensure that this is done asynchronously so
+        * efivarfs_kill_super() doesn't deadlock by waiting on
+        * efivarfs_pm_notify() to finish.
+        */
+       if (IS_ERR(path.mnt)) {
+
+               /* TODO: offload to workqueue so that we don't deadlock. */
+               deactivate_super(sfi->sb);
+               pr_err("efivarfs: internal mount failed\n");
+               return PTR_ERR(path.mnt);
+       }
+       atomic_dec(&sfi->sb->s_active);
+
        pr_info("efivarfs: resyncing variable state\n");

        /* O_NOATIME is required to prevent oops on NULL mnt */

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[James Bottomley]

On Sat, 2025-03-15 at 11:04 +0100, Christian Brauner wrote:
[...]
> Since efivars uses a single global superblock and we know that sfi-
> >sb is still alive (After all we've just pinned it above.)
> vfs_kern_mount() will reuse the same superblock.
> 
> There's two cases to consider:
> 
> (1) vfs_kern_mount() was successful. In this case path->mnt will hold
> an active superblock reference that will be released asynchronously
> via __fput(). That is safe and correct.
> 
> (2) vfs_kern_mount() fails. That's an issue because you need to call
> deactivate_super() which will have a similar deadlock problem.
> If efivarfs_pm_notify() now holds the last reference to the
> superblock then deactivate_super() super will put that last
> reference and call efivarfs_kill_super() which in turn will wait for
> efivarfs_pm_notify() to finish. => deadlock
> 
> So in the error case you need to offload the call to
> deactivate_super() to a workqueue.

OK, got that (although it did make my head explode a bit ... this is
certainly subtle stuff).  To do the delayed work for the
deactivate_super(), I hijacked the superblock destroy_work structure
which I think is safe because by the time the work structure is
executed, we own it and so it can be reused for the actual destroy_work
in deactivate_super().

However, there's another problem: the mntput after kernel_file_open may
synchronously call cleanup_mnt() (and thus deactivate_super()) if the
open fails because it's marked MNT_INTERNAL, which is caused by
SB_KERNMOUNT.  I fixed this just by not passing the SB_KERNMOUNT flag,
which feels a bit hacky.

I've put together everything at the bottom, however, I can't test the
error legs of this because trying to trigger and unmount and hybernate
at exactly the right point is pretty much impossible.  The rest seems
to work as advertised, although I would like a tested-by from the
apparmour people because I do run apparmour in my debian test rig but
don't see the problem.

Regards,

James

---

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 6eae8cf655c1..2d826e98066b 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -474,12 +474,25 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
 	return err;
 }
 
+static void efivarfs_deactivate_super_work(struct work_struct *work)
+{
+	struct super_block *s = container_of(work, struct super_block,
+					     destroy_work);
+	/*
+	 * note: here s->destroy_work is free for reuse (which
+	 * will happen in deactivate_super)
+	 */
+	deactivate_super(s);
+}
+
+static struct file_system_type efivarfs_type;
+
 static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
 			      void *ptr)
 {
 	struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info,
 						    pm_nb);
-	struct path path = { .mnt = NULL, .dentry = sfi->sb->s_root, };
+	struct path path;
 	struct efivarfs_ctx ectx = {
 		.ctx = {
 			.actor	= efivarfs_actor,
@@ -487,6 +500,7 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
 		.sb = sfi->sb,
 	};
 	struct file *file;
+	struct super_block *s = sfi->sb;
 	static bool rescan_done = true;
 
 	if (action == PM_HIBERNATION_PREPARE) {
@@ -499,11 +513,39 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
 	if (rescan_done)
 		return NOTIFY_DONE;
 
+	/* ensure single superblock is alive and pin it */
+	if (!atomic_inc_not_zero(&s->s_active))
+		return NOTIFY_DONE;
+
 	pr_info("efivarfs: resyncing variable state\n");
 
-	/* O_NOATIME is required to prevent oops on NULL mnt */
+	path.dentry = sfi->sb->s_root;
+
+	/* do not add SB_KERNMOUNT which causes MNT_INTERNAL, see below */
+	path.mnt = vfs_kern_mount(&efivarfs_type, 0,
+				  efivarfs_type.name, NULL);
+	if (IS_ERR(path.mnt)) {
+		pr_err("efivarfs: internal mount failed\n");
+		/*
+		 * We may be the last pinner of the superblock but
+		 * calling efivarfs_kill_sb from within the notifier
+		 * here would deadlock trying to unregister it
+		 */
+		INIT_WORK(&s->destroy_work, efivarfs_deactivate_super_work);
+		schedule_work(&s->destroy_work);
+		return PTR_ERR(path.mnt);
+	}
+
+	/* path.mnt now has pin on superblock, so this must be above one */
+	atomic_dec(&s->s_active);
+
 	file = kernel_file_open(&path, O_RDONLY | O_DIRECTORY | O_NOATIME,
 				current_cred());
+	/*
+	 * safe even if last put because no MNT_INTERNAL means this
+	 * will do delayed deactivate_super and not deadlock
+	 */
+	mntput(path.mnt);
 	if (IS_ERR(file))
 		return NOTIFY_DONE;
 

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[Christian Brauner]

On Sat, Mar 15, 2025 at 02:41:43PM -0400, James Bottomley wrote:
> On Sat, 2025-03-15 at 11:04 +0100, Christian Brauner wrote:
> [...]
> > Since efivars uses a single global superblock and we know that sfi-
> > >sb is still alive (After all we've just pinned it above.)
> > vfs_kern_mount() will reuse the same superblock.
> > 
> > There's two cases to consider:
> > 
> > (1) vfs_kern_mount() was successful. In this case path->mnt will hold
> > an active superblock reference that will be released asynchronously
> > via __fput(). That is safe and correct.
> > 
> > (2) vfs_kern_mount() fails. That's an issue because you need to call
> > deactivate_super() which will have a similar deadlock problem.
> > If efivarfs_pm_notify() now holds the last reference to the
> > superblock then deactivate_super() super will put that last
> > reference and call efivarfs_kill_super() which in turn will wait for
> > efivarfs_pm_notify() to finish. => deadlock
> > 
> > So in the error case you need to offload the call to
> > deactivate_super() to a workqueue.
> 
> OK, got that (although it did make my head explode a bit ... this is
> certainly subtle stuff).  To do the delayed work for the
> deactivate_super(), I hijacked the superblock destroy_work structure
> which I think is safe because by the time the work structure is
> executed, we own it and so it can be reused for the actual destroy_work
> in deactivate_super().
> 
> However, there's another problem: the mntput after kernel_file_open may
> synchronously call cleanup_mnt() (and thus deactivate_super()) if the
> open fails because it's marked MNT_INTERNAL, which is caused by
> SB_KERNMOUNT.  I fixed this just by not passing the SB_KERNMOUNT flag,
> which feels a bit hacky.

It actually isn't. We know that vfs_kern_mount() will always resurface
the single superblock that's exposed to userspace because we've just
taken a reference to it earlier in efivarfs_pm_notify(). So that
SB_KERNMOUNT flag is ignored because no new superblock is allocated. It
would only matter if we'd end up allocating a new superblock which we
never do.

And if we did it would be a bug because the superblock we allocate could
be reused at any time if a userspace task mounts efivarfs before
efivarfs_pm_notify() has destroyed it (or the respective workqueue).
But that superblock would then have SB_KERNMOUNT for something that's
not supposed to be one.

And whether or not that helper mount has MNT_INTERNAL is immaterial to
what you're doing here afaict.

So not passing the SB_KERNMOUNT flag is the right thing (see devtmpfs as
well). You could slap a comment in here explaining that we never
allocate a new superblock so it's clear to people not familiar with this
particular code.

> 
> I've put together everything at the bottom, however, I can't test the
> error legs of this because trying to trigger and unmount and hybernate
> at exactly the right point is pretty much impossible.  The rest seems
> to work as advertised, although I would like a tested-by from the
> apparmour people because I do run apparmour in my debian test rig but
> don't see the problem.
> 
> Regards,
> 
> James
> 
> ---
> 
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index 6eae8cf655c1..2d826e98066b 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -474,12 +474,25 @@ static int efivarfs_check_missing(efi_char16_t *name16, efi_guid_t vendor,
>  	return err;
>  }
>  
> +static void efivarfs_deactivate_super_work(struct work_struct *work)
> +{
> +	struct super_block *s = container_of(work, struct super_block,
> +					     destroy_work);
> +	/*
> +	 * note: here s->destroy_work is free for reuse (which
> +	 * will happen in deactivate_super)
> +	 */
> +	deactivate_super(s);
> +}
> +
> +static struct file_system_type efivarfs_type;
> +
>  static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
>  			      void *ptr)
>  {
>  	struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info,
>  						    pm_nb);
> -	struct path path = { .mnt = NULL, .dentry = sfi->sb->s_root, };
> +	struct path path;
>  	struct efivarfs_ctx ectx = {
>  		.ctx = {
>  			.actor	= efivarfs_actor,
> @@ -487,6 +500,7 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
>  		.sb = sfi->sb,
>  	};
>  	struct file *file;
> +	struct super_block *s = sfi->sb;
>  	static bool rescan_done = true;
>  
>  	if (action == PM_HIBERNATION_PREPARE) {
> @@ -499,11 +513,39 @@ static int efivarfs_pm_notify(struct notifier_block *nb, unsigned long action,
>  	if (rescan_done)
>  		return NOTIFY_DONE;
>  
> +	/* ensure single superblock is alive and pin it */
> +	if (!atomic_inc_not_zero(&s->s_active))
> +		return NOTIFY_DONE;
> +
>  	pr_info("efivarfs: resyncing variable state\n");
>  
> -	/* O_NOATIME is required to prevent oops on NULL mnt */
> +	path.dentry = sfi->sb->s_root;
> +
> +	/* do not add SB_KERNMOUNT which causes MNT_INTERNAL, see below */
> +	path.mnt = vfs_kern_mount(&efivarfs_type, 0,
> +				  efivarfs_type.name, NULL);
> +	if (IS_ERR(path.mnt)) {
> +		pr_err("efivarfs: internal mount failed\n");
> +		/*
> +		 * We may be the last pinner of the superblock but
> +		 * calling efivarfs_kill_sb from within the notifier
> +		 * here would deadlock trying to unregister it
> +		 */
> +		INIT_WORK(&s->destroy_work, efivarfs_deactivate_super_work);
> +		schedule_work(&s->destroy_work);
> +		return PTR_ERR(path.mnt);
> +	}
> +
> +	/* path.mnt now has pin on superblock, so this must be above one */
> +	atomic_dec(&s->s_active);
> +
>  	file = kernel_file_open(&path, O_RDONLY | O_DIRECTORY | O_NOATIME,
>  				current_cred());
> +	/*
> +	 * safe even if last put because no MNT_INTERNAL means this
> +	 * will do delayed deactivate_super and not deadlock
> +	 */
> +	mntput(path.mnt);
>  	if (IS_ERR(file))
>  		return NOTIFY_DONE;
>  
> 
> 

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[James Bottomley]

On Sun, 2025-03-16 at 07:46 +0100, Christian Brauner wrote:
> On Sat, Mar 15, 2025 at 02:41:43PM -0400, James Bottomley wrote:
[...]
> > However, there's another problem: the mntput after kernel_file_open
> > may synchronously call cleanup_mnt() (and thus deactivate_super())
> > if the open fails because it's marked MNT_INTERNAL, which is caused
> > by SB_KERNMOUNT.  I fixed this just by not passing the SB_KERNMOUNT
> > flag, which feels a bit hacky.
> 
> It actually isn't. We know that vfs_kern_mount() will always
> resurface the single superblock that's exposed to userspace because
> we've just taken a reference to it earlier in efivarfs_pm_notify().
> So that SB_KERNMOUNT flag is ignored because no new superblock is
> allocated. It would only matter if we'd end up allocating a new
> superblock which we never do.

I agree with the above: fc->sb_flags never propagates to the existing
superblock.  However, nothing propagates the superblock flags back to
fc->sb_flags either.  The check in vfs_create_mount() is on fc-
>sb_flags.  Since the code is a bit hard to follow I added a printk on
the path.mnt flags and sure enough it comes back with MNT_INTERNAL when
SB_KERNMOUNT is set.

> And if we did it would be a bug because the superblock we allocate
> could be reused at any time if a userspace task mounts efivarfs
> before efivarfs_pm_notify() has destroyed it (or the respective
> workqueue). But that superblock would then have SB_KERNMOUNT for
> something that's not supposed to be one.

True, but the flags don't propagate to the superblock, so no bug.

> And whether or not that helper mount has MNT_INTERNAL is immaterial
> to what you're doing here afaict.

I think the problem is the call chain mntput() -> mntput_no_expire()
which directly calls cleanup_mnt() -> deactivate_super() if that flag
is set.  Though I don't see that kernel_file_open() could ever fail
except for some catastrophic reason like out of memory, so perhaps it
isn't worth quibbling about.

> So not passing the SB_KERNMOUNT flag is the right thing (see devtmpfs
> as well). You could slap a comment in here explaining that we never
> allocate a new superblock so it's clear to people not familiar with
> this particular code.

OK, so you agree that the code as written looks correct? Even if we
don't necessarily quite agree on why.

Regards,

James

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[Ard Biesheuvel]

On Sun, 16 Mar 2025 at 15:26, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Sun, 2025-03-16 at 07:46 +0100, Christian Brauner wrote:
> > On Sat, Mar 15, 2025 at 02:41:43PM -0400, James Bottomley wrote:
> [...]
> > > However, there's another problem: the mntput after kernel_file_open
> > > may synchronously call cleanup_mnt() (and thus deactivate_super())
> > > if the open fails because it's marked MNT_INTERNAL, which is caused
> > > by SB_KERNMOUNT.  I fixed this just by not passing the SB_KERNMOUNT
> > > flag, which feels a bit hacky.
> >
> > It actually isn't. We know that vfs_kern_mount() will always
> > resurface the single superblock that's exposed to userspace because
> > we've just taken a reference to it earlier in efivarfs_pm_notify().
> > So that SB_KERNMOUNT flag is ignored because no new superblock is
> > allocated. It would only matter if we'd end up allocating a new
> > superblock which we never do.
>
> I agree with the above: fc->sb_flags never propagates to the existing
> superblock.  However, nothing propagates the superblock flags back to
> fc->sb_flags either.  The check in vfs_create_mount() is on fc-
> >sb_flags.  Since the code is a bit hard to follow I added a printk on
> the path.mnt flags and sure enough it comes back with MNT_INTERNAL when
> SB_KERNMOUNT is set.
>
> > And if we did it would be a bug because the superblock we allocate
> > could be reused at any time if a userspace task mounts efivarfs
> > before efivarfs_pm_notify() has destroyed it (or the respective
> > workqueue). But that superblock would then have SB_KERNMOUNT for
> > something that's not supposed to be one.
>
> True, but the flags don't propagate to the superblock, so no bug.
>
> > And whether or not that helper mount has MNT_INTERNAL is immaterial
> > to what you're doing here afaict.
>
> I think the problem is the call chain mntput() -> mntput_no_expire()
> which directly calls cleanup_mnt() -> deactivate_super() if that flag
> is set.  Though I don't see that kernel_file_open() could ever fail
> except for some catastrophic reason like out of memory, so perhaps it
> isn't worth quibbling about.
>
> > So not passing the SB_KERNMOUNT flag is the right thing (see devtmpfs
> > as well). You could slap a comment in here explaining that we never
> > allocate a new superblock so it's clear to people not familiar with
> > this particular code.
>
> OK, so you agree that the code as written looks correct? Even if we
> don't necessarily quite agree on why.
>

Thanks for making progress on this. It would be nice if we could fix
this before the v6.14 release, given that the code in question was
introduced this cycle.

And there's another suggestion from Al, to use inode_lock_nested() to
work around the lockdep warning. I can take care of that one, unless
you prefer to do it yourself?

Re: [RFC 1/1] fix NULL mnt [was Re: apparmor NULL pointer dereference on resume [efivarfs]]

[Christian Brauner]

On Sun, Mar 16, 2025 at 10:26:12AM -0400, James Bottomley wrote:
> On Sun, 2025-03-16 at 07:46 +0100, Christian Brauner wrote:
> > On Sat, Mar 15, 2025 at 02:41:43PM -0400, James Bottomley wrote:
> [...]
> > > However, there's another problem: the mntput after kernel_file_open
> > > may synchronously call cleanup_mnt() (and thus deactivate_super())
> > > if the open fails because it's marked MNT_INTERNAL, which is caused
> > > by SB_KERNMOUNT.  I fixed this just by not passing the SB_KERNMOUNT
> > > flag, which feels a bit hacky.
> > 
> > It actually isn't. We know that vfs_kern_mount() will always
> > resurface the single superblock that's exposed to userspace because
> > we've just taken a reference to it earlier in efivarfs_pm_notify().
> > So that SB_KERNMOUNT flag is ignored because no new superblock is
> > allocated. It would only matter if we'd end up allocating a new
> > superblock which we never do.
> 
> I agree with the above: fc->sb_flags never propagates to the existing
> superblock.  However, nothing propagates the superblock flags back to
> fc->sb_flags either.  The check in vfs_create_mount() is on fc-
> >sb_flags.  Since the code is a bit hard to follow I added a printk on
> the path.mnt flags and sure enough it comes back with MNT_INTERNAL when
> SB_KERNMOUNT is set.
> 
> > And if we did it would be a bug because the superblock we allocate
> > could be reused at any time if a userspace task mounts efivarfs
> > before efivarfs_pm_notify() has destroyed it (or the respective
> > workqueue). But that superblock would then have SB_KERNMOUNT for
> > something that's not supposed to be one.
> 
> True, but the flags don't propagate to the superblock, so no bug.

SB_KERNMOUNT does propagate to the superblock if it is newly allocated
via sget_fc(): alloc_super(fc->fs_type, fc->sb_flags, user_ns);

But you misunderstood. "If we did it" means "If efivarfs_pm_notify()
somehow were to allocate a new superblock (which it doesn't) then having
SB_KERNMOUNT raised on the newly allocated superblock would be bug
because the superblock could be reused by userspace mounting efivars.

So removing it is the correct thing in either case. It's just confusing
to anyone maintaining that code thinking that it'd be possible for a
superblock to resurface with SB_KERNMOUNT.

> 
> > And whether or not that helper mount has MNT_INTERNAL is immaterial
> > to what you're doing here afaict.
> 
> I think the problem is the call chain mntput() -> mntput_no_expire()
> which directly calls cleanup_mnt() -> deactivate_super() if that flag
> is set.  Though I don't see that kernel_file_open() could ever fail
> except for some catastrophic reason like out of memory, so perhaps it
> isn't worth quibbling about.

Not what I'm saying. Not having MNT_INTERNAL is paramount to not
deadlocking but by not having it you're not losing anything.

> 
> > So not passing the SB_KERNMOUNT flag is the right thing (see devtmpfs
> > as well). You could slap a comment in here explaining that we never
> > allocate a new superblock so it's clear to people not familiar with
> > this particular code.
> 
> OK, so you agree that the code as written looks correct? Even if we
> don't necessarily quite agree on why.

We agree but you just misunderstood. :)