diff mbox series

ceph: fix memory leak in mount error path when using test_dummy_encryption

Message ID 20221103153619.11068-1-lhenriques@suse.de
State New
Headers
Series ceph: fix memory leak in mount error path when using test_dummy_encryption |

Commit Message

Luis Henriques Nov. 3, 2022, 3:36 p.m. UTC 
  Because ceph_init_fs_context() will never be invoced in case we get a
mount error, destroy_mount_options() won't be releasing fscrypt resources
with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
an invocation to this function in the mount error path.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/super.c | 1 +
 1 file changed, 1 insertion(+)

Comments

kernel test robot Nov. 4, 2022, 6:40 a.m. UTC | #1 
Hi Luís,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ceph-client/for-linus]
[also build test ERROR on linus/master v6.1-rc3 next-20221104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
base:   https://github.com/ceph/ceph-client.git for-linus
patch link:    https://lore.kernel.org/r/20221103153619.11068-1-lhenriques%40suse.de
patch subject: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption
config: nios2-randconfig-r014-20221103
compiler: nios2-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2a75b40477b60d0aba1b72379f7142225bea2a48
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
        git checkout 2a75b40477b60d0aba1b72379f7142225bea2a48
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/ceph/super.c: In function 'ceph_get_tree':
   fs/ceph/super.c:1268:9: error: implicit declaration of function 'fscrypt_free_dummy_policy' [-Werror=implicit-function-declaration]
    1268 |         fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
>> fs/ceph/super.c:1268:39: error: 'struct ceph_fs_client' has no member named 'fsc_dummy_enc_policy'
    1268 |         fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
         |                                       ^~
   cc1: some warnings being treated as errors


vim +1268 fs/ceph/super.c

  1196	
  1197	static int ceph_get_tree(struct fs_context *fc)
  1198	{
  1199		struct ceph_parse_opts_ctx *pctx = fc->fs_private;
  1200		struct ceph_mount_options *fsopt = pctx->opts;
  1201		struct super_block *sb;
  1202		struct ceph_fs_client *fsc;
  1203		struct dentry *res;
  1204		int (*compare_super)(struct super_block *, struct fs_context *) =
  1205			ceph_compare_super;
  1206		int err;
  1207	
  1208		dout("ceph_get_tree\n");
  1209	
  1210		if (!fc->source)
  1211			return invalfc(fc, "No source");
  1212		if (fsopt->new_dev_syntax && !fsopt->mon_addr)
  1213			return invalfc(fc, "No monitor address");
  1214	
  1215		/* create client (which we may/may not use) */
  1216		fsc = create_fs_client(pctx->opts, pctx->copts);
  1217		pctx->opts = NULL;
  1218		pctx->copts = NULL;
  1219		if (IS_ERR(fsc)) {
  1220			err = PTR_ERR(fsc);
  1221			goto out_final;
  1222		}
  1223	
  1224		err = ceph_mdsc_init(fsc);
  1225		if (err < 0)
  1226			goto out;
  1227	
  1228		if (ceph_test_opt(fsc->client, NOSHARE))
  1229			compare_super = NULL;
  1230	
  1231		fc->s_fs_info = fsc;
  1232		sb = sget_fc(fc, compare_super, ceph_set_super);
  1233		fc->s_fs_info = NULL;
  1234		if (IS_ERR(sb)) {
  1235			err = PTR_ERR(sb);
  1236			goto out;
  1237		}
  1238	
  1239		if (ceph_sb_to_client(sb) != fsc) {
  1240			destroy_fs_client(fsc);
  1241			fsc = ceph_sb_to_client(sb);
  1242			dout("get_sb got existing client %p\n", fsc);
  1243		} else {
  1244			dout("get_sb using new client %p\n", fsc);
  1245			err = ceph_setup_bdi(sb, fsc);
  1246			if (err < 0)
  1247				goto out_splat;
  1248		}
  1249	
  1250		res = ceph_real_mount(fsc, fc);
  1251		if (IS_ERR(res)) {
  1252			err = PTR_ERR(res);
  1253			goto out_splat;
  1254		}
  1255		dout("root %p inode %p ino %llx.%llx\n", res,
  1256		     d_inode(res), ceph_vinop(d_inode(res)));
  1257		fc->root = fsc->sb->s_root;
  1258		return 0;
  1259	
  1260	out_splat:
  1261		if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
  1262			pr_info("No mds server is up or the cluster is laggy\n");
  1263			err = -EHOSTUNREACH;
  1264		}
  1265	
  1266		ceph_mdsc_close_sessions(fsc->mdsc);
  1267		deactivate_locked_super(sb);
> 1268		fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
  1269		goto out_final;
  1270	
  1271	out:
  1272		destroy_fs_client(fsc);
  1273	out_final:
  1274		dout("ceph_get_tree fail %d\n", err);
  1275		return err;
  1276	}
  1277
