On Sun, May 7, 2023 at 7:56 PM Hu Weiwen <huww98@xxxxxxxxxxx> wrote:
>
> From: Hu Weiwen <sehuww@xxxxxxxxxxxxxxxx>
>
> These are present in the device spec of cephfs. So they should be
> treated as immutable. Also reject `mount()' calls where options and
> device spec are inconsistent.
>
> Signed-off-by: Hu Weiwen <sehuww@xxxxxxxxxxxxxxxx>
> ---
> net/ceph/ceph_common.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index 4c6441536d55..c59c5ccc23a8 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -440,17 +440,33 @@ int ceph_parse_param(struct fs_parameter *param, struct ceph_options *opt,
> break;
>
> case Opt_fsid:
> - err = ceph_parse_fsid(param->string, &opt->fsid);
> + {
> + struct ceph_fsid fsid;
> +
> + err = ceph_parse_fsid(param->string, &fsid);
> if (err) {
> error_plog(&log, "Failed to parse fsid: %d", err);
> return err;
> }
> - opt->flags |= CEPH_OPT_FSID;
> +
> + if (!(opt->flags & CEPH_OPT_FSID)) {
> + opt->fsid = fsid;
> + opt->flags |= CEPH_OPT_FSID;
> + } else if (ceph_fsid_compare(&opt->fsid, &fsid)) {
> + error_plog(&log, "fsid already set to %pU",
> + &opt->fsid);
> + return -EINVAL;
> + }
> break;
> + }
> case Opt_name:
> - kfree(opt->name);
> - opt->name = param->string;
> - param->string = NULL;
> + if (!opt->name) {
> + opt->name = param->string;
> + param->string = NULL;
> + } else if (strcmp(opt->name, param->string)) {
> + error_plog(&log, "name already set to %s", opt->name);
> + return -EINVAL;
> + }
> break;
> case Opt_secret:
> ceph_crypto_key_destroy(opt->key);
> --
> 2.25.1
>
Hi Hu,
I'm not following the reason for singling out "fsid" and "name" in
ceph_parse_param() in net/ceph. All options are overridable in the
sense that the last setting wins on purpose, this is a long-standing
behavior. Allowing "key" or "secret" to be overridden while rejecting
the corresponding override for "name" is weird.
If there is a desire to treat "fsid" and "name" specially in CephFS
because they are coming from the device spec and aren't considered to
be mount options in the usual sense, I would suggest enforcing this
behavior in fs/ceph (and only when the device spec syntax is used).
Thanks,
Ilya
