Re: [PATCH 05/19] fs: assign an anon dev_t in common code

From: Al Viro <viro@zeniv.linux.org.uk>
To: Christoph Hellwig <hch@lst.de>
Cc: Christian Brauner <brauner@kernel.org>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	Fenghua Yu <fenghua.yu@intel.com>,
	Reinette Chatre <reinette.chatre@intel.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Tejun Heo <tj@kernel.org>,
	Trond Myklebust <trond.myklebust@hammerspace.com>,
	Anna Schumaker <anna@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Damien Le Moal <dlemoal@kernel.org>,
	Naohiro Aota <naohiro.aota@wdc.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-s390@vger.kernel.org, linux-rdma@vger.kernel.org,
	linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org,
	cgroups@vger.kernel.org
Subject: Re: [PATCH 05/19] fs: assign an anon dev_t in common code
Date: Thu, 14 Sep 2023 01:34:24 +0100	[thread overview]
Message-ID: <20230914003424.GD800259@ZenIV> (raw)
In-Reply-To: <20230913111013.77623-6-hch@lst.de>

On Wed, Sep 13, 2023 at 08:09:59AM -0300, Christoph Hellwig wrote:

> diff --git a/fs/super.c b/fs/super.c
> index bbe55f0651cca4..5c685b4944c2d6 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -787,7 +787,7 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	struct super_block *s = NULL;
>  	struct super_block *old;
>  	struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
> -	int err;
> +	int err = 0;
>  
>  retry:
>  	spin_lock(&sb_lock);
> @@ -806,14 +806,26 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	}
>  
>  	s->s_fs_info = fc->s_fs_info;
> -	err = set(s, fc);
> -	if (err) {
> -		s->s_fs_info = NULL;
> -		spin_unlock(&sb_lock);
> -		destroy_unused_super(s);
> -		return ERR_PTR(err);
> +	if (set) {
> +		err = set(s, fc);
> +		if (err) {
> +			s->s_fs_info = NULL;

Pointless (as the original had been); destroy_unused_super() doesn't
even look at ->s_fs_info.

> +			goto unlock_and_destroy;
> +		}
>  	}
>  	fc->s_fs_info = NULL;

Here we are transferring the ownership from fc to superblock; it used to sit
right next to insertion into lists and all failure exits past that point must
go through deactivate_locked_super(), so ->kill_sb() would be called on those
and it would take care of s->s_fs_info.  However, your variant has that
ownership transfer done at the point before get_anon_bdev(), and that got
you a new failure exit where you are still calling destroy_unused_super():

> +	if (!s->s_dev) {
> +		/*
> +		 * If the file system didn't set a s_dev (which is usually only
> +		 * done for block based file systems), set an anonymous dev_t
> +		 * here now so that we always have a valid ->s_dev.
> +		 */
> +		err = get_anon_bdev(&s->s_dev);
> +		if (err)
> +			goto unlock_and_destroy;

This.  And that's a leak - fc has no reference left in it, and your
unlock_and_destroy won't even look at what's in ->s_fs_info, let alone know
what to do with it.

IOW, clearing fc->s_fs_info should've been done after that chunk.

And looking at the change in sget(),

> +	if (set) {
> +		err = set(s, data);
> +		if (err)
> +			goto unlock_and_destroy;
>  	}
> +
> +	if (!s->s_dev) {
> +		err = get_anon_bdev(&s->s_dev);
> +		if (err)
> +			goto unlock_and_destroy;
> +	}

I'd rather expressed it (both there and in sget_fc() as well) as
	if (set)
		err = set(s, data);
	if (!err && !s->s_dev)
		err = get_anon_bdev(&s->s_dev);
	if (err)
		goto unlock_and_destroy;

That's really what your transformation does - you are lifting the
calls of get_anon_bdev() (in guise of set_anon_super()) from the
tails of 'set' callbacks into the caller, making them conditional
upon the lack of other errors from 'set' and upon the ->s_dev left
zero and allow NULL for the case when that was all that had been
there.

The only place where you do something different is this:

> @@ -1191,7 +1191,6 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
>  static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>  {
>  	struct ceph_fs_client *fsc = s->s_fs_info;
> -	int ret;
>  
>  	dout("set_super %p\n", s);
>  
> @@ -1211,11 +1210,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>  	s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>  
>  	ceph_fscrypt_set_ops(s);
> -
> -	ret = set_anon_super_fc(s, fc);
> -	if (ret != 0)
> -		fsc->sb = NULL;
> -	return ret;
> +	return 0;

fsc->sb = NULL has disappeared here; it *is* OK (the caller won't look at
fsc->sb after failed sget_fc()), but that's worth a mention somewhere.
A separate commit removing that clearing fsc->sb in ceph_set_super(),
perhaps?