On Tue, 18 Mar 2025 at 04:37, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Mar 17, 2025 at 11:06:01PM -0400, James Bottomley wrote:
>
> > + /* 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 a single superblock could
> > + * expose to userspace and which also causes MNT_INTERNAL, see
> > + * below
> > + */
> > + path.mnt = vfs_kern_mount(&efivarfs_type, 0,
> > + efivarfs_type.name, NULL);
>
> Umm... That's probably safe, but not as a long-term solution -
> it's too intimately dependent upon fs/super.c internals.
> The reasons why you can't run into ->s_umount deadlock here
> are non-trivial...
Thanks - I'll incorporate this observation in the commit log, if you don't mind.
To me, it seems rather counter-intuitive that we need a second mount
in order to be able to implement this. Synchronizing the efivarfs
contents with the backing store after an event that may have modified
the latter is only needed when it is mounted to begin with, and as a
VFS non-expert, I struggle to understand why it is a) ok and b)
preferred to create a new mount to pass to kernel_file_open(). Could
we add a paragraph to the commit log that explains this?
But if the VFS experts agree that this is a reasonable band-aid for
the time being, I will take the changes themselves as they are. I
intend to send this out asap as a fix for the v6.14 release.
