Re: [RFC PATCH] ceph: don't allow access to MDS-private inodes

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2021-04-01 at 16:16 -0400, Jeff Layton wrote:
> The MDS reserves a set of inodes for its own usage, and these should
> never be accessible to clients. Add a new helper to vet a proposed
> inode number against that range, and complain loudly and refuse to
> create or look it up if it's in it.
> 
> Also, ensure that the MDS doesn't try to delegate that range to us
> either. Print a warning if it does, and don't save the range in the
> xarray.
> 
> URL: https://tracker.ceph.com/issues/49922
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/inode.c      |  5 +++++
>  fs/ceph/mds_client.c |  7 +++++++
>  fs/ceph/super.h      | 24 ++++++++++++++++++++++++
>  3 files changed, 36 insertions(+)
> 

Should have cc'ed Ilya too -- mea culpa...

The idea here is to not allow the client to attempt to access this inode
number range (and gather some info about how it might be occurring).

This is RFC since Zheng seemed to indicate in a corresponding PR for the
MDS that there may be reason to allow the clients to get at these. If
there's not such a reason though, then I think something like this patch
would be good.

> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 2c512475c170..6aa796c59e1b 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -54,8 +54,13 @@ static int ceph_set_ino_cb(struct inode *inode, void *data)
>  
>  struct inode *ceph_get_inode(struct super_block *sb, struct ceph_vino vino)
>  {
> +	int ret;
>  	struct inode *inode;
>  
> +	ret = ceph_vet_vino(vino);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	inode = iget5_locked(sb, (unsigned long)vino.ino, ceph_ino_compare,
>  			     ceph_set_ino_cb, &vino);
>  	if (!inode)
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 73ecb7d128c9..2d7dcd295bb9 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -433,6 +433,13 @@ static int ceph_parse_deleg_inos(void **p, void *end,
>  
>  		ceph_decode_64_safe(p, end, start, bad);
>  		ceph_decode_64_safe(p, end, len, bad);
> +
> +		/* Don't accept a delegation of system inodes */
> +		if (start < CEPH_INO_SYSTEM_BASE) {
> +			pr_warn_ratelimited("ceph: ignoring reserved inode range delegation (start=0x%llx len=0x%llx)\n",
> +					start, len);
> +			continue;
> +		}
>  		while (len--) {
>  			int err = xa_insert(&s->s_delegated_inos, ino = start++,
>  					    DELEGATED_INO_AVAILABLE,
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 5e0e1aeee1b5..1d63c5a28665 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -529,10 +529,34 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
>  		ci->i_vino.snap == pvino->snap;
>  }
>  
> +/*
> + * The MDS reserves a set of inodes for its own usage. These should never
> + * be accessible by clients, and so the MDS has no reason to ever hand these
> + * out.
> + *
> + * These come from src/mds/mdstypes.h in the ceph sources.
> + */
> +#define CEPH_MAX_MDS		0x100
> +#define CEPH_NUM_STRAY		10
> +#define CEPH_INO_SYSTEM_BASE	((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
> +
> +static inline int ceph_vet_vino(const struct ceph_vino vino)
> +{
> +	if (vino.ino < CEPH_INO_SYSTEM_BASE && vino.ino != CEPH_INO_ROOT) {
> +		WARN_RATELIMIT(1, "Attempt to access reserved inode number 0x%llx", vino.ino);
> +		return -EREMOTEIO;
> +	}
> +	return 0;
> +}
>  

I think I'll also need to fix up the LOOKUPINO callers in export.c as
well, and for that, I'll probably rework the above function to return
bool (and change the name). The callers will need to be changed
accordingly.

>  static inline struct inode *ceph_find_inode(struct super_block *sb,
>  					    struct ceph_vino vino)
>  {
> +	int ret = ceph_vet_vino(vino);
> +
> +	if (ret)
> +		return NULL;
> +
>  	/*
>  	 * NB: The hashval will be run through the fs/inode.c hash function
>  	 * anyway, so there is no need to squash the inode number down to

-- 
Jeff Layton <jlayton@kernel.org>