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

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

[Luís Henriques]

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);
 	goto out_final;
 
 out:

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Luís Henriques]

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

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Chen, Rong A]

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
> 

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Luís Henriques]

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

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Xiubo Li]

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:
>

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Luís Henriques]

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

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Xiubo Li]

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
>

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Luís Henriques]

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

Re: [PATCH] ceph: fix memory leak in mount error path when using test_dummy_encryption

[Xiubo Li]

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
>