Re: [PATCH 25/52] do_new_mount_rc(): use __free() to deal with dropping mnt on failure

[Christian Brauner <brauner@xxxxxxxxxx>]

On Mon, Aug 25, 2025 at 05:09:39PM +0100, Al Viro wrote:
> On Mon, Aug 25, 2025 at 03:29:33PM +0200, Christian Brauner wrote:
> > > -	mnt = vfs_create_mount(fc);
> > > +	struct vfsmount *mnt __free(mntput) = vfs_create_mount(fc);
> > 
> > Ugh, can we please not start declaring variables in the middle of a
> > scope.
> 
> Seeing that it *is* the beginning of its scope, what do you suggest?

What? Did I miss earlier or later changes because:

static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
			   unsigned int mnt_flags)
{
	struct vfsmount *mnt;
	struct pinned_mountpoint mp = {};
	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);

	mnt = vfs_create_mount(fc);
	if (IS_ERR(mnt))
		return PTR_ERR(mnt);

How does up_write() create a new scope?

	mnt_warn_timestamp_expiry(mountpoint, mnt);

	error = lock_mount(mountpoint, &mp);
	if (!error) {
		error = do_add_mount(real_mount(mnt), mp.mp,
				     mountpoint, mnt_flags);
		unlock_mount(&mp);
	}
	if (error < 0)
		mntput(mnt);
	return error;
}

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

static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
			   unsigned int mnt_flags)
{
	struct pinned_mountpoint mp = {};
	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);
	if (is_err(mnt))
		return ptr_err(mnt);