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:

quoted

On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:

quoted

On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:

quoted

(cc Al Viro)

On Mon, 10 Mar 2025 at 22:49, James Bottomley
[off-list ref] wrote:

[...]

quoted

quoted

quoted

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.

quoted

quoted

&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.

quoted

quoted

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

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

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.

quoted

quoted

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.

quoted

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.