Luis Henriques Nov. 4, 2022, 9:47 a.m. UTC | #2 
On Fri, Nov 04, 2022 at 02:40:25PM +0800, kernel test robot wrote:
> Hi Luís,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on ceph-client/for-linus]
> [also build test ERROR on linus/master v6.1-rc3 next-20221104]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
> base:   https://github.com/ceph/ceph-client.git for-linus

Well, thank you very much!  Now, how do I tell this bot that this patch
isn't targeting this branch?

Cheers,
--
Luís
Chen, Rong A Nov. 4, 2022, 10:02 a.m. UTC | #3 
On 11/4/2022 5:47 PM, Luís Henriques wrote:
> On Fri, Nov 04, 2022 at 02:40:25PM +0800, kernel test robot wrote:
>> Hi Luís,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on ceph-client/for-linus]
>> [also build test ERROR on linus/master v6.1-rc3 next-20221104]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
>> base:   https://github.com/ceph/ceph-client.git for-linus
> 
> Well, thank you very much!  Now, how do I tell this bot that this patch
> isn't targeting this branch?

Hi Luis,

The below message may help:

 >> [If your patch is applied to the wrong git tree, kindly drop us a note.
 >> And when submitting patch, we suggest to use '--base' as documented in
 >> https://git-scm.com/docs/git-format-patch#_base_tree_information]

we also appreciate that if developers can tell us the right branch
to improve the bot when applied to wrong place.

Best Regards,
Rong Chen

> 
> Cheers,
> --
> Luís
>
Luis Henriques Nov. 4, 2022, 11:21 a.m. UTC | #4 
On Fri, Nov 04, 2022 at 06:02:55PM +0800, Chen, Rong A wrote:
> 
> 
> On 11/4/2022 5:47 PM, Luís Henriques wrote:
> > On Fri, Nov 04, 2022 at 02:40:25PM +0800, kernel test robot wrote:
> > > Hi Luís,
> > > 
> > > Thank you for the patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on ceph-client/for-linus]
> > > [also build test ERROR on linus/master v6.1-rc3 next-20221104]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
> > > base:   https://github.com/ceph/ceph-client.git for-linus
> > 
> > Well, thank you very much!  Now, how do I tell this bot that this patch
> > isn't targeting this branch?
> 
> Hi Luis,
> 
> The below message may help:
> 
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch#_base_tree_information]

Ah! Awesome, thank you very much for pointing me at this.  I'll try to
remember next time to use '--base' when sending patches for a specific
branch.

> we also appreciate that if developers can tell us the right branch
> to improve the bot when applied to wrong place.

Yeah, I guess that in general the bot is picking the right branch for
ceph.  In this case, the patch was for the fscrypt development branch, so
my mistake for not using '--base'.

