[PATCH][next] fs: dlm: Fix memory leak of object mh

[PATCH][next] fs: dlm: Fix memory leak of object mh

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

There is an error return path that is not kfree'ing mh after
it has been successfully allocates.  Fix this by free'ing it.

Addresses-Coverity: ("Resource leak")
Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 fs/dlm/rcom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
index 085f21966c72..19298edc1573 100644
--- a/fs/dlm/rcom.c
+++ b/fs/dlm/rcom.c
@@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
 	if (rc_in->rc_id == 0xFFFFFFFF) {
 		log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
 		dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
+		kfree(mh);
 		return;
 	}
 
-- 
2.31.1

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Alexander Ahring Oder Aring]

Hi,

On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> There is an error return path that is not kfree'ing mh after
> it has been successfully allocates.  Fix this by free'ing it.
>
> Addresses-Coverity: ("Resource leak")
> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/dlm/rcom.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> index 085f21966c72..19298edc1573 100644
> --- a/fs/dlm/rcom.c
> +++ b/fs/dlm/rcom.c
> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
>         if (rc_in->rc_id == 0xFFFFFFFF) {
>                 log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
>                 dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> +               kfree(mh);
>                 return;

This seems to be a bigger issue, we cannot revert the buffer
allocation with a kfree, we cannot revert it at all. We should avoid
any error handling between create_rcom() and send_rcom(). In general
between get_buffer/commit_buffer.

I don't see a problem with moving the error handling before
create_rcom(). That should fix the issue.

- Alex

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Colin Ian King]

On 26/05/2021 15:19, Alexander Ahring Oder Aring wrote:
> Hi,
> 
> On Wed, May 26, 2021 at 9:40 AM Colin King <colin.king@canonical.com> wrote:
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There is an error return path that is not kfree'ing mh after
>> it has been successfully allocates.  Fix this by free'ing it.
>>
>> Addresses-Coverity: ("Resource leak")
>> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  fs/dlm/rcom.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
>> index 085f21966c72..19298edc1573 100644
>> --- a/fs/dlm/rcom.c
>> +++ b/fs/dlm/rcom.c
>> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
>>         if (rc_in->rc_id == 0xFFFFFFFF) {
>>                 log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
>>                 dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
>> +               kfree(mh);
>>                 return;
> 
> This seems to be a bigger issue, we cannot revert the buffer
> allocation with a kfree, we cannot revert it at all. We should avoid
> any error handling between create_rcom() and send_rcom(). In general
> between get_buffer/commit_buffer.
> 
> I don't see a problem with moving the error handling before
> create_rcom(). That should fix the issue.

Good point, I'll send a V2 in a while

> 
> - Alex
> 

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Dan Carpenter]

On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There is an error return path that is not kfree'ing mh after
> it has been successfully allocates.  Fix this by free'ing it.
> 
> Addresses-Coverity: ("Resource leak")
> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  fs/dlm/rcom.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> index 085f21966c72..19298edc1573 100644
> --- a/fs/dlm/rcom.c
> +++ b/fs/dlm/rcom.c
> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
>  	if (rc_in->rc_id == 0xFFFFFFFF) {
>  		log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
>  		dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> +		kfree(mh);

Am I looking at the same code as you?  (I often am not able to review
your patches because you're doing development on stuff that hasn't hit
linux-next).  Anyway, to me this doesn't seem like the correct fix at
all.  There are some other things to free and the "mh" pointer is on
a bunch of lists so it leads to use after frees.

regards,
dan carpenter

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Colin Ian King]

On 26/05/2021 16:01, Dan Carpenter wrote:
> On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There is an error return path that is not kfree'ing mh after
>> it has been successfully allocates.  Fix this by free'ing it.
>>
>> Addresses-Coverity: ("Resource leak")
>> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  fs/dlm/rcom.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
>> index 085f21966c72..19298edc1573 100644
>> --- a/fs/dlm/rcom.c
>> +++ b/fs/dlm/rcom.c
>> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
>>  	if (rc_in->rc_id == 0xFFFFFFFF) {
>>  		log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
>>  		dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
>> +		kfree(mh);
> 
> Am I looking at the same code as you?  (I often am not able to review
> your patches because you're doing development on stuff that hasn't hit
> linux-next).  Anyway, to me this doesn't seem like the correct fix at
> all.  There are some other things to free and the "mh" pointer is on
> a bunch of lists so it leads to use after frees.

I've send a V2. It was indeed a brown-paper-bag bad fix.

> 
> regards,
> dan carpenter
> 

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Dan Carpenter]

