Re: [PATCH] fs/ceph/super: add mount options "snapdir{mode,uid,gid}"

Xiubo Li
2022-10-09T06:23:44Z

Hi Max,

Sorry for late and just back from a long vocation.

On 27/09/2022 20:08, Max Kellermann wrote:
> By default, the ".snap" directory inherits the parent's permissions
> and ownership, which allows all users to create new cephfs snapshots
> in arbitrary directories they have write access on.
>
> In some environments, giving everybody this capability is not
> desirable, but there is currently no way to disallow only some users
> to create snapshots. It is only possible to revoke the permission to
> the whole client (i.e. all users on the computer which mounts the
> cephfs).
>
> This patch allows overriding the permissions and ownership of all
> virtual ".snap" directories in a cephfs mount, which allows
> restricting (read and write) access to snapshots.
>
> For example, the mount options:
>
> snapdirmode=0751,snapdiruid=0,snapdirgid=4
>
> ... allows only user "root" to create or delete snapshots, and group
> "adm" (gid=4) is allowed to get a list of snapshots. All others are
> allowed to read the contents of existing snapshots (if they know the
> name).

I don't think this is a good place to implement this in client side.
Should this be a feature in cephx.

With this for the same directories in different mounts will behave
differently. Isn't that odd ?

-- Xiubo

> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
> fs/ceph/inode.c | 7 ++++---
> fs/ceph/super.c | 33 +++++++++++++++++++++++++++++++++
> fs/ceph/super.h | 4 ++++
> 3 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 56c53ab3618e..0e9388af2821 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -80,6 +80,7 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> };
> struct inode *inode = ceph_get_inode(parent->i_sb, vino);
> struct ceph_inode_info *ci = ceph_inode(inode);
> + struct ceph_mount_options *const fsopt = ceph_inode_to_client(parent)->mount_options;
>
> if (IS_ERR(inode))
> return inode;
> @@ -96,9 +97,9 @@ struct inode *ceph_get_snapdir(struct inode *parent)
> goto err;
> }
>
> - inode->i_mode = parent->i_mode;
> - inode->i_uid = parent->i_uid;
> - inode->i_gid = parent->i_gid;
> + inode->i_mode = fsopt->snapdir_mode == (umode_t)-1 ? parent->i_mode : fsopt->snapdir_mode;
> + inode->i_uid = uid_eq(fsopt->snapdir_uid, INVALID_UID) ? parent->i_uid : fsopt->snapdir_uid;
> + inode->i_gid = gid_eq(fsopt->snapdir_gid, INVALID_GID) ? parent->i_gid : fsopt->snapdir_gid;
> inode->i_mtime = parent->i_mtime;
> inode->i_ctime = parent->i_ctime;
> inode->i_atime = parent->i_atime;
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 40140805bdcf..5e5713946f7b 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -143,6 +143,9 @@ enum {
> Opt_readdir_max_entries,
> Opt_readdir_max_bytes,
> Opt_congestion_kb,
> + Opt_snapdirmode,
> + Opt_snapdiruid,
> + Opt_snapdirgid,
> /* int args above */
> Opt_snapdirname,
> Opt_mds_namespace,
> @@ -200,6 +203,9 @@ static const struct fs_parameter_spec ceph_mount_parameters[] = {
> fsparam_flag_no ("require_active_mds", Opt_require_active_mds),
> fsparam_u32 ("rsize", Opt_rsize),
> fsparam_string ("snapdirname", Opt_snapdirname),
> + fsparam_u32oct ("snapdirmode", Opt_snapdirmode),
> + fsparam_u32 ("snapdiruid", Opt_snapdiruid),
> + fsparam_u32 ("snapdirgid", Opt_snapdirgid),
> fsparam_string ("source", Opt_source),
> fsparam_string ("mon_addr", Opt_mon_addr),
> fsparam_u32 ("wsize", Opt_wsize),
> @@ -414,6 +420,22 @@ static int ceph_parse_mount_param(struct fs_context *fc,
> fsopt->snapdir_name = param->string;
> param->string = NULL;
> break;
> + case Opt_snapdirmode:
> + fsopt->snapdir_mode = result.uint_32;
> + if (fsopt->snapdir_mode & ~0777)
> + return invalfc(fc, "Invalid snapdirmode");
> + fsopt->snapdir_mode |= S_IFDIR;
> + break;
> + case Opt_snapdiruid:
> + fsopt->snapdir_uid = make_kuid(current_user_ns(), result.uint_32);
> + if (!uid_valid(fsopt->snapdir_uid))
> + return invalfc(fc, "Invalid snapdiruid");
> + break;
> + case Opt_snapdirgid:
> + fsopt->snapdir_gid = make_kgid(current_user_ns(), result.uint_32);
> + if (!gid_valid(fsopt->snapdir_gid))
> + return invalfc(fc, "Invalid snapdirgid");
> + break;
> case Opt_mds_namespace:
> if (!namespace_equals(fsopt, param->string, strlen(param->string)))
> return invalfc(fc, "Mismatching mds_namespace");
> @@ -734,6 +756,14 @@ static int ceph_show_options(struct seq_file *m, struct dentry *root)
> seq_printf(m, ",readdir_max_bytes=%u", fsopt->max_readdir_bytes);
> if (strcmp(fsopt->snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
> seq_show_option(m, "snapdirname", fsopt->snapdir_name);
> + if (fsopt->snapdir_mode != (umode_t)-1)
> + seq_printf(m, ",snapdirmode=%o", fsopt->snapdir_mode);
> + if (!uid_eq(fsopt->snapdir_uid, INVALID_UID))
> + seq_printf(m, ",snapdiruid=%o",
> + from_kuid_munged(&init_user_ns, fsopt->snapdir_uid));
> + if (!gid_eq(fsopt->snapdir_gid, INVALID_GID))
> + seq_printf(m, ",snapdirgid=%o",
> + from_kgid_munged(&init_user_ns, fsopt->snapdir_gid));
>
> return 0;
> }
> @@ -1335,6 +1365,9 @@ static int ceph_init_fs_context(struct fs_context *fc)
> fsopt->wsize = CEPH_MAX_WRITE_SIZE;
> fsopt->rsize = CEPH_MAX_READ_SIZE;
> fsopt->rasize = CEPH_RASIZE_DEFAULT;
> + fsopt->snapdir_mode = (umode_t)-1;
> + fsopt->snapdir_uid = INVALID_UID;
> + fsopt->snapdir_gid = INVALID_GID;
> fsopt->snapdir_name = kstrdup(CEPH_SNAPDIRNAME_DEFAULT, GFP_KERNEL);
> if (!fsopt->snapdir_name)
> goto nomem;
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index d44a366b2f1b..3c930816078d 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -85,6 +85,10 @@ struct ceph_mount_options {
> unsigned int max_readdir; /* max readdir result (entries) */
> unsigned int max_readdir_bytes; /* max readdir result (bytes) */
>
> + umode_t snapdir_mode;
> + kuid_t snapdir_uid;
> + kgid_t snapdir_gid;
> +
> bool new_dev_syntax;
>
> /*