Cheers,
--
Luís
kernel test robot Nov. 4, 2022, 2:05 p.m. UTC | #5 
Hi Luís,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on ceph-client/for-linus]
[also build test ERROR on linus/master v6.1-rc3 next-20221104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
base:   https://github.com/ceph/ceph-client.git for-linus
patch link:    https://lore.kernel.org/r/20221103153619.11068-1-lhenriques%40suse.de
patch subject: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption
config: hexagon-buildonly-randconfig-r006-20221104
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 2bbafe04fe785a9469bea5a3737f8d7d3ce4aca2)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/2a75b40477b60d0aba1b72379f7142225bea2a48
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lu-s-Henriques/ceph-fix-memory-leak-in-mount-error-path-when-using-test_dummy_encryption/20221103-233629
        git checkout 2a75b40477b60d0aba1b72379f7142225bea2a48
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/ceph/super.c:1268:2: error: call to undeclared function 'fscrypt_free_dummy_policy'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
           ^
>> fs/ceph/super.c:1268:34: error: no member named 'fsc_dummy_enc_policy' in 'struct ceph_fs_client'
           fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
                                      ~~~  ^
   2 errors generated.


vim +1268 fs/ceph/super.c

  1196	
  1197	static int ceph_get_tree(struct fs_context *fc)
  1198	{
  1199		struct ceph_parse_opts_ctx *pctx = fc->fs_private;
  1200		struct ceph_mount_options *fsopt = pctx->opts;
  1201		struct super_block *sb;
  1202		struct ceph_fs_client *fsc;
  1203		struct dentry *res;
  1204		int (*compare_super)(struct super_block *, struct fs_context *) =
  1205			ceph_compare_super;
  1206		int err;
  1207	
  1208		dout("ceph_get_tree\n");
  1209	
  1210		if (!fc->source)
  1211			return invalfc(fc, "No source");
  1212		if (fsopt->new_dev_syntax && !fsopt->mon_addr)
  1213			return invalfc(fc, "No monitor address");
  1214	
  1215		/* create client (which we may/may not use) */
  1216		fsc = create_fs_client(pctx->opts, pctx->copts);
  1217		pctx->opts = NULL;
  1218		pctx->copts = NULL;
  1219		if (IS_ERR(fsc)) {
  1220			err = PTR_ERR(fsc);
  1221			goto out_final;
  1222		}
  1223	
  1224		err = ceph_mdsc_init(fsc);
  1225		if (err < 0)
  1226			goto out;
  1227	
  1228		if (ceph_test_opt(fsc->client, NOSHARE))
  1229			compare_super = NULL;
  1230	
  1231		fc->s_fs_info = fsc;
  1232		sb = sget_fc(fc, compare_super, ceph_set_super);
  1233		fc->s_fs_info = NULL;
  1234		if (IS_ERR(sb)) {
  1235			err = PTR_ERR(sb);
  1236			goto out;
  1237		}
  1238	
  1239		if (ceph_sb_to_client(sb) != fsc) {
  1240			destroy_fs_client(fsc);
  1241			fsc = ceph_sb_to_client(sb);
  1242			dout("get_sb got existing client %p\n", fsc);
  1243		} else {
  1244			dout("get_sb using new client %p\n", fsc);
  1245			err = ceph_setup_bdi(sb, fsc);
  1246			if (err < 0)
  1247				goto out_splat;
  1248		}
  1249	
  1250		res = ceph_real_mount(fsc, fc);
  1251		if (IS_ERR(res)) {
  1252			err = PTR_ERR(res);
  1253			goto out_splat;
  1254		}
  1255		dout("root %p inode %p ino %llx.%llx\n", res,
  1256		     d_inode(res), ceph_vinop(d_inode(res)));
  1257		fc->root = fsc->sb->s_root;
  1258		return 0;
  1259	
  1260	out_splat:
  1261		if (!ceph_mdsmap_is_cluster_available(fsc->mdsc->mdsmap)) {
  1262			pr_info("No mds server is up or the cluster is laggy\n");
  1263			err = -EHOSTUNREACH;
  1264		}
  1265	
  1266		ceph_mdsc_close_sessions(fsc->mdsc);
  1267		deactivate_locked_super(sb);
> 1268		fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
  1269		goto out_final;
  1270	
  1271	out:
  1272		destroy_fs_client(fsc);
  1273	out_final:
  1274		dout("ceph_get_tree fail %d\n", err);
  1275		return err;
  1276	}
  1277