On Wed, May 26, 2021 at 04:11:06PM +0100, Colin Ian King wrote:
> On 26/05/2021 16:01, Dan Carpenter wrote:
> > On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> There is an error return path that is not kfree'ing mh after
> >> it has been successfully allocates.  Fix this by free'ing it.
> >>
> >> Addresses-Coverity: ("Resource leak")
> >> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >>  fs/dlm/rcom.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> >> index 085f21966c72..19298edc1573 100644
> >> --- a/fs/dlm/rcom.c
> >> +++ b/fs/dlm/rcom.c
> >> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
> >>  	if (rc_in->rc_id == 0xFFFFFFFF) {
> >>  		log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
> >>  		dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> >> +		kfree(mh);
> > 
> > Am I looking at the same code as you?  (I often am not able to review
> > your patches because you're doing development on stuff that hasn't hit
> > linux-next).  Anyway, to me this doesn't seem like the correct fix at
> > all.  There are some other things to free and the "mh" pointer is on
> > a bunch of lists so it leads to use after frees.
      ^^^^^^^^^^^^^^
This is sort of impossible, of course, because the struct only has one
list_head so it can only be in one list and not a "bunch of lists".

The dlm code seems to be going out of its way to use void pointers and
that makes it difficult to parse with Smatch.

But in other subsystems, we could make it a rule that list_heads are
"poison" "init" or "added".  If you freed a memory with an "added"
list_head then print a warning.  Or if you added a list_head but it was
already in the added state then print a warning.  Another idea is that
if you freed a struct mh before the mh->page allocation was freed then
print a warning about the leak.  This one is probably more prone to
false positives but there might be workarounds for those.  #IdeasToImplement

regards,
dan carpenter

Re: [PATCH][next] fs: dlm: Fix memory leak of object mh

[Alexander Ahring Oder Aring]

Hi,

On Wed, May 26, 2021 at 2:24 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, May 26, 2021 at 04:11:06PM +0100, Colin Ian King wrote:
> > On 26/05/2021 16:01, Dan Carpenter wrote:
> > > On Wed, May 26, 2021 at 02:40:39PM +0100, Colin King wrote:
> > >> From: Colin Ian King <colin.king@canonical.com>
> > >>
> > >> There is an error return path that is not kfree'ing mh after
> > >> it has been successfully allocates.  Fix this by free'ing it.
> > >>
> > >> Addresses-Coverity: ("Resource leak")
> > >> Fixes: a070a91cf140 ("fs: dlm: add more midcomms hooks")
> > >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > >> ---
> > >>  fs/dlm/rcom.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/fs/dlm/rcom.c b/fs/dlm/rcom.c
> > >> index 085f21966c72..19298edc1573 100644
> > >> --- a/fs/dlm/rcom.c
> > >> +++ b/fs/dlm/rcom.c
> > >> @@ -393,6 +393,7 @@ static void receive_rcom_lookup(struct dlm_ls *ls, struct dlm_rcom *rc_in)
> > >>    if (rc_in->rc_id == 0xFFFFFFFF) {
> > >>            log_error(ls, "receive_rcom_lookup dump from %d", nodeid);
> > >>            dlm_dump_rsb_name(ls, rc_in->rc_buf, len);
> > >> +          kfree(mh);
> > >
> > > Am I looking at the same code as you?  (I often am not able to review
> > > your patches because you're doing development on stuff that hasn't hit
> > > linux-next).  Anyway, to me this doesn't seem like the correct fix at
> > > all.  There are some other things to free and the "mh" pointer is on
> > > a bunch of lists so it leads to use after frees.
>       ^^^^^^^^^^^^^^
> This is sort of impossible, of course, because the struct only has one
> list_head so it can only be in one list and not a "bunch of lists".
>

It is a bunch of lists because mh_handle holds pointers with ref
counters to other structures which are part of lists. :) There is a
list_del() only if hits zero.

> The dlm code seems to be going out of its way to use void pointers and
> that makes it difficult to parse with Smatch.
>

That has been changed on dlm/next. There exists a struct mh_handle *
and a dlm_msg * to get rid of void * handles.

> But in other subsystems, we could make it a rule that list_heads are
> "poison" "init" or "added".  If you freed a memory with an "added"
> list_head then print a warning.  Or if you added a list_head but it was
> already in the added state then print a warning.  Another idea is that
> if you freed a struct mh before the mh->page allocation was freed then
> print a warning about the leak.  This one is probably more prone to
> false positives but there might be workarounds for those.  #IdeasToImplement
>

Currently if a buffer is allocated it is not possible to free it
again. The allocated buffer of the page will be transmitted
(kernel_sendpage()) out in a contiguous way. If somebody wants to
release memory the page buffer needs to be reordered and it can only
be done before commit().

- Alex