On Tue, Aug 26, 2025 at 10:27:56AM +0200, Christian Brauner wrote:
> > Declaring it above, initializing with NULL and reassigning here?
> > That's actually just as wrong, if not more so - any assignment added
>
> I disagree. I do very much prefer having cleanups at the top of the
> function or e.g.,:
>
> if (foo) {
> struct vfsmount *mnt __free(mntput) = vfs_create_mount(fc);
> }
>
> Because it is really easy to figure out visually. But just doing it
> somewhere in the middle is just confusing.
So basically you treat __free() simply as a syntax sugar for "call this
on exits from this block", rather than an approximation for "here's an
auto object we've created, this should be called to destroy it at the
end of its scope/lifetime"?
IMO it's a bad practice - it makes life much harder when you are tracing
callchains, etc.
FWIW, I wonder if the things would be cleaner if we did security_sb_kern_mount()
and mount_too_revealing() *after* unlocking the superblock and getting a vfsmount.
The latter definitely doesn't give a damn about superblock being locked and
AFAICS neither does the only in-tree instance of ->sb_kern_mount().
That way we have the real initialization reasonably close to __free() and
control flow is easier to follow...
Folks, how about something like the delta below (on top of the posted queue)?
diff --git a/fs/namespace.c b/fs/namespace.c
index 63b74d7384fd..191e7f776de5 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3689,24 +3689,22 @@ static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags
static int do_new_mount_fc(struct fs_context *fc, const struct path *mountpoint,
unsigned int mnt_flags)
{
+ struct vfsmount *mnt __free(mntput) = NULL;
struct super_block *sb = fc->root->d_sb;
int error;
- error = security_sb_kern_mount(sb);
- if (!error && mount_too_revealing(sb, &mnt_flags))
- error = -EPERM;
-
- if (unlikely(error)) {
- fc_drop_locked(fc);
- return error;
- }
-
up_write(&sb->s_umount);
-
- struct vfsmount *mnt __free(mntput) = vfs_create_mount(fc);
+ mnt = vfs_create_mount(fc);
if (IS_ERR(mnt))
return PTR_ERR(mnt);
+ error = security_sb_kern_mount(sb);
+ if (unlikely(error))
+ return error;
+
+ if (mount_too_revealing(sb, &mnt_flags))
+ return -EPERM;
+
mnt_warn_timestamp_expiry(mountpoint, mnt);
LOCK_MOUNT(mp, mountpoint);
