diff mbox series

[3/3] libceph: reject mismatching name and fsid

Message ID TYCP286MB2066D19A68A9176E289BB4FDC0709@TYCP286MB2066.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State New, archived
Headers show
Series ceph: account for name and fsid in new device spec | expand

Commit Message

胡玮文 May 7, 2023, 5:55 p.m. UTC
From: Hu Weiwen <sehuww@mail.scut.edu.cn>

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@mail.scut.edu.cn>
---
 net/ceph/ceph_common.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Comments

Xiubo Li May 10, 2023, 7:02 a.m. UTC | #1
On 5/8/23 01:55, Hu Weiwen wrote:
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>
> 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@mail.scut.edu.cn>
> ---
>   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);
> +	{

BTW, do we need the '{}' here ?


> +		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);
胡玮文 May 10, 2023, 8:44 a.m. UTC | #2
On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote:
> 
> On 5/8/23 01:55, Hu Weiwen wrote:
> > From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > 
> > 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@mail.scut.edu.cn>
> > ---
> >   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);
> > +	{
> 
> BTW, do we need the '{}' here ?

I want to declare 'fsid' variable closer to its usage.  But a declaration
cannot follow a case label:
  
  error: a label can only be part of a statement and a declaration is not a statement

searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400
such usage.  Should be pretty common.

> > +		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);
>
Xiubo Li May 10, 2023, 10:44 a.m. UTC | #3
On 5/10/23 16:44, 胡玮文 wrote:
> On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote:
>> On 5/8/23 01:55, Hu Weiwen wrote:
>>> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>>>
>>> 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@mail.scut.edu.cn>
>>> ---
>>>    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);
>>> +	{
>> BTW, do we need the '{}' here ?
> I want to declare 'fsid' variable closer to its usage.  But a declaration
> cannot follow a case label:
>    
>    error: a label can only be part of a statement and a declaration is not a statement
>
> searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400
> such usage.  Should be pretty common.

Did you see this when compiling ? So odd I jsut remove them and it 
worked for me.


>>> +		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);
胡玮文 May 10, 2023, 11:46 a.m. UTC | #4
On Wed, May 10, 2023 at 06:44:22PM +0800, Xiubo Li wrote:
> 
> On 5/10/23 16:44, 胡玮文 wrote:
> > On Wed, May 10, 2023 at 03:02:09PM +0800, Xiubo Li wrote:
> > > On 5/8/23 01:55, Hu Weiwen wrote:
> > > > From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> > > > 
> > > > 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@mail.scut.edu.cn>
> > > > ---
> > > >    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);
> > > > +	{
> > > BTW, do we need the '{}' here ?
> > I want to declare 'fsid' variable closer to its usage.  But a declaration
> > cannot follow a case label:
> >    error: a label can only be part of a statement and a declaration is not a statement
> > 
> > searching for 'case \w+:\n\s+\{' in the source tree reveals about 1400
> > such usage.  Should be pretty common.
> 
> Did you see this when compiling ? So odd I jsut remove them and it worked
> for me.

Yes, my compiler is gcc version 9.4.0 from ubuntu 20.04. clangd 16.0.2
also gives error.

Console output:

  CC      net/ceph/ceph_common.o
net/ceph/ceph_common.c: In function ‘ceph_parse_param’:
net/ceph/ceph_common.c:443:3: error: a label can only be part of a statement and a declaration is not a statement
  443 |   struct ceph_fsid 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);
>
Ilya Dryomov June 27, 2023, 11:47 a.m. UTC | #5
On Sun, May 7, 2023 at 7:56 PM Hu Weiwen <huww98@outlook.com> wrote:
>
> From: Hu Weiwen <sehuww@mail.scut.edu.cn>
>
> 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@mail.scut.edu.cn>
> ---
>  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
胡玮文 June 30, 2023, 4:44 a.m. UTC | #6
On Tue, Jun 27, 2023 at 01:47:53PM +0200, Ilya Dryomov wrote:
> On Sun, May 7, 2023 at 7:56 PM Hu Weiwen <huww98@outlook.com> wrote:
> >
> > From: Hu Weiwen <sehuww@mail.scut.edu.cn>
> >
> > 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@mail.scut.edu.cn>
> > ---
> >  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
Hi Ilya,

I agree. Thank you for your advise. Please see my patch v2.

Hu Weiwen
diff mbox series

Patch

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);