When the switch statment goes to default and return an error, ptr should
be freed since it is allocated in hfs_find_init.
Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init")
Signed-off-by: Zhipeng Lu <[email protected]>
---
fs/hfs/bfind.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index ef9498a6e88a..7aa3b9aba4d1 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
break;
default:
+ kfree(fd->search_key);
return -EINVAL;
}
return 0;
--
2.34.1
> On 22 Jan 2024, at 20:27, Zhipeng Lu <[email protected]> wrote:
>
> When the switch statment goes to default and return an error, ptr should
> be freed since it is allocated in hfs_find_init.
>
Do you have any memory leaks report? Could you share it in the comments?
Which use-case reproduces the issue? It will be easier to review the fix
If you can share the path of reproduction.
Thanks,
Slava.
> Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init")
> Signed-off-by: Zhipeng Lu <[email protected]>
> ---
> fs/hfs/bfind.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> index ef9498a6e88a..7aa3b9aba4d1 100644
> --- a/fs/hfs/bfind.c
> +++ b/fs/hfs/bfind.c
> @@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
> mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
> break;
> default:
> + kfree(fd->search_key);
> return -EINVAL;
> }
> return 0;
> --
> 2.34.1
>
> > On 22 Jan 2024, at 20:27, Zhipeng Lu <[email protected]> wrote:
> >
> > When the switch statment goes to default and return an error, ptr should
> > be freed since it is allocated in hfs_find_init.
> >
>
> Do you have any memory leaks report? Could you share it in the comments?
> Which use-case reproduces the issue? It will be easier to review the fix
> If you can share the path of reproduction.
>
> Thanks,
> Slava.
Well, we found this potential memory leak by static analysis.
We found that all of hfs_find_init's callers won't release `ptr` when
hfs_find_init fails, while they will do release `ptr` when functions
that after hfs_find_init fails. This tactic observation suggests that
hfs_find_init proberly should release `ptr` when it fails, i.e. in the
default branch of switch in this patch.
Besides, we noticed another implementation of hfs_find_init in
fs/hfsplus/bfind.c, which is essentially identical to the one in
this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch
to trigger an error-handling.
Thanks,
Zhipeng.
>
> > Fixes: b3b2177a2d79 ("hfs: add lock nesting notation to hfs_find_init")
> > Signed-off-by: Zhipeng Lu <[email protected]>
> > ---
> > fs/hfs/bfind.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> > index ef9498a6e88a..7aa3b9aba4d1 100644
> > --- a/fs/hfs/bfind.c
> > +++ b/fs/hfs/bfind.c
> > @@ -36,6 +36,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
> > mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
> > break;
> > default:
> > + kfree(fd->search_key);
> > return -EINVAL;
> > }
> > return 0;
> > --
> > 2.34.1
> >
> On 29 Jan 2024, at 15:54, [email protected] wrote:
>
>>> On 22 Jan 2024, at 20:27, Zhipeng Lu <[email protected]> wrote:
>>>
>>> When the switch statment goes to default and return an error, ptr should
>>> be freed since it is allocated in hfs_find_init.
>>>
>>
>> Do you have any memory leaks report? Could you share it in the comments?
>> Which use-case reproduces the issue? It will be easier to review the fix
>> If you can share the path of reproduction.
>>
>> Thanks,
>> Slava.
>
> Well, we found this potential memory leak by static analysis.
>
> We found that all of hfs_find_init's callers won't release `ptr` when
> hfs_find_init fails, while they will do release `ptr` when functions
> that after hfs_find_init fails. This tactic observation suggests that
> hfs_find_init proberly should release `ptr` when it fails, i.e. in the
> default branch of switch in this patch.
>
> Besides, we noticed another implementation of hfs_find_init in
> fs/hfsplus/bfind.c, which is essentially identical to the one in
> this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch
> to trigger an error-handling.
>
I see. I believe it makes sense to add all of this explanation
into comment section. Modification looks good. Mostly, hfs_find_exit()
does freeing resources and if hfs_find_init() fails, then hfs_find_exit()
is never called. Maybe, it makes sense to set fd->tree = NULL too but
it is not critical, as far as I can see.
Could you please rework the comment section of the patch?
Thanks,
Slava.
> > On 29 Jan 2024, at 15:54, [email protected] wrote:
> >
> >>> On 22 Jan 2024, at 20:27, Zhipeng Lu <[email protected]> wrote:
> >>>
> >>> When the switch statment goes to default and return an error, ptr should
> >>> be freed since it is allocated in hfs_find_init.
> >>>
> >>
> >> Do you have any memory leaks report? Could you share it in the comments?
> >> Which use-case reproduces the issue? It will be easier to review the fix
> >> If you can share the path of reproduction.
> >>
> >> Thanks,
> >> Slava.
> >
> > Well, we found this potential memory leak by static analysis.
> >
> > We found that all of hfs_find_init's callers won't release `ptr` when
> > hfs_find_init fails, while they will do release `ptr` when functions
> > that after hfs_find_init fails. This tactic observation suggests that
> > hfs_find_init proberly should release `ptr` when it fails, i.e. in the
> > default branch of switch in this patch.
> >
> > Besides, we noticed another implementation of hfs_find_init in
> > fs/hfsplus/bfind.c, which is essentially identical to the one in
> > this patch (in fs/hfs/bfind.c) but calling `BUG();` in default branch
> > to trigger an error-handling.
> >
>
> I see. I believe it makes sense to add all of this explanation
> into comment section. Modification looks good. Mostly, hfs_find_exit()
> does freeing resources and if hfs_find_init() fails, then hfs_find_exit()
> is never called. Maybe, it makes sense to set fd->tree = NULL too but
> it is not critical, as far as I can see.
>
> Could you please rework the comment section of the patch?
Sure, I'll including such idea in this patch and send a v2 version of
this patch.
Thanks,
Zhipeng.
> Thanks,
> Slava.