Xiubo Li Nov. 7, 2022, 7:47 a.m. UTC | #6 
On 03/11/2022 23:36, Luís Henriques wrote:
> Because ceph_init_fs_context() will never be invoced in case we get a
> mount error, destroy_mount_options() won't be releasing fscrypt resources
> with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
> an invocation to this function in the mount error path.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/super.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 2224d44d21c0..6b9fd04b25cd 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -1362,6 +1362,7 @@ static int ceph_get_tree(struct fs_context *fc)
>   
>   	ceph_mdsc_close_sessions(fsc->mdsc);
>   	deactivate_locked_super(sb);
> +	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);

Hi Luis,

BTW, any reason the following code won't be triggered ?

deactivate_locked_super(sb);

   --> fs->kill_sb(s);

         --> ceph_kill_sb()

               --> kill_anon_super()

                     --> generic_shutdown_super()

                           --> sop->put_super()

                                 --> ceph_put_super()

                                       --> ceph_fscrypt_free_dummy_policy()

                                            --> fscrypt_free_dummy_policy(

Thanks!

- Xiubo


>   	goto out_final;
>   
>   out:
>
Luis Henriques Nov. 7, 2022, 10:23 a.m. UTC | #7 
On Mon, Nov 07, 2022 at 03:47:23PM +0800, Xiubo Li wrote:
> 
> On 03/11/2022 23:36, Luís Henriques wrote:
> > Because ceph_init_fs_context() will never be invoced in case we get a
> > mount error, destroy_mount_options() won't be releasing fscrypt resources
> > with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
> > an invocation to this function in the mount error path.
> > 
> > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > ---
> >   fs/ceph/super.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 2224d44d21c0..6b9fd04b25cd 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -1362,6 +1362,7 @@ static int ceph_get_tree(struct fs_context *fc)
> >   	ceph_mdsc_close_sessions(fsc->mdsc);
> >   	deactivate_locked_super(sb);
> > +	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
> 
> Hi Luis,
> 
> BTW, any reason the following code won't be triggered ?
> 
> deactivate_locked_super(sb);
> 
>   --> fs->kill_sb(s);
> 
>         --> ceph_kill_sb()
> 
>               --> kill_anon_super()
> 
>                     --> generic_shutdown_super()
> 
>                           --> sop->put_super()
> 
>                                 --> ceph_put_super()
> 
>                                       --> ceph_fscrypt_free_dummy_policy()
> 
>                                            --> fscrypt_free_dummy_policy(
> 

Here's what I'm seeing here:

  sys_mount->path_mount->do_new_mount->vfs_get_tree->ceph_get_tree

ceph_get_tree() fails due to ceph_real_mount() returning an error.  My
understanding is that that, since fc->root is never set, that code path
will never be triggered.  Does that make sense?

An easy way to reproduce is by running fstest ceph/005 with the
'test_dummy_encryption' option.  (I'll probably need to send a patch to
disable this test when this option is present.)

Cheers,
--
Luís
Xiubo Li Nov. 7, 2022, 11:06 a.m. UTC | #8 
On 07/11/2022 18:23, Luís Henriques wrote:
> On Mon, Nov 07, 2022 at 03:47:23PM +0800, Xiubo Li wrote:
>> On 03/11/2022 23:36, Luís Henriques wrote:
>>> Because ceph_init_fs_context() will never be invoced in case we get a
>>> mount error, destroy_mount_options() won't be releasing fscrypt resources
>>> with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
>>> an invocation to this function in the mount error path.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>    fs/ceph/super.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>> index 2224d44d21c0..6b9fd04b25cd 100644
>>> --- a/fs/ceph/super.c
>>> +++ b/fs/ceph/super.c
>>> @@ -1362,6 +1362,7 @@ static int ceph_get_tree(struct fs_context *fc)
>>>    	ceph_mdsc_close_sessions(fsc->mdsc);
>>>    	deactivate_locked_super(sb);
>>> +	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
>> Hi Luis,
>>
>> BTW, any reason the following code won't be triggered ?
>>
>> deactivate_locked_super(sb);
>>
>>    --> fs->kill_sb(s);
>>
>>          --> ceph_kill_sb()
>>
>>                --> kill_anon_super()
>>
>>                      --> generic_shutdown_super()
>>
>>                            --> sop->put_super()
>>
>>                                  --> ceph_put_super()
>>
>>                                        --> ceph_fscrypt_free_dummy_policy()
>>
>>                                             --> fscrypt_free_dummy_policy(
>>
> Here's what I'm seeing here:
>
>    sys_mount->path_mount->do_new_mount->vfs_get_tree->ceph_get_tree
>
> ceph_get_tree() fails due to ceph_real_mount() returning an error.  My
> understanding is that that, since fc->root is never set, that code path
> will never be triggered.  Does that make sense?

Okay, you are right!

How about fixing it in ceph_real_mount() instead ?

>
> An easy way to reproduce is by running fstest ceph/005 with the
> 'test_dummy_encryption' option.  (I'll probably need to send a patch to
> disable this test when this option is present.)

Anyway this should be fixed in kceph.

Thanks!

- Xiubo


>
> Cheers,
> --
> Luís
>
Luis Henriques Nov. 7, 2022, 2:19 p.m. UTC | #9 
On Mon, Nov 07, 2022 at 07:06:40PM +0800, Xiubo Li wrote:
> 
> On 07/11/2022 18:23, Luís Henriques wrote:
> > On Mon, Nov 07, 2022 at 03:47:23PM +0800, Xiubo Li wrote:
> > > On 03/11/2022 23:36, Luís Henriques wrote:
> > > > Because ceph_init_fs_context() will never be invoced in case we get a
> > > > mount error, destroy_mount_options() won't be releasing fscrypt resources
> > > > with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
> > > > an invocation to this function in the mount error path.
> > > > 
> > > > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > > > ---
> > > >    fs/ceph/super.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > > > index 2224d44d21c0..6b9fd04b25cd 100644
> > > > --- a/fs/ceph/super.c
> > > > +++ b/fs/ceph/super.c
> > > > @@ -1362,6 +1362,7 @@ static int ceph_get_tree(struct fs_context *fc)
> > > >    	ceph_mdsc_close_sessions(fsc->mdsc);
> > > >    	deactivate_locked_super(sb);
> > > > +	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
> > > Hi Luis,
> > > 
> > > BTW, any reason the following code won't be triggered ?
> > > 
> > > deactivate_locked_super(sb);
> > > 
> > >    --> fs->kill_sb(s);
> > > 
> > >          --> ceph_kill_sb()
> > > 
> > >                --> kill_anon_super()
> > > 
> > >                      --> generic_shutdown_super()
> > > 
> > >                            --> sop->put_super()
> > > 
> > >                                  --> ceph_put_super()
> > > 
> > >                                        --> ceph_fscrypt_free_dummy_policy()
> > > 
> > >                                             --> fscrypt_free_dummy_policy(
> > > 
> > Here's what I'm seeing here:
> > 
> >    sys_mount->path_mount->do_new_mount->vfs_get_tree->ceph_get_tree
> > 
> > ceph_get_tree() fails due to ceph_real_mount() returning an error.  My
> > understanding is that that, since fc->root is never set, that code path
> > will never be triggered.  Does that make sense?
> 
> Okay, you are right!
> 
> How about fixing it in ceph_real_mount() instead ?

Sure, I can send a patch for doing that instead.  However, my opinion is
that it makes more sense to do it, mostly because ceph_get_tree() is
already doing clean-up work on the error path (ceph_mdsc_close_sessions()
and deactivate_locked_super()).

But let me know if you really prefer doing in ceph_read_mount() and I'll
send v2.

> > 
> > An easy way to reproduce is by running fstest ceph/005 with the
> > 'test_dummy_encryption' option.  (I'll probably need to send a patch to
> > disable this test when this option is present.)
> 
> Anyway this should be fixed in kceph.

Yes, agreed.

Cheers,
--
Luís
Xiubo Li Nov. 8, 2022, 5:56 a.m. UTC | #10 
On 07/11/2022 22:19, Luís Henriques wrote:
> On Mon, Nov 07, 2022 at 07:06:40PM +0800, Xiubo Li wrote:
>> On 07/11/2022 18:23, Luís Henriques wrote:
>>> On Mon, Nov 07, 2022 at 03:47:23PM +0800, Xiubo Li wrote:
>>>> On 03/11/2022 23:36, Luís Henriques wrote:
>>>>> Because ceph_init_fs_context() will never be invoced in case we get a
>>>>> mount error, destroy_mount_options() won't be releasing fscrypt resources
>>>>> with fscrypt_free_dummy_policy().  This will result in a memory leak.  Add
>>>>> an invocation to this function in the mount error path.
>>>>>
>>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>>> ---
>>>>>     fs/ceph/super.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>>>> index 2224d44d21c0..6b9fd04b25cd 100644
>>>>> --- a/fs/ceph/super.c
>>>>> +++ b/fs/ceph/super.c
>>>>> @@ -1362,6 +1362,7 @@ static int ceph_get_tree(struct fs_context *fc)
>>>>>     	ceph_mdsc_close_sessions(fsc->mdsc);
>>>>>     	deactivate_locked_super(sb);
>>>>> +	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
>>>> Hi Luis,
>>>>
>>>> BTW, any reason the following code won't be triggered ?
>>>>
>>>> deactivate_locked_super(sb);
>>>>
>>>>     --> fs->kill_sb(s);
>>>>
>>>>           --> ceph_kill_sb()
>>>>
>>>>                 --> kill_anon_super()
>>>>
>>>>                       --> generic_shutdown_super()
>>>>
>>>>                             --> sop->put_super()
>>>>
>>>>                                   --> ceph_put_super()
>>>>
>>>>                                         --> ceph_fscrypt_free_dummy_policy()
>>>>
>>>>                                              --> fscrypt_free_dummy_policy(
>>>>
>>> Here's what I'm seeing here:
>>>
>>>     sys_mount->path_mount->do_new_mount->vfs_get_tree->ceph_get_tree
>>>
>>> ceph_get_tree() fails due to ceph_real_mount() returning an error.  My
>>> understanding is that that, since fc->root is never set, that code path
>>> will never be triggered.  Does that make sense?
>> Okay, you are right!
>>
>> How about fixing it in ceph_real_mount() instead ?
> Sure, I can send a patch for doing that instead.  However, my opinion is
> that it makes more sense to do it, mostly because ceph_get_tree() is
> already doing clean-up work on the error path (ceph_mdsc_close_sessions()
> and deactivate_locked_super()).
>
> But let me know if you really prefer doing in ceph_read_mount() and I'll
> send v2.

IMO it'd better to do this in ceph_real_mount(), which will make the 
code to be easier to read.

Thanks!

- Xiubo

>>> An easy way to reproduce is by running fstest ceph/005 with the
>>> 'test_dummy_encryption' option.  (I'll probably need to send a patch to
>>> disable this test when this option is present.)
>> Anyway this should be fixed in kceph.
> Yes, agreed.
>
> Cheers,
> --
> Luís
>
diff mbox series

Patch

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 2224d44d21c0..6b9fd04b25cd 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -1362,6 +1362,7 @@  static int ceph_get_tree(struct fs_context *fc)
 
 	ceph_mdsc_close_sessions(fsc->mdsc);
 	deactivate_locked_super(sb);
+	fscrypt_free_dummy_policy(&fsc->fsc_dummy_enc_policy);
 	goto out_final;
 
 out: