Re: [PATCH] efivarfs: fix NULL dereference on resume

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-03-18 at 07:49 +0000, Al Viro wrote:
> On Tue, Mar 18, 2025 at 08:04:59AM +0100, Ard Biesheuvel wrote:
> 
> > 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?
> 
> I'm not at all convinced that iterate_dir() is the right thing to use
> there, but *IF* we go that way, yes, we need a reference to struct
> mount.  We are not going to introduce a very special kind of struct
> file, along with the arseloads of checking for that crap all over the
> place - not for the sake of one weird case in one weird filesystem.
> 
> file->f_path is a valid struct path, which means that ->mnt->mnt_sb
> == ->dentry->d_sb and refcount of ->mnt is positive as long as struct
> file exists.
> 
> Keeping a persistent internal struct mount is, of course, possible,
> but it will make the damn thing impossible to rmmod, etc. - it will
> remain in place until the reboot.

Right, that's the configfs problem and we do want the module to be
removable (and not occupying memory) when it's not mounted in
userspace.

> It might be possible to put together something like "grab a reference
> to superblock and allocate a temporary struct mount refering to it"
> (which is what that vfs_kern_mount() boils down to).  But I would
> very much prefer to have it go over the list of children of ->s_root
> manually, instead of playing silly buggers with iterate_dir().

I did think of that ... how hard, after all can it be just to traverse
all the single level children (we never do subdirectories in efivarfs).
However, doing that seemed to involve replicating the whole of
libfs.c:dcache_readdir() and scan_positives() plus the cursor
allocation it uses is currently marked as an internal interface.  If
there's a simpler way of doing it, I'm all ears, but the code in
libfs.c is subtle and complex and should probably stay there.

So I think the only route to this would be to extract most of the guts
of dcache_readdir() into a helper function that takes a callback where
it currently does dir_emit() and expose that for filesystem use.  Is
that a route you'd like me to investigate?

> And yes, it would require exclusion with dissolving dentry tree on
> umount, for obvious reasons.  Which might be done with ->s_active
> or simply by unregistering that notifier chain as the very first step
> in ->kill_sb() there.

I can move it above kill_litter_super(), sure.  However, the check on
s_active at the top of efivarfs_pm_notify() should ensure nothing
dissolves the tree until we're finished.

Regards,

James