[PATCH] makedumpfile: fix free partial_bitmap2 error

[PATCH] makedumpfile: fix free partial_bitmap2 error

[Arthur Zou]

Description:
In create_dump_bitmap, after prepare_bitmap2_buffer_cyclic was invoked,
info->partial_bitmap2 will pointed to a block of contiguous memory. But
free it in a wrong way because what free_bitmap2_buffer() free is
info->bitmap2 not info->partial_bitmap2.

Signed-off-by: Arthur Zou <zzou@redhat.com>
---
 makedumpfile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index ce4a866..f0d2997 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -5143,7 +5143,8 @@ create_dump_bitmap(void)
 
 			info->num_dumpable = get_num_dumpable_cyclic();
 
-			free_bitmap2_buffer();
+			if (info->partial_bitmap2 != NULL)
+				free(info->partial_bitmap2);
 		}
 
 	} else {
-- 
1.8.4.2


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: fix free partial_bitmap2 error

[Baoquan He]

On 04/25/14 at 09:43am, Arthur Zou wrote:
> Description:
> In create_dump_bitmap, after prepare_bitmap2_buffer_cyclic was invoked,
> info->partial_bitmap2 will pointed to a block of contiguous memory. But
> free it in a wrong way because what free_bitmap2_buffer() free is
> info->bitmap2 not info->partial_bitmap2.
> 
> Signed-off-by: Arthur Zou <zzou@redhat.com>
> ---
>  makedumpfile.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index ce4a866..f0d2997 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -5143,7 +5143,8 @@ create_dump_bitmap(void)
>  
>  			info->num_dumpable = get_num_dumpable_cyclic();
>  
> -			free_bitmap2_buffer();
> +			if (info->partial_bitmap2 != NULL)
> +				free(info->partial_bitmap2);

ACK

Hi Atsushi,

Maybe in my case which the lzo dump random failure triggered by this
one.  Because for elf dump, since the wrong calculation of
cyclic_bufsize is corrected, OOM never happened. For this bug, it didn't
happen either after the fix applied in this patch.

So I guess the 80% of free memory is a safe value, though it's very
close to the OOM threshold.

Thanks
Baoquan


>  		}
>  
>  	} else {
> -- 
> 1.8.4.2
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: fix free partial_bitmap2 error

[Baoquan He]

On 04/28/14 at 02:21pm, Baoquan He wrote:
> On 04/25/14 at 09:43am, Arthur Zou wrote:
> > Description:
> > In create_dump_bitmap, after prepare_bitmap2_buffer_cyclic was invoked,
> > info->partial_bitmap2 will pointed to a block of contiguous memory. But
> > free it in a wrong way because what free_bitmap2_buffer() free is
> > info->bitmap2 not info->partial_bitmap2.
> > 
> > Signed-off-by: Arthur Zou <zzou@redhat.com>
> > ---
> >  makedumpfile.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index ce4a866..f0d2997 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -5143,7 +5143,8 @@ create_dump_bitmap(void)
> >  
> >  			info->num_dumpable = get_num_dumpable_cyclic();
> >  
> > -			free_bitmap2_buffer();
> > +			if (info->partial_bitmap2 != NULL)
> > +				free(info->partial_bitmap2);
> 
> ACK
> 
> Hi Atsushi,
> 
> Maybe in my case which the lzo dump random failure triggered by this
> one.  Because for elf dump, since the wrong calculation of
> cyclic_bufsize is corrected, OOM never happened. For this bug, it didn't
> happen either after the fix applied in this patch.
> 

Well, I was wrong. It can't prove 80% of free memory is a safe value,
espeically for my case. Please ignore below paragraph, it's not clear in
my head.

> So I guess the 80% of free memory is a safe value, though it's very
> close to the OOM threshold.


> 
> Thanks
> Baoquan
> 
> 
> >  		}
> >  
> >  	} else {
> > -- 
> > 1.8.4.2
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: fix free partial_bitmap2 error

[Atsushi Kumagai]

>Description:
>In create_dump_bitmap, after prepare_bitmap2_buffer_cyclic was invoked,
>info->partial_bitmap2 will pointed to a block of contiguous memory. But
>free it in a wrong way because what free_bitmap2_buffer() free is
>info->bitmap2 not info->partial_bitmap2.
>
>Signed-off-by: Arthur Zou <zzou@redhat.com>

Good catch, Thanks!

>---
> makedumpfile.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index ce4a866..f0d2997 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -5143,7 +5143,8 @@ create_dump_bitmap(void)
>
> 			info->num_dumpable = get_num_dumpable_cyclic();
>
>-			free_bitmap2_buffer();
>+			if (info->partial_bitmap2 != NULL)
>+				free(info->partial_bitmap2);

I think it's better to create free_bitmap2_buffer_cyclic() for this.

BTW, write_kdump_pages_and_bitmap_cyclic() also have the same mistake:

        struct cycle cycle = {0};
        for_each_cycle(0, info->max_mapnr, &cycle)
        {
                if (!create_1st_bitmap_cyclic(&cycle))
                        return FALSE;
                if (!write_kdump_bitmap1_cyclic(&cycle))
                        return FALSE;
        }


        free_bitmap1_buffer();

What we should do here is to free partial_bitmap1, so we should
create free_bitmap1_buffer_cyclic() and call it here instead.
Luckily, this mistake is harmless due to the free() at the end
of main():

                if (info->partial_bitmap1 != NULL)
                        free(info->partial_bitmap1);
                if (info->partial_bitmap2 != NULL)
                        free(info->partial_bitmap2);

Now, I think it's better to remove these free(), it's possible
if partial_bitmap1 and partial_bitmap2 are freed in
write_kdump_pages_and_bitmap_cyclic() or write_elf_pages_cyclic().

So could you fix your patch based on my comments?


Thanks
Atsushi Kumagai

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec