| Message ID | 20221103153619.11068-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix memory leak in mount error path when using test_dummy_encryption | expand |
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 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 >
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 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: >
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
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 >
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
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 --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:
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(+)