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 <[email protected]>
---
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);
goto out_final;
out:
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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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
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
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
>
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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> ---
> 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:
>
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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
>
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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]>
>>>>> ---
>>>>> 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
>