[PATCH 0/3] makedumpfile: fix segfault with -X in XEN environment

[PATCH 0/3] makedumpfile: fix segfault with -X in XEN environment

[Martin Wilck]

exclude_xen4_user_domain()i calls clear_bit_on_2nd_bitmap(pfn, NULL)
to exclude domU ranges. This resolves to

  set_bitmap(info->bitmap2, pfn, 0, NULL)
  -> set_bitmap_buffer(info->bitmap2, pfn, 0, NULL)  (because bitmap2->fd == 0)
     ==> segfault, set_bitmap_buffer can't handle NULL as cycle pointer.

If non-cyclic approach is used (always under XEN AFAICS), makedumpfile needs a 
bitmap fd to avoid this crash. But info->flag_cyclic can change after 
open_dump_bitmap() is called.

This patch series fixes that by moving the call to open_dump_bitmap() after
the call to initial(). Tested successfully on both Linux and XEN, x86_64.

Also submitted to https://sourceforge.net/p/makedumpfile/patches/215/

Martin Wilck (3):
  open_dump_bitmap: open bitmap file in non-cyclic case
  move call to open_dump_bitmap() to after call to initial()
  close_dump_bitmap: simplify logic

 makedumpfile.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
2.9.2


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

[PATCH 1/3] open_dump_bitmap: open bitmap file in non-cyclic case

[Martin Wilck]

The logic of set_bitmap() requires that a bitmap fd exists in the
non-cyclic case.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 853b999..9f05396 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1365,7 +1365,7 @@ open_dump_bitmap(void)
 
 	/* Unnecessary to open */
 	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	    && !info->flag_sadump && !info->flag_mem_usage && info->flag_cyclic)
 		return TRUE;
 
 	tmpname = getenv("TMPDIR");
-- 
2.9.2


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

[PATCH 2/3] move call to open_dump_bitmap() to after call to initial()

[Martin Wilck]

When open_files_for_creating_dumpfile() is called, we don't have all
necessary information to determine whether a bitmap file is actually
needed. In particular, we don't know whether info->flag_cyclic will
ultimately be set. This patch moves the call to open_dump_bitmap()
to after initialize() when all flags are known.

For the dump_dmesg() path, no bitmap file is needed.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 9f05396..43278f1 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1480,9 +1480,6 @@ open_files_for_creating_dumpfile(void)
 	if (!open_dump_memory())
 		return FALSE;
 
-	if (!open_dump_bitmap())
-		return FALSE;
-
 	return TRUE;
 }
 
@@ -9708,6 +9705,9 @@ create_dumpfile(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	/* create an array of translations from pfn to vmemmap pages */
 	if (info->flag_excludevm) {
 		if (find_vmemmap() == FAILED) {
@@ -10878,6 +10878,9 @@ int show_mem_usage(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	if (!prepare_bitmap_buffer())
 		return FALSE;
 
-- 
2.9.2


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

[PATCH 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

The boolean expression replicates the logic of open_dump_bitmap().
It's simpler and less error-prone to simply check if fd_bitmap is
valid.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 43278f1..771ab7c 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8579,8 +8579,7 @@ close_dump_file(void)
 void
 close_dump_bitmap(void)
 {
-	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	if (!info->fd_bitmap)
 		return;
 
 	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
-- 
2.9.2


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

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Petr Tesarik]

On Wed, 10 Aug 2016 14:56:58 +0200
Martin Wilck <mwilck@suse.de> wrote:

> The boolean expression replicates the logic of open_dump_bitmap().
> It's simpler and less error-prone to simply check if fd_bitmap is
> valid.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.de>
> ---
>  makedumpfile.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/makedumpfile.c b/makedumpfile.c
> index 43278f1..771ab7c 100644
> --- a/makedumpfile.c
> +++ b/makedumpfile.c
> @@ -8579,8 +8579,7 @@ close_dump_file(void)
>  void
>  close_dump_bitmap(void)
>  {
> -	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
> -	    && !info->flag_sadump && !info->flag_mem_usage)
> +	if (!info->fd_bitmap)

Strictly speaking, zero is a valid FD. I can see that it is highly
unlikely to be the bitmap FD, but it would be a nice cleanup to
initialize fd_bitmap to a negative number and check info->fd_bitmap < 0.
I'm just not sure where to put the initializition...

OTOH I know I'm asking you to fix something that you didn't break.

Petr T

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

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

On Wed, 2016-08-10 at 15:08 +0200, Petr Tesarik wrote:
> On Wed, 10 Aug 2016 14:56:58 +0200
> Martin Wilck <mwilck@suse.de> wrote:
> 
> > The boolean expression replicates the logic of open_dump_bitmap().
> > It's simpler and less error-prone to simply check if fd_bitmap is
> > valid.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.de>
> > ---
> >  makedumpfile.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index 43278f1..771ab7c 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
> >  void
> >  close_dump_bitmap(void)
> >  {
> > -	if (!info->working_dir && !info->flag_reassemble && !info-
> > >flag_refiltering
> > -	    && !info->flag_sadump && !info->flag_mem_usage)
> > +	if (!info->fd_bitmap)
> 
> Strictly speaking, zero is a valid FD. I can see that it is highly
> unlikely to be the bitmap FD, but it would be a nice cleanup to
> initialize fd_bitmap to a negative number and check info->fd_bitmap <
> 0.
> I'm just not sure where to put the initializition...


> > OTOH I know I'm asking you to fix something that you didn't break.

I had the same thought, and the same excuse not to address it in this
patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
checks like "if (info->fd_bitmap)". I just followed that pattern for
now.

Martin

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

RE: [PATCH 3/3] close_dump_bitmap: simplify logic

[Atsushi Kumagai]

>> > The boolean expression replicates the logic of open_dump_bitmap().
>> > It's simpler and less error-prone to simply check if fd_bitmap is
>> > valid.
>> >
>> > Signed-off-by: Martin Wilck <mwilck@suse.de>
>> > ---
>> >  makedumpfile.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/makedumpfile.c b/makedumpfile.c
>> > index 43278f1..771ab7c 100644
>> > --- a/makedumpfile.c
>> > +++ b/makedumpfile.c
>> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
>> >  void
>> >  close_dump_bitmap(void)
>> >  {
>> > -	if (!info->working_dir && !info->flag_reassemble && !info-
>> > >flag_refiltering
>> > -	    && !info->flag_sadump && !info->flag_mem_usage)
>> > +	if (!info->fd_bitmap)
>>
>> Strictly speaking, zero is a valid FD. I can see that it is highly
>> unlikely to be the bitmap FD, but it would be a nice cleanup to
>> initialize fd_bitmap to a negative number and check info->fd_bitmap <
>> 0.
>> I'm just not sure where to put the initializition...
>
>
>> > OTOH I know I'm asking you to fix something that you didn't break.
>
>I had the same thought, and the same excuse not to address it in this
>patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
>checks like "if (info->fd_bitmap)". I just followed that pattern for
>now.

I see, it would be better to make the checks strict on this occasion.
So, could you work for that cleanup before your three patches as an
additional cleanup patch ?


Thanks,
Atsushi Kumagai
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Petr Tesarik]

On Tue, 16 Aug 2016 00:37:09 +0000
Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:

> >> > The boolean expression replicates the logic of open_dump_bitmap().
> >> > It's simpler and less error-prone to simply check if fd_bitmap is
> >> > valid.
> >> >
> >> > Signed-off-by: Martin Wilck <mwilck@suse.de>
> >> > ---
> >> >  makedumpfile.c | 3 +--
> >> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >
> >> > diff --git a/makedumpfile.c b/makedumpfile.c
> >> > index 43278f1..771ab7c 100644
> >> > --- a/makedumpfile.c
> >> > +++ b/makedumpfile.c
> >> > @@ -8579,8 +8579,7 @@ close_dump_file(void)
> >> >  void
> >> >  close_dump_bitmap(void)
> >> >  {
> >> > -	if (!info->working_dir && !info->flag_reassemble && !info-
> >> > >flag_refiltering
> >> > -	    && !info->flag_sadump && !info->flag_mem_usage)
> >> > +	if (!info->fd_bitmap)
> >>
> >> Strictly speaking, zero is a valid FD. I can see that it is highly
> >> unlikely to be the bitmap FD, but it would be a nice cleanup to
> >> initialize fd_bitmap to a negative number and check info->fd_bitmap <
> >> 0.
> >> I'm just not sure where to put the initializition...
> >
> >
> >> > OTOH I know I'm asking you to fix something that you didn't break.
> >
> >I had the same thought, and the same excuse not to address it in this
> >patch set. If you grep makedumpfile.c for "fd_bitmap", you'll see many
> >checks like "if (info->fd_bitmap)". I just followed that pattern for
> >now.
> 
> I see, it would be better to make the checks strict on this occasion.
> So, could you work for that cleanup before your three patches as an
> additional cleanup patch ?

OK, I take it. ;-)

Martin, do you mind rebasing your patch when I'm done with the cleanup?

Petr T

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

Re: [PATCH 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

On Tue, 2016-08-16 at 07:59 +0200, Petr Tesarik wrote:
> On Tue, 16 Aug 2016 00:37:09 +0000
> Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:
> 
> > So, could you work for that cleanup before your three patches as an
> > additional cleanup patch ?
> 
> OK, I take it. ;-)
> 
> Martin, do you mind rebasing your patch when I'm done with the
> cleanup?

No problem.

Regards
Martin



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

[PATCH] Cleanup: Use a negative number for uninitialized file descriptors

[Petr Tesarik]

Do not use zero to denote an invalid file descriptor.

First, zero is a valid value, although quite unlikely to be used for
anything except standard input.

Second, open(2) returns a negative value on failure, so there are
already checks for a negative value in some places.

The purpose of this patch is not to allow running in an evil environment
(with closed stdin), but to aid in debugging by using a consistent value
for uninitialized file descriptors which is also regarded as invalid by
the kernel. For example, attempts to close a negative FD will fail
(unlike an attempt to close FD 0).

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 makedumpfile.c | 68 +++++++++++++++++++++++++++++++++-------------------------
 makedumpfile.h |  2 +-
 2 files changed, 40 insertions(+), 30 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 21784e8..d168dfd 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3730,10 +3730,10 @@ free_for_parallel()
 		return;
 
 	for (i = 0; i < info->num_threads; i++) {
-		if (FD_MEMORY_PARALLEL(i) > 0)
+		if (FD_MEMORY_PARALLEL(i) >= 0)
 			close(FD_MEMORY_PARALLEL(i));
 
-		if (FD_BITMAP_MEMORY_PARALLEL(i) > 0)
+		if (FD_BITMAP_MEMORY_PARALLEL(i) >= 0)
 			close(FD_BITMAP_MEMORY_PARALLEL(i));
 	}
 }
@@ -4038,13 +4038,13 @@ out:
 void
 initialize_bitmap(struct dump_bitmap *bitmap)
 {
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		bitmap->fd        = info->fd_bitmap;
 		bitmap->file_name = info->name_bitmap;
 		bitmap->no_block  = -1;
 		memset(bitmap->buf, 0, BUFSIZE_BITMAP);
 	} else {
-		bitmap->fd        = 0;
+		bitmap->fd        = -1;
 		bitmap->file_name = NULL;
 		bitmap->no_block  = -1;
 		memset(bitmap->buf, 0, info->bufsize_cyclic);
@@ -4154,7 +4154,7 @@ set_bitmap_buffer(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cyc
 int
 set_bitmap(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cycle *cycle)
 {
-	if (bitmap->fd) {
+	if (bitmap->fd >= 0) {
 		return set_bitmap_file(bitmap, pfn, val);
 	} else {
 		return set_bitmap_buffer(bitmap, pfn, val, cycle);
@@ -4170,7 +4170,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
 	/*
 	 * The bitmap doesn't have the fd, it's a on-memory bitmap.
 	 */
-	if (bitmap->fd == 0)
+	if (bitmap->fd < 0)
 		return TRUE;
 	/*
 	 * The bitmap buffer is not dirty, and it is not necessary
@@ -5403,7 +5403,7 @@ create_1st_bitmap_buffer(struct cycle *cycle)
 int
 create_1st_bitmap(struct cycle *cycle)
 {
-	if (info->bitmap1->fd) {
+	if (info->bitmap1->fd >= 0) {
 		return create_1st_bitmap_file();
 	} else {
 		return create_1st_bitmap_buffer(cycle);
@@ -5414,7 +5414,7 @@ static inline int
 is_in_segs(unsigned long long paddr)
 {
 	if (info->flag_refiltering || info->flag_sadump) {
-		if (info->bitmap1->fd == 0) {
+		if (info->bitmap1->fd < 0) {
 			initialize_1st_bitmap(info->bitmap1);
 			create_1st_bitmap_file();
 		}
@@ -5872,7 +5872,7 @@ copy_bitmap_file(void)
 int
 copy_bitmap(void)
 {
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		return copy_bitmap_file();
 	} else {
 		return copy_bitmap_buffer();
@@ -6313,7 +6313,7 @@ prepare_bitmap1_buffer(void)
 		return FALSE;
 	}
 
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		if ((info->bitmap1->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
 			ERRMSG("Can't allocate memory for the 1st bitmaps's buffer. %s\n",
 			       strerror(errno));
@@ -6352,7 +6352,7 @@ prepare_bitmap2_buffer(void)
 		       strerror(errno));
 		return FALSE;
 	}
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		if ((info->bitmap2->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
 			ERRMSG("Can't allocate memory for the 2nd bitmaps's buffer. %s\n",
 			       strerror(errno));
@@ -7582,7 +7582,7 @@ kdump_thread_function_cyclic(void *arg) {
 
 	fd_memory = FD_MEMORY_PARALLEL(kdump_thread_args->thread_num);
 
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		bitmap_parallel.buf = malloc(BUFSIZE_BITMAP);
 		if (bitmap_parallel.buf == NULL){
 			ERRMSG("Can't allocate memory for bitmap_parallel.buf. %s\n",
@@ -7628,7 +7628,7 @@ kdump_thread_function_cyclic(void *arg) {
 			pthread_mutex_lock(&info->current_pfn_mutex);
 			for (pfn = info->current_pfn; pfn < cycle->end_pfn; pfn++) {
 				dumpable = is_dumpable(
-					info->fd_bitmap ? &bitmap_parallel : info->bitmap2,
+					info->fd_bitmap >= 0 ? &bitmap_parallel : info->bitmap2,
 					pfn,
 					cycle);
 				if (dumpable)
@@ -7723,7 +7723,7 @@ next:
 	retval = NULL;
 
 fail:
-	if (bitmap_memory_parallel.fd > 0)
+	if (bitmap_memory_parallel.fd >= 0)
 		close(bitmap_memory_parallel.fd);
 	if (bitmap_parallel.buf != NULL)
 		free(bitmap_parallel.buf);
@@ -8461,7 +8461,7 @@ out:
 
 int
 write_kdump_bitmap1(struct cycle *cycle) {
-	if (info->bitmap1->fd) {
+	if (info->bitmap1->fd >= 0) {
 		return write_kdump_bitmap1_file();
 	} else {
 		return write_kdump_bitmap1_buffer(cycle);
@@ -8470,7 +8470,7 @@ write_kdump_bitmap1(struct cycle *cycle) {
 
 int
 write_kdump_bitmap2(struct cycle *cycle) {
-	if (info->bitmap2->fd) {
+	if (info->bitmap2->fd >= 0) {
 		return write_kdump_bitmap2_file();
 	} else {
 		return write_kdump_bitmap2_buffer(cycle);
@@ -8597,9 +8597,10 @@ close_vmcoreinfo(void)
 void
 close_dump_memory(void)
 {
-	if ((info->fd_memory = close(info->fd_memory)) < 0)
+	if (close(info->fd_memory) < 0)
 		ERRMSG("Can't close the dump memory(%s). %s\n",
 		    info->name_memory, strerror(errno));
+	info->fd_memory = -1;
 }
 
 void
@@ -8608,9 +8609,10 @@ close_dump_file(void)
 	if (info->flag_flatten)
 		return;
 
-	if ((info->fd_dumpfile = close(info->fd_dumpfile)) < 0)
+	if (close(info->fd_dumpfile) < 0)
 		ERRMSG("Can't close the dump file(%s). %s\n",
 		    info->name_dumpfile, strerror(errno));
+	info->fd_dumpfile = -1;
 }
 
 void
@@ -8620,9 +8622,10 @@ close_dump_bitmap(void)
 	    && !info->flag_sadump && !info->flag_mem_usage)
 		return;
 
-	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
+	if (close(info->fd_bitmap) < 0)
 		ERRMSG("Can't close the bitmap file(%s). %s\n",
 		    info->name_bitmap, strerror(errno));
+	info->fd_bitmap = -1;
 	free(info->name_bitmap);
 	info->name_bitmap = NULL;
 }
@@ -8631,16 +8634,18 @@ void
 close_kernel_file(void)
 {
 	if (info->name_vmlinux) {
-		if ((info->fd_vmlinux = close(info->fd_vmlinux)) < 0) {
+		if (close(info->fd_vmlinux) < 0) {
 			ERRMSG("Can't close the kernel file(%s). %s\n",
 			    info->name_vmlinux, strerror(errno));
 		}
+		info->fd_vmlinux = -1;
 	}
 	if (info->name_xen_syms) {
-		if ((info->fd_xen_syms = close(info->fd_xen_syms)) < 0) {
+		if (close(info->fd_xen_syms) < 0) {
 			ERRMSG("Can't close the kernel file(%s). %s\n",
 			    info->name_xen_syms, strerror(errno));
 		}
+		info->fd_xen_syms = -1;
 	}
 }
 
@@ -10202,7 +10207,7 @@ reassemble_kdump_header(void)
 
 	ret = TRUE;
 out:
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 	free(buf_bitmap);
 
@@ -10212,7 +10217,7 @@ out:
 int
 reassemble_kdump_pages(void)
 {
-	int i, fd = 0, ret = FALSE;
+	int i, fd = -1, ret = FALSE;
 	off_t offset_first_ph, offset_ph_org, offset_eraseinfo;
 	off_t offset_data_new, offset_zero_page = 0;
 	mdf_pfn_t pfn, start_pfn, end_pfn;
@@ -10336,7 +10341,7 @@ reassemble_kdump_pages(void)
 			offset_data_new += pd.size;
 		}
 		close(fd);
-		fd = 0;
+		fd = -1;
 	}
 	if (!write_cache_bufsz(&cd_pd))
 		goto out;
@@ -10379,7 +10384,7 @@ reassemble_kdump_pages(void)
 		size_eraseinfo += SPLITTING_SIZE_EI(i);
 
 		close(fd);
-		fd = 0;
+		fd = -1;
 	}
 	if (size_eraseinfo) {
 		if (!write_cache_bufsz(&cd_data))
@@ -10400,7 +10405,7 @@ out:
 
 	if (data)
 		free(data);
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 
 	return ret;
@@ -10973,6 +10978,11 @@ main(int argc, char *argv[])
 		    strerror(errno));
 		goto out;
 	}
+	info->fd_vmlinux = -1;
+	info->fd_xen_syms = -1;
+	info->fd_memory = -1;
+	info->fd_dumpfile = -1;
+	info->fd_bitmap = -1;
 	initialize_tables();
 
 	/*
@@ -11268,11 +11278,11 @@ out:
 				free(info->bitmap_memory->buf);
 			free(info->bitmap_memory);
 		}
-		if (info->fd_memory)
+		if (info->fd_memory >= 0)
 			close(info->fd_memory);
-		if (info->fd_dumpfile)
+		if (info->fd_dumpfile >= 0)
 			close(info->fd_dumpfile);
-		if (info->fd_bitmap)
+		if (info->fd_bitmap >= 0)
 			close(info->fd_bitmap);
 		if (vt.node_online_map != NULL)
 			free(vt.node_online_map);
diff --git a/makedumpfile.h b/makedumpfile.h
index 533e5b8..1814139 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1955,7 +1955,7 @@ is_dumpable_file(struct dump_bitmap *bitmap, mdf_pfn_t pfn)
 static inline int
 is_dumpable(struct dump_bitmap *bitmap, mdf_pfn_t pfn, struct cycle *cycle)
 {
-	if (bitmap->fd == 0) {
+	if (bitmap->fd < 0) {
 		return is_dumpable_buffer(bitmap, pfn, cycle);
 	} else {
 		return is_dumpable_file(bitmap, pfn);
-- 
2.6.6


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

RE: [PATCH] Cleanup: Use a negative number for uninitialized file descriptors

[Atsushi Kumagai]

Hello Petr,

>Do not use zero to denote an invalid file descriptor.
>
>First, zero is a valid value, although quite unlikely to be used for
>anything except standard input.
>
>Second, open(2) returns a negative value on failure, so there are
>already checks for a negative value in some places.
>
>The purpose of this patch is not to allow running in an evil environment
>(with closed stdin), but to aid in debugging by using a consistent value
>for uninitialized file descriptors which is also regarded as invalid by
>the kernel. For example, attempts to close a negative FD will fail
>(unlike an attempt to close FD 0).
>
>Signed-off-by: Petr Tesarik <ptesarik@suse.com>

Good, thanks for your work, but could you fix
more two points as below ?


dwarf_info.c::
   1595         if (dwarf_info.module_name
   1596                         && strcmp(dwarf_info.module_name, "vmlinux")
   1597                         && strcmp(dwarf_info.module_name, "xen-syms")) {
   1598                 if (dwarf_info.fd_debuginfo > 0)                 // should be '>= 0'
   1599                         close(dwarf_info.fd_debuginfo);

sadump_info.c::
   1919                 for (i = 1; i < si->num_disks; ++i) {
   1920                         if (si->diskset_info[i].fd_memory)      // should be 'fd_memory >=0'
   1921                                 close(si->diskset_info[i].fd_memory);


Thanks,
Atsushi Kumagai

>---
> makedumpfile.c | 68 +++++++++++++++++++++++++++++++++-------------------------
> makedumpfile.h |  2 +-
> 2 files changed, 40 insertions(+), 30 deletions(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 21784e8..d168dfd 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -3730,10 +3730,10 @@ free_for_parallel()
> 		return;
>
> 	for (i = 0; i < info->num_threads; i++) {
>-		if (FD_MEMORY_PARALLEL(i) > 0)
>+		if (FD_MEMORY_PARALLEL(i) >= 0)
> 			close(FD_MEMORY_PARALLEL(i));
>
>-		if (FD_BITMAP_MEMORY_PARALLEL(i) > 0)
>+		if (FD_BITMAP_MEMORY_PARALLEL(i) >= 0)
> 			close(FD_BITMAP_MEMORY_PARALLEL(i));
> 	}
> }
>@@ -4038,13 +4038,13 @@ out:
> void
> initialize_bitmap(struct dump_bitmap *bitmap)
> {
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		bitmap->fd        = info->fd_bitmap;
> 		bitmap->file_name = info->name_bitmap;
> 		bitmap->no_block  = -1;
> 		memset(bitmap->buf, 0, BUFSIZE_BITMAP);
> 	} else {
>-		bitmap->fd        = 0;
>+		bitmap->fd        = -1;
> 		bitmap->file_name = NULL;
> 		bitmap->no_block  = -1;
> 		memset(bitmap->buf, 0, info->bufsize_cyclic);
>@@ -4154,7 +4154,7 @@ set_bitmap_buffer(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cyc
> int
> set_bitmap(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cycle *cycle)
> {
>-	if (bitmap->fd) {
>+	if (bitmap->fd >= 0) {
> 		return set_bitmap_file(bitmap, pfn, val);
> 	} else {
> 		return set_bitmap_buffer(bitmap, pfn, val, cycle);
>@@ -4170,7 +4170,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
> 	/*
> 	 * The bitmap doesn't have the fd, it's a on-memory bitmap.
> 	 */
>-	if (bitmap->fd == 0)
>+	if (bitmap->fd < 0)
> 		return TRUE;
> 	/*
> 	 * The bitmap buffer is not dirty, and it is not necessary
>@@ -5403,7 +5403,7 @@ create_1st_bitmap_buffer(struct cycle *cycle)
> int
> create_1st_bitmap(struct cycle *cycle)
> {
>-	if (info->bitmap1->fd) {
>+	if (info->bitmap1->fd >= 0) {
> 		return create_1st_bitmap_file();
> 	} else {
> 		return create_1st_bitmap_buffer(cycle);
>@@ -5414,7 +5414,7 @@ static inline int
> is_in_segs(unsigned long long paddr)
> {
> 	if (info->flag_refiltering || info->flag_sadump) {
>-		if (info->bitmap1->fd == 0) {
>+		if (info->bitmap1->fd < 0) {
> 			initialize_1st_bitmap(info->bitmap1);
> 			create_1st_bitmap_file();
> 		}
>@@ -5872,7 +5872,7 @@ copy_bitmap_file(void)
> int
> copy_bitmap(void)
> {
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		return copy_bitmap_file();
> 	} else {
> 		return copy_bitmap_buffer();
>@@ -6313,7 +6313,7 @@ prepare_bitmap1_buffer(void)
> 		return FALSE;
> 	}
>
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		if ((info->bitmap1->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> 			ERRMSG("Can't allocate memory for the 1st bitmaps's buffer. %s\n",
> 			       strerror(errno));
>@@ -6352,7 +6352,7 @@ prepare_bitmap2_buffer(void)
> 		       strerror(errno));
> 		return FALSE;
> 	}
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		if ((info->bitmap2->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> 			ERRMSG("Can't allocate memory for the 2nd bitmaps's buffer. %s\n",
> 			       strerror(errno));
>@@ -7582,7 +7582,7 @@ kdump_thread_function_cyclic(void *arg) {
>
> 	fd_memory = FD_MEMORY_PARALLEL(kdump_thread_args->thread_num);
>
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		bitmap_parallel.buf = malloc(BUFSIZE_BITMAP);
> 		if (bitmap_parallel.buf == NULL){
> 			ERRMSG("Can't allocate memory for bitmap_parallel.buf. %s\n",
>@@ -7628,7 +7628,7 @@ kdump_thread_function_cyclic(void *arg) {
> 			pthread_mutex_lock(&info->current_pfn_mutex);
> 			for (pfn = info->current_pfn; pfn < cycle->end_pfn; pfn++) {
> 				dumpable = is_dumpable(
>-					info->fd_bitmap ? &bitmap_parallel : info->bitmap2,
>+					info->fd_bitmap >= 0 ? &bitmap_parallel : info->bitmap2,
> 					pfn,
> 					cycle);
> 				if (dumpable)
>@@ -7723,7 +7723,7 @@ next:
> 	retval = NULL;
>
> fail:
>-	if (bitmap_memory_parallel.fd > 0)
>+	if (bitmap_memory_parallel.fd >= 0)
> 		close(bitmap_memory_parallel.fd);
> 	if (bitmap_parallel.buf != NULL)
> 		free(bitmap_parallel.buf);
>@@ -8461,7 +8461,7 @@ out:
>
> int
> write_kdump_bitmap1(struct cycle *cycle) {
>-	if (info->bitmap1->fd) {
>+	if (info->bitmap1->fd >= 0) {
> 		return write_kdump_bitmap1_file();
> 	} else {
> 		return write_kdump_bitmap1_buffer(cycle);
>@@ -8470,7 +8470,7 @@ write_kdump_bitmap1(struct cycle *cycle) {
>
> int
> write_kdump_bitmap2(struct cycle *cycle) {
>-	if (info->bitmap2->fd) {
>+	if (info->bitmap2->fd >= 0) {
> 		return write_kdump_bitmap2_file();
> 	} else {
> 		return write_kdump_bitmap2_buffer(cycle);
>@@ -8597,9 +8597,10 @@ close_vmcoreinfo(void)
> void
> close_dump_memory(void)
> {
>-	if ((info->fd_memory = close(info->fd_memory)) < 0)
>+	if (close(info->fd_memory) < 0)
> 		ERRMSG("Can't close the dump memory(%s). %s\n",
> 		    info->name_memory, strerror(errno));
>+	info->fd_memory = -1;
> }
>
> void
>@@ -8608,9 +8609,10 @@ close_dump_file(void)
> 	if (info->flag_flatten)
> 		return;
>
>-	if ((info->fd_dumpfile = close(info->fd_dumpfile)) < 0)
>+	if (close(info->fd_dumpfile) < 0)
> 		ERRMSG("Can't close the dump file(%s). %s\n",
> 		    info->name_dumpfile, strerror(errno));
>+	info->fd_dumpfile = -1;
> }
>
> void
>@@ -8620,9 +8622,10 @@ close_dump_bitmap(void)
> 	    && !info->flag_sadump && !info->flag_mem_usage)
> 		return;
>
>-	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
>+	if (close(info->fd_bitmap) < 0)
> 		ERRMSG("Can't close the bitmap file(%s). %s\n",
> 		    info->name_bitmap, strerror(errno));
>+	info->fd_bitmap = -1;
> 	free(info->name_bitmap);
> 	info->name_bitmap = NULL;
> }
>@@ -8631,16 +8634,18 @@ void
> close_kernel_file(void)
> {
> 	if (info->name_vmlinux) {
>-		if ((info->fd_vmlinux = close(info->fd_vmlinux)) < 0) {
>+		if (close(info->fd_vmlinux) < 0) {
> 			ERRMSG("Can't close the kernel file(%s). %s\n",
> 			    info->name_vmlinux, strerror(errno));
> 		}
>+		info->fd_vmlinux = -1;
> 	}
> 	if (info->name_xen_syms) {
>-		if ((info->fd_xen_syms = close(info->fd_xen_syms)) < 0) {
>+		if (close(info->fd_xen_syms) < 0) {
> 			ERRMSG("Can't close the kernel file(%s). %s\n",
> 			    info->name_xen_syms, strerror(errno));
> 		}
>+		info->fd_xen_syms = -1;
> 	}
> }
>
>@@ -10202,7 +10207,7 @@ reassemble_kdump_header(void)
>
> 	ret = TRUE;
> out:
>-	if (fd > 0)
>+	if (fd >= 0)
> 		close(fd);
> 	free(buf_bitmap);
>
>@@ -10212,7 +10217,7 @@ out:
> int
> reassemble_kdump_pages(void)
> {
>-	int i, fd = 0, ret = FALSE;
>+	int i, fd = -1, ret = FALSE;
> 	off_t offset_first_ph, offset_ph_org, offset_eraseinfo;
> 	off_t offset_data_new, offset_zero_page = 0;
> 	mdf_pfn_t pfn, start_pfn, end_pfn;
>@@ -10336,7 +10341,7 @@ reassemble_kdump_pages(void)
> 			offset_data_new += pd.size;
> 		}
> 		close(fd);
>-		fd = 0;
>+		fd = -1;
> 	}
> 	if (!write_cache_bufsz(&cd_pd))
> 		goto out;
>@@ -10379,7 +10384,7 @@ reassemble_kdump_pages(void)
> 		size_eraseinfo += SPLITTING_SIZE_EI(i);
>
> 		close(fd);
>-		fd = 0;
>+		fd = -1;
> 	}
> 	if (size_eraseinfo) {
> 		if (!write_cache_bufsz(&cd_data))
>@@ -10400,7 +10405,7 @@ out:
>
> 	if (data)
> 		free(data);
>-	if (fd > 0)
>+	if (fd >= 0)
> 		close(fd);
>
> 	return ret;
>@@ -10973,6 +10978,11 @@ main(int argc, char *argv[])
> 		    strerror(errno));
> 		goto out;
> 	}
>+	info->fd_vmlinux = -1;
>+	info->fd_xen_syms = -1;
>+	info->fd_memory = -1;
>+	info->fd_dumpfile = -1;
>+	info->fd_bitmap = -1;
> 	initialize_tables();
>
> 	/*
>@@ -11268,11 +11278,11 @@ out:
> 				free(info->bitmap_memory->buf);
> 			free(info->bitmap_memory);
> 		}
>-		if (info->fd_memory)
>+		if (info->fd_memory >= 0)
> 			close(info->fd_memory);
>-		if (info->fd_dumpfile)
>+		if (info->fd_dumpfile >= 0)
> 			close(info->fd_dumpfile);
>-		if (info->fd_bitmap)
>+		if (info->fd_bitmap >= 0)
> 			close(info->fd_bitmap);
> 		if (vt.node_online_map != NULL)
> 			free(vt.node_online_map);
>diff --git a/makedumpfile.h b/makedumpfile.h
>index 533e5b8..1814139 100644
>--- a/makedumpfile.h
>+++ b/makedumpfile.h
>@@ -1955,7 +1955,7 @@ is_dumpable_file(struct dump_bitmap *bitmap, mdf_pfn_t pfn)
> static inline int
> is_dumpable(struct dump_bitmap *bitmap, mdf_pfn_t pfn, struct cycle *cycle)
> {
>-	if (bitmap->fd == 0) {
>+	if (bitmap->fd < 0) {
> 		return is_dumpable_buffer(bitmap, pfn, cycle);
> 	} else {
> 		return is_dumpable_file(bitmap, pfn);
>--
>2.6.6

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

Re: [PATCH] Cleanup: Use a negative number for uninitialized file descriptors

[Petr Tesarik]

Hello Atsushi,

On Fri, 9 Sep 2016 06:27:17 +0000
Atsushi Kumagai <ats-kumagai@wm.jp.nec.com> wrote:

> Hello Petr,
> 
> >Do not use zero to denote an invalid file descriptor.
> >
> >First, zero is a valid value, although quite unlikely to be used for
> >anything except standard input.
> >
> >Second, open(2) returns a negative value on failure, so there are
> >already checks for a negative value in some places.
> >
> >The purpose of this patch is not to allow running in an evil environment
> >(with closed stdin), but to aid in debugging by using a consistent value
> >for uninitialized file descriptors which is also regarded as invalid by
> >the kernel. For example, attempts to close a negative FD will fail
> >(unlike an attempt to close FD 0).
> >
> >Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> 
> Good, thanks for your work, but could you fix
> more two points as below ?

I see. I skipped elf_info.c, dwarf_info.c, sadump_info.c and other
files, because the file descriptors there are initialized from other
variables, already checked in the main module, but you're right, the
checks should become consistent.

Expect a version 2 of the patch soon.

Petr T

> dwarf_info.c::
>    1595         if (dwarf_info.module_name
>    1596                         && strcmp(dwarf_info.module_name, "vmlinux")
>    1597                         && strcmp(dwarf_info.module_name, "xen-syms")) {
>    1598                 if (dwarf_info.fd_debuginfo > 0)                 // should be '>= 0'
>    1599                         close(dwarf_info.fd_debuginfo);
> 
> sadump_info.c::
>    1919                 for (i = 1; i < si->num_disks; ++i) {
>    1920                         if (si->diskset_info[i].fd_memory)      // should be 'fd_memory >=0'
>    1921                                 close(si->diskset_info[i].fd_memory);
> 
> 
> Thanks,
> Atsushi Kumagai
> 
> >---
> > makedumpfile.c | 68 +++++++++++++++++++++++++++++++++-------------------------
> > makedumpfile.h |  2 +-
> > 2 files changed, 40 insertions(+), 30 deletions(-)
> >
> >diff --git a/makedumpfile.c b/makedumpfile.c
> >index 21784e8..d168dfd 100644
> >--- a/makedumpfile.c
> >+++ b/makedumpfile.c
> >@@ -3730,10 +3730,10 @@ free_for_parallel()
> > 		return;
> >
> > 	for (i = 0; i < info->num_threads; i++) {
> >-		if (FD_MEMORY_PARALLEL(i) > 0)
> >+		if (FD_MEMORY_PARALLEL(i) >= 0)
> > 			close(FD_MEMORY_PARALLEL(i));
> >
> >-		if (FD_BITMAP_MEMORY_PARALLEL(i) > 0)
> >+		if (FD_BITMAP_MEMORY_PARALLEL(i) >= 0)
> > 			close(FD_BITMAP_MEMORY_PARALLEL(i));
> > 	}
> > }
> >@@ -4038,13 +4038,13 @@ out:
> > void
> > initialize_bitmap(struct dump_bitmap *bitmap)
> > {
> >-	if (info->fd_bitmap) {
> >+	if (info->fd_bitmap >= 0) {
> > 		bitmap->fd        = info->fd_bitmap;
> > 		bitmap->file_name = info->name_bitmap;
> > 		bitmap->no_block  = -1;
> > 		memset(bitmap->buf, 0, BUFSIZE_BITMAP);
> > 	} else {
> >-		bitmap->fd        = 0;
> >+		bitmap->fd        = -1;
> > 		bitmap->file_name = NULL;
> > 		bitmap->no_block  = -1;
> > 		memset(bitmap->buf, 0, info->bufsize_cyclic);
> >@@ -4154,7 +4154,7 @@ set_bitmap_buffer(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cyc
> > int
> > set_bitmap(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cycle *cycle)
> > {
> >-	if (bitmap->fd) {
> >+	if (bitmap->fd >= 0) {
> > 		return set_bitmap_file(bitmap, pfn, val);
> > 	} else {
> > 		return set_bitmap_buffer(bitmap, pfn, val, cycle);
> >@@ -4170,7 +4170,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
> > 	/*
> > 	 * The bitmap doesn't have the fd, it's a on-memory bitmap.
> > 	 */
> >-	if (bitmap->fd == 0)
> >+	if (bitmap->fd < 0)
> > 		return TRUE;
> > 	/*
> > 	 * The bitmap buffer is not dirty, and it is not necessary
> >@@ -5403,7 +5403,7 @@ create_1st_bitmap_buffer(struct cycle *cycle)
> > int
> > create_1st_bitmap(struct cycle *cycle)
> > {
> >-	if (info->bitmap1->fd) {
> >+	if (info->bitmap1->fd >= 0) {
> > 		return create_1st_bitmap_file();
> > 	} else {
> > 		return create_1st_bitmap_buffer(cycle);
> >@@ -5414,7 +5414,7 @@ static inline int
> > is_in_segs(unsigned long long paddr)
> > {
> > 	if (info->flag_refiltering || info->flag_sadump) {
> >-		if (info->bitmap1->fd == 0) {
> >+		if (info->bitmap1->fd < 0) {
> > 			initialize_1st_bitmap(info->bitmap1);
> > 			create_1st_bitmap_file();
> > 		}
> >@@ -5872,7 +5872,7 @@ copy_bitmap_file(void)
> > int
> > copy_bitmap(void)
> > {
> >-	if (info->fd_bitmap) {
> >+	if (info->fd_bitmap >= 0) {
> > 		return copy_bitmap_file();
> > 	} else {
> > 		return copy_bitmap_buffer();
> >@@ -6313,7 +6313,7 @@ prepare_bitmap1_buffer(void)
> > 		return FALSE;
> > 	}
> >
> >-	if (info->fd_bitmap) {
> >+	if (info->fd_bitmap >= 0) {
> > 		if ((info->bitmap1->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> > 			ERRMSG("Can't allocate memory for the 1st bitmaps's buffer. %s\n",
> > 			       strerror(errno));
> >@@ -6352,7 +6352,7 @@ prepare_bitmap2_buffer(void)
> > 		       strerror(errno));
> > 		return FALSE;
> > 	}
> >-	if (info->fd_bitmap) {
> >+	if (info->fd_bitmap >= 0) {
> > 		if ((info->bitmap2->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> > 			ERRMSG("Can't allocate memory for the 2nd bitmaps's buffer. %s\n",
> > 			       strerror(errno));
> >@@ -7582,7 +7582,7 @@ kdump_thread_function_cyclic(void *arg) {
> >
> > 	fd_memory = FD_MEMORY_PARALLEL(kdump_thread_args->thread_num);
> >
> >-	if (info->fd_bitmap) {
> >+	if (info->fd_bitmap >= 0) {
> > 		bitmap_parallel.buf = malloc(BUFSIZE_BITMAP);
> > 		if (bitmap_parallel.buf == NULL){
> > 			ERRMSG("Can't allocate memory for bitmap_parallel.buf. %s\n",
> >@@ -7628,7 +7628,7 @@ kdump_thread_function_cyclic(void *arg) {
> > 			pthread_mutex_lock(&info->current_pfn_mutex);
> > 			for (pfn = info->current_pfn; pfn < cycle->end_pfn; pfn++) {
> > 				dumpable = is_dumpable(
> >-					info->fd_bitmap ? &bitmap_parallel : info->bitmap2,
> >+					info->fd_bitmap >= 0 ? &bitmap_parallel : info->bitmap2,
> > 					pfn,
> > 					cycle);
> > 				if (dumpable)
> >@@ -7723,7 +7723,7 @@ next:
> > 	retval = NULL;
> >
> > fail:
> >-	if (bitmap_memory_parallel.fd > 0)
> >+	if (bitmap_memory_parallel.fd >= 0)
> > 		close(bitmap_memory_parallel.fd);
> > 	if (bitmap_parallel.buf != NULL)
> > 		free(bitmap_parallel.buf);
> >@@ -8461,7 +8461,7 @@ out:
> >
> > int
> > write_kdump_bitmap1(struct cycle *cycle) {
> >-	if (info->bitmap1->fd) {
> >+	if (info->bitmap1->fd >= 0) {
> > 		return write_kdump_bitmap1_file();
> > 	} else {
> > 		return write_kdump_bitmap1_buffer(cycle);
> >@@ -8470,7 +8470,7 @@ write_kdump_bitmap1(struct cycle *cycle) {
> >
> > int
> > write_kdump_bitmap2(struct cycle *cycle) {
> >-	if (info->bitmap2->fd) {
> >+	if (info->bitmap2->fd >= 0) {
> > 		return write_kdump_bitmap2_file();
> > 	} else {
> > 		return write_kdump_bitmap2_buffer(cycle);
> >@@ -8597,9 +8597,10 @@ close_vmcoreinfo(void)
> > void
> > close_dump_memory(void)
> > {
> >-	if ((info->fd_memory = close(info->fd_memory)) < 0)
> >+	if (close(info->fd_memory) < 0)
> > 		ERRMSG("Can't close the dump memory(%s). %s\n",
> > 		    info->name_memory, strerror(errno));
> >+	info->fd_memory = -1;
> > }
> >
> > void
> >@@ -8608,9 +8609,10 @@ close_dump_file(void)
> > 	if (info->flag_flatten)
> > 		return;
> >
> >-	if ((info->fd_dumpfile = close(info->fd_dumpfile)) < 0)
> >+	if (close(info->fd_dumpfile) < 0)
> > 		ERRMSG("Can't close the dump file(%s). %s\n",
> > 		    info->name_dumpfile, strerror(errno));
> >+	info->fd_dumpfile = -1;
> > }
> >
> > void
> >@@ -8620,9 +8622,10 @@ close_dump_bitmap(void)
> > 	    && !info->flag_sadump && !info->flag_mem_usage)
> > 		return;
> >
> >-	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
> >+	if (close(info->fd_bitmap) < 0)
> > 		ERRMSG("Can't close the bitmap file(%s). %s\n",
> > 		    info->name_bitmap, strerror(errno));
> >+	info->fd_bitmap = -1;
> > 	free(info->name_bitmap);
> > 	info->name_bitmap = NULL;
> > }
> >@@ -8631,16 +8634,18 @@ void
> > close_kernel_file(void)
> > {
> > 	if (info->name_vmlinux) {
> >-		if ((info->fd_vmlinux = close(info->fd_vmlinux)) < 0) {
> >+		if (close(info->fd_vmlinux) < 0) {
> > 			ERRMSG("Can't close the kernel file(%s). %s\n",
> > 			    info->name_vmlinux, strerror(errno));
> > 		}
> >+		info->fd_vmlinux = -1;
> > 	}
> > 	if (info->name_xen_syms) {
> >-		if ((info->fd_xen_syms = close(info->fd_xen_syms)) < 0) {
> >+		if (close(info->fd_xen_syms) < 0) {
> > 			ERRMSG("Can't close the kernel file(%s). %s\n",
> > 			    info->name_xen_syms, strerror(errno));
> > 		}
> >+		info->fd_xen_syms = -1;
> > 	}
> > }
> >
> >@@ -10202,7 +10207,7 @@ reassemble_kdump_header(void)
> >
> > 	ret = TRUE;
> > out:
> >-	if (fd > 0)
> >+	if (fd >= 0)
> > 		close(fd);
> > 	free(buf_bitmap);
> >
> >@@ -10212,7 +10217,7 @@ out:
> > int
> > reassemble_kdump_pages(void)
> > {
> >-	int i, fd = 0, ret = FALSE;
> >+	int i, fd = -1, ret = FALSE;
> > 	off_t offset_first_ph, offset_ph_org, offset_eraseinfo;
> > 	off_t offset_data_new, offset_zero_page = 0;
> > 	mdf_pfn_t pfn, start_pfn, end_pfn;
> >@@ -10336,7 +10341,7 @@ reassemble_kdump_pages(void)
> > 			offset_data_new += pd.size;
> > 		}
> > 		close(fd);
> >-		fd = 0;
> >+		fd = -1;
> > 	}
> > 	if (!write_cache_bufsz(&cd_pd))
> > 		goto out;
> >@@ -10379,7 +10384,7 @@ reassemble_kdump_pages(void)
> > 		size_eraseinfo += SPLITTING_SIZE_EI(i);
> >
> > 		close(fd);
> >-		fd = 0;
> >+		fd = -1;
> > 	}
> > 	if (size_eraseinfo) {
> > 		if (!write_cache_bufsz(&cd_data))
> >@@ -10400,7 +10405,7 @@ out:
> >
> > 	if (data)
> > 		free(data);
> >-	if (fd > 0)
> >+	if (fd >= 0)
> > 		close(fd);
> >
> > 	return ret;
> >@@ -10973,6 +10978,11 @@ main(int argc, char *argv[])
> > 		    strerror(errno));
> > 		goto out;
> > 	}
> >+	info->fd_vmlinux = -1;
> >+	info->fd_xen_syms = -1;
> >+	info->fd_memory = -1;
> >+	info->fd_dumpfile = -1;
> >+	info->fd_bitmap = -1;
> > 	initialize_tables();
> >
> > 	/*
> >@@ -11268,11 +11278,11 @@ out:
> > 				free(info->bitmap_memory->buf);
> > 			free(info->bitmap_memory);
> > 		}
> >-		if (info->fd_memory)
> >+		if (info->fd_memory >= 0)
> > 			close(info->fd_memory);
> >-		if (info->fd_dumpfile)
> >+		if (info->fd_dumpfile >= 0)
> > 			close(info->fd_dumpfile);
> >-		if (info->fd_bitmap)
> >+		if (info->fd_bitmap >= 0)
> > 			close(info->fd_bitmap);
> > 		if (vt.node_online_map != NULL)
> > 			free(vt.node_online_map);
> >diff --git a/makedumpfile.h b/makedumpfile.h
> >index 533e5b8..1814139 100644
> >--- a/makedumpfile.h
> >+++ b/makedumpfile.h
> >@@ -1955,7 +1955,7 @@ is_dumpable_file(struct dump_bitmap *bitmap, mdf_pfn_t pfn)
> > static inline int
> > is_dumpable(struct dump_bitmap *bitmap, mdf_pfn_t pfn, struct cycle *cycle)
> > {
> >-	if (bitmap->fd == 0) {
> >+	if (bitmap->fd < 0) {
> > 		return is_dumpable_buffer(bitmap, pfn, cycle);
> > 	} else {
> > 		return is_dumpable_file(bitmap, pfn);
> >--
> >2.6.6


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

Re: [PATCH v2] Cleanup: Use a negative number for uninitialized file descriptors

[Petr Tesarik]

Do not use zero to denote an invalid file descriptor.

First, zero is a valid value, although quite unlikely to be used for
anything except standard input.

Second, open(2) returns a negative value on failure, so there are
already checks for a negative value in some places.

The purpose of this patch is not to allow running in an evil environment
(with closed stdin), but to aid in debugging by using a consistent value
for uninitialized file descriptors which is also regarded as invalid by
the kernel. For example, attempts to close a negative FD will fail
(unlike an attempt to close FD 0).

Changes from v1:
  - Cleanup file descriptor usage in dwarf_info.c and sadump_info.c

Signed-off-by: Petr Tesarik <ptesarik@suse.com>

---
 dwarf_info.c   |  6 ++++--
 makedumpfile.c | 68 +++++++++++++++++++++++++++++++++-------------------------
 makedumpfile.h |  2 +-
 sadump_info.c  |  3 ++-
 4 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/dwarf_info.c b/dwarf_info.c
index 8c491d3..4f9ad12 100644
--- a/dwarf_info.c
+++ b/dwarf_info.c
@@ -53,7 +53,9 @@ struct dwarf_info {
 	char	src_name[LEN_SRCFILE];	/* OUT */
 	Dwarf_Off die_offset;		/* OUT */
 };
-static struct dwarf_info	dwarf_info;
+static struct dwarf_info	dwarf_info = {
+	.fd_debuginfo = -1,
+};
 
 
 /*
@@ -1595,7 +1597,7 @@ set_dwarf_debuginfo(char *mod_name, char *os_release,
 	if (dwarf_info.module_name
 			&& strcmp(dwarf_info.module_name, "vmlinux")
 			&& strcmp(dwarf_info.module_name, "xen-syms")) {
-		if (dwarf_info.fd_debuginfo > 0)
+		if (dwarf_info.fd_debuginfo >= 0)
 			close(dwarf_info.fd_debuginfo);
 		if (dwarf_info.name_debuginfo)
 			free(dwarf_info.name_debuginfo);
diff --git a/makedumpfile.c b/makedumpfile.c
index 21784e8..d168dfd 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3730,10 +3730,10 @@ free_for_parallel()
 		return;
 
 	for (i = 0; i < info->num_threads; i++) {
-		if (FD_MEMORY_PARALLEL(i) > 0)
+		if (FD_MEMORY_PARALLEL(i) >= 0)
 			close(FD_MEMORY_PARALLEL(i));
 
-		if (FD_BITMAP_MEMORY_PARALLEL(i) > 0)
+		if (FD_BITMAP_MEMORY_PARALLEL(i) >= 0)
 			close(FD_BITMAP_MEMORY_PARALLEL(i));
 	}
 }
@@ -4038,13 +4038,13 @@ out:
 void
 initialize_bitmap(struct dump_bitmap *bitmap)
 {
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		bitmap->fd        = info->fd_bitmap;
 		bitmap->file_name = info->name_bitmap;
 		bitmap->no_block  = -1;
 		memset(bitmap->buf, 0, BUFSIZE_BITMAP);
 	} else {
-		bitmap->fd        = 0;
+		bitmap->fd        = -1;
 		bitmap->file_name = NULL;
 		bitmap->no_block  = -1;
 		memset(bitmap->buf, 0, info->bufsize_cyclic);
@@ -4154,7 +4154,7 @@ set_bitmap_buffer(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cyc
 int
 set_bitmap(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cycle *cycle)
 {
-	if (bitmap->fd) {
+	if (bitmap->fd >= 0) {
 		return set_bitmap_file(bitmap, pfn, val);
 	} else {
 		return set_bitmap_buffer(bitmap, pfn, val, cycle);
@@ -4170,7 +4170,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
 	/*
 	 * The bitmap doesn't have the fd, it's a on-memory bitmap.
 	 */
-	if (bitmap->fd == 0)
+	if (bitmap->fd < 0)
 		return TRUE;
 	/*
 	 * The bitmap buffer is not dirty, and it is not necessary
@@ -5403,7 +5403,7 @@ create_1st_bitmap_buffer(struct cycle *cycle)
 int
 create_1st_bitmap(struct cycle *cycle)
 {
-	if (info->bitmap1->fd) {
+	if (info->bitmap1->fd >= 0) {
 		return create_1st_bitmap_file();
 	} else {
 		return create_1st_bitmap_buffer(cycle);
@@ -5414,7 +5414,7 @@ static inline int
 is_in_segs(unsigned long long paddr)
 {
 	if (info->flag_refiltering || info->flag_sadump) {
-		if (info->bitmap1->fd == 0) {
+		if (info->bitmap1->fd < 0) {
 			initialize_1st_bitmap(info->bitmap1);
 			create_1st_bitmap_file();
 		}
@@ -5872,7 +5872,7 @@ copy_bitmap_file(void)
 int
 copy_bitmap(void)
 {
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		return copy_bitmap_file();
 	} else {
 		return copy_bitmap_buffer();
@@ -6313,7 +6313,7 @@ prepare_bitmap1_buffer(void)
 		return FALSE;
 	}
 
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		if ((info->bitmap1->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
 			ERRMSG("Can't allocate memory for the 1st bitmaps's buffer. %s\n",
 			       strerror(errno));
@@ -6352,7 +6352,7 @@ prepare_bitmap2_buffer(void)
 		       strerror(errno));
 		return FALSE;
 	}
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		if ((info->bitmap2->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
 			ERRMSG("Can't allocate memory for the 2nd bitmaps's buffer. %s\n",
 			       strerror(errno));
@@ -7582,7 +7582,7 @@ kdump_thread_function_cyclic(void *arg) {
 
 	fd_memory = FD_MEMORY_PARALLEL(kdump_thread_args->thread_num);
 
-	if (info->fd_bitmap) {
+	if (info->fd_bitmap >= 0) {
 		bitmap_parallel.buf = malloc(BUFSIZE_BITMAP);
 		if (bitmap_parallel.buf == NULL){
 			ERRMSG("Can't allocate memory for bitmap_parallel.buf. %s\n",
@@ -7628,7 +7628,7 @@ kdump_thread_function_cyclic(void *arg) {
 			pthread_mutex_lock(&info->current_pfn_mutex);
 			for (pfn = info->current_pfn; pfn < cycle->end_pfn; pfn++) {
 				dumpable = is_dumpable(
-					info->fd_bitmap ? &bitmap_parallel : info->bitmap2,
+					info->fd_bitmap >= 0 ? &bitmap_parallel : info->bitmap2,
 					pfn,
 					cycle);
 				if (dumpable)
@@ -7723,7 +7723,7 @@ next:
 	retval = NULL;
 
 fail:
-	if (bitmap_memory_parallel.fd > 0)
+	if (bitmap_memory_parallel.fd >= 0)
 		close(bitmap_memory_parallel.fd);
 	if (bitmap_parallel.buf != NULL)
 		free(bitmap_parallel.buf);
@@ -8461,7 +8461,7 @@ out:
 
 int
 write_kdump_bitmap1(struct cycle *cycle) {
-	if (info->bitmap1->fd) {
+	if (info->bitmap1->fd >= 0) {
 		return write_kdump_bitmap1_file();
 	} else {
 		return write_kdump_bitmap1_buffer(cycle);
@@ -8470,7 +8470,7 @@ write_kdump_bitmap1(struct cycle *cycle) {
 
 int
 write_kdump_bitmap2(struct cycle *cycle) {
-	if (info->bitmap2->fd) {
+	if (info->bitmap2->fd >= 0) {
 		return write_kdump_bitmap2_file();
 	} else {
 		return write_kdump_bitmap2_buffer(cycle);
@@ -8597,9 +8597,10 @@ close_vmcoreinfo(void)
 void
 close_dump_memory(void)
 {
-	if ((info->fd_memory = close(info->fd_memory)) < 0)
+	if (close(info->fd_memory) < 0)
 		ERRMSG("Can't close the dump memory(%s). %s\n",
 		    info->name_memory, strerror(errno));
+	info->fd_memory = -1;
 }
 
 void
@@ -8608,9 +8609,10 @@ close_dump_file(void)
 	if (info->flag_flatten)
 		return;
 
-	if ((info->fd_dumpfile = close(info->fd_dumpfile)) < 0)
+	if (close(info->fd_dumpfile) < 0)
 		ERRMSG("Can't close the dump file(%s). %s\n",
 		    info->name_dumpfile, strerror(errno));
+	info->fd_dumpfile = -1;
 }
 
 void
@@ -8620,9 +8622,10 @@ close_dump_bitmap(void)
 	    && !info->flag_sadump && !info->flag_mem_usage)
 		return;
 
-	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
+	if (close(info->fd_bitmap) < 0)
 		ERRMSG("Can't close the bitmap file(%s). %s\n",
 		    info->name_bitmap, strerror(errno));
+	info->fd_bitmap = -1;
 	free(info->name_bitmap);
 	info->name_bitmap = NULL;
 }
@@ -8631,16 +8634,18 @@ void
 close_kernel_file(void)
 {
 	if (info->name_vmlinux) {
-		if ((info->fd_vmlinux = close(info->fd_vmlinux)) < 0) {
+		if (close(info->fd_vmlinux) < 0) {
 			ERRMSG("Can't close the kernel file(%s). %s\n",
 			    info->name_vmlinux, strerror(errno));
 		}
+		info->fd_vmlinux = -1;
 	}
 	if (info->name_xen_syms) {
-		if ((info->fd_xen_syms = close(info->fd_xen_syms)) < 0) {
+		if (close(info->fd_xen_syms) < 0) {
 			ERRMSG("Can't close the kernel file(%s). %s\n",
 			    info->name_xen_syms, strerror(errno));
 		}
+		info->fd_xen_syms = -1;
 	}
 }
 
@@ -10202,7 +10207,7 @@ reassemble_kdump_header(void)
 
 	ret = TRUE;
 out:
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 	free(buf_bitmap);
 
@@ -10212,7 +10217,7 @@ out:
 int
 reassemble_kdump_pages(void)
 {
-	int i, fd = 0, ret = FALSE;
+	int i, fd = -1, ret = FALSE;
 	off_t offset_first_ph, offset_ph_org, offset_eraseinfo;
 	off_t offset_data_new, offset_zero_page = 0;
 	mdf_pfn_t pfn, start_pfn, end_pfn;
@@ -10336,7 +10341,7 @@ reassemble_kdump_pages(void)
 			offset_data_new += pd.size;
 		}
 		close(fd);
-		fd = 0;
+		fd = -1;
 	}
 	if (!write_cache_bufsz(&cd_pd))
 		goto out;
@@ -10379,7 +10384,7 @@ reassemble_kdump_pages(void)
 		size_eraseinfo += SPLITTING_SIZE_EI(i);
 
 		close(fd);
-		fd = 0;
+		fd = -1;
 	}
 	if (size_eraseinfo) {
 		if (!write_cache_bufsz(&cd_data))
@@ -10400,7 +10405,7 @@ out:
 
 	if (data)
 		free(data);
-	if (fd > 0)
+	if (fd >= 0)
 		close(fd);
 
 	return ret;
@@ -10973,6 +10978,11 @@ main(int argc, char *argv[])
 		    strerror(errno));
 		goto out;
 	}
+	info->fd_vmlinux = -1;
+	info->fd_xen_syms = -1;
+	info->fd_memory = -1;
+	info->fd_dumpfile = -1;
+	info->fd_bitmap = -1;
 	initialize_tables();
 
 	/*
@@ -11268,11 +11278,11 @@ out:
 				free(info->bitmap_memory->buf);
 			free(info->bitmap_memory);
 		}
-		if (info->fd_memory)
+		if (info->fd_memory >= 0)
 			close(info->fd_memory);
-		if (info->fd_dumpfile)
+		if (info->fd_dumpfile >= 0)
 			close(info->fd_dumpfile);
-		if (info->fd_bitmap)
+		if (info->fd_bitmap >= 0)
 			close(info->fd_bitmap);
 		if (vt.node_online_map != NULL)
 			free(vt.node_online_map);
diff --git a/makedumpfile.h b/makedumpfile.h
index 533e5b8..1814139 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1955,7 +1955,7 @@ is_dumpable_file(struct dump_bitmap *bitmap, mdf_pfn_t pfn)
 static inline int
 is_dumpable(struct dump_bitmap *bitmap, mdf_pfn_t pfn, struct cycle *cycle)
 {
-	if (bitmap->fd == 0) {
+	if (bitmap->fd < 0) {
 		return is_dumpable_buffer(bitmap, pfn, cycle);
 	} else {
 		return is_dumpable_file(bitmap, pfn);
diff --git a/sadump_info.c b/sadump_info.c
index 5ff5595..f77a020 100644
--- a/sadump_info.c
+++ b/sadump_info.c
@@ -1853,6 +1853,7 @@ sadump_add_diskset_info(char *name_memory)
 	}
 
 	si->diskset_info[si->num_disks - 1].name_memory = name_memory;
+	si->diskset_info[si->num_disks - 1].fd_memory = -1;
 
 	return TRUE;
 }
@@ -1917,7 +1918,7 @@ free_sadump_info(void)
 		int i;
 
 		for (i = 1; i < si->num_disks; ++i) {
-			if (si->diskset_info[i].fd_memory)
+			if (si->diskset_info[i].fd_memory >= 0)
 				close(si->diskset_info[i].fd_memory);
 			if (si->diskset_info[i].sph_memory)
 				free(si->diskset_info[i].sph_memory);
-- 
2.6.6


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

RE: [PATCH v2] Cleanup: Use a negative number for uninitialized file descriptors

[Atsushi Kumagai]

Hello,

>Do not use zero to denote an invalid file descriptor.
>
>First, zero is a valid value, although quite unlikely to be used for
>anything except standard input.
>
>Second, open(2) returns a negative value on failure, so there are
>already checks for a negative value in some places.
>
>The purpose of this patch is not to allow running in an evil environment
>(with closed stdin), but to aid in debugging by using a consistent value
>for uninitialized file descriptors which is also regarded as invalid by
>the kernel. For example, attempts to close a negative FD will fail
>(unlike an attempt to close FD 0).
>
>Changes from v1:
>  - Cleanup file descriptor usage in dwarf_info.c and sadump_info.c

Thanks for your quick response.
This fix isn't necessary but better to do it as you said.

Martin, could you rebase your three patches ?
I've updated the devel branch.


Thanks,
Atsushi Kumagai

>Signed-off-by: Petr Tesarik <ptesarik@suse.com>
>
>---
> dwarf_info.c   |  6 ++++--
> makedumpfile.c | 68 +++++++++++++++++++++++++++++++++-------------------------
> makedumpfile.h |  2 +-
> sadump_info.c  |  3 ++-
> 4 files changed, 46 insertions(+), 33 deletions(-)
>
>diff --git a/dwarf_info.c b/dwarf_info.c
>index 8c491d3..4f9ad12 100644
>--- a/dwarf_info.c
>+++ b/dwarf_info.c
>@@ -53,7 +53,9 @@ struct dwarf_info {
> 	char	src_name[LEN_SRCFILE];	/* OUT */
> 	Dwarf_Off die_offset;		/* OUT */
> };
>-static struct dwarf_info	dwarf_info;
>+static struct dwarf_info	dwarf_info = {
>+	.fd_debuginfo = -1,
>+};
>
>
> /*
>@@ -1595,7 +1597,7 @@ set_dwarf_debuginfo(char *mod_name, char *os_release,
> 	if (dwarf_info.module_name
> 			&& strcmp(dwarf_info.module_name, "vmlinux")
> 			&& strcmp(dwarf_info.module_name, "xen-syms")) {
>-		if (dwarf_info.fd_debuginfo > 0)
>+		if (dwarf_info.fd_debuginfo >= 0)
> 			close(dwarf_info.fd_debuginfo);
> 		if (dwarf_info.name_debuginfo)
> 			free(dwarf_info.name_debuginfo);
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 21784e8..d168dfd 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -3730,10 +3730,10 @@ free_for_parallel()
> 		return;
>
> 	for (i = 0; i < info->num_threads; i++) {
>-		if (FD_MEMORY_PARALLEL(i) > 0)
>+		if (FD_MEMORY_PARALLEL(i) >= 0)
> 			close(FD_MEMORY_PARALLEL(i));
>
>-		if (FD_BITMAP_MEMORY_PARALLEL(i) > 0)
>+		if (FD_BITMAP_MEMORY_PARALLEL(i) >= 0)
> 			close(FD_BITMAP_MEMORY_PARALLEL(i));
> 	}
> }
>@@ -4038,13 +4038,13 @@ out:
> void
> initialize_bitmap(struct dump_bitmap *bitmap)
> {
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		bitmap->fd        = info->fd_bitmap;
> 		bitmap->file_name = info->name_bitmap;
> 		bitmap->no_block  = -1;
> 		memset(bitmap->buf, 0, BUFSIZE_BITMAP);
> 	} else {
>-		bitmap->fd        = 0;
>+		bitmap->fd        = -1;
> 		bitmap->file_name = NULL;
> 		bitmap->no_block  = -1;
> 		memset(bitmap->buf, 0, info->bufsize_cyclic);
>@@ -4154,7 +4154,7 @@ set_bitmap_buffer(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cyc
> int
> set_bitmap(struct dump_bitmap *bitmap, mdf_pfn_t pfn, int val, struct cycle *cycle)
> {
>-	if (bitmap->fd) {
>+	if (bitmap->fd >= 0) {
> 		return set_bitmap_file(bitmap, pfn, val);
> 	} else {
> 		return set_bitmap_buffer(bitmap, pfn, val, cycle);
>@@ -4170,7 +4170,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
> 	/*
> 	 * The bitmap doesn't have the fd, it's a on-memory bitmap.
> 	 */
>-	if (bitmap->fd == 0)
>+	if (bitmap->fd < 0)
> 		return TRUE;
> 	/*
> 	 * The bitmap buffer is not dirty, and it is not necessary
>@@ -5403,7 +5403,7 @@ create_1st_bitmap_buffer(struct cycle *cycle)
> int
> create_1st_bitmap(struct cycle *cycle)
> {
>-	if (info->bitmap1->fd) {
>+	if (info->bitmap1->fd >= 0) {
> 		return create_1st_bitmap_file();
> 	} else {
> 		return create_1st_bitmap_buffer(cycle);
>@@ -5414,7 +5414,7 @@ static inline int
> is_in_segs(unsigned long long paddr)
> {
> 	if (info->flag_refiltering || info->flag_sadump) {
>-		if (info->bitmap1->fd == 0) {
>+		if (info->bitmap1->fd < 0) {
> 			initialize_1st_bitmap(info->bitmap1);
> 			create_1st_bitmap_file();
> 		}
>@@ -5872,7 +5872,7 @@ copy_bitmap_file(void)
> int
> copy_bitmap(void)
> {
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		return copy_bitmap_file();
> 	} else {
> 		return copy_bitmap_buffer();
>@@ -6313,7 +6313,7 @@ prepare_bitmap1_buffer(void)
> 		return FALSE;
> 	}
>
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		if ((info->bitmap1->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> 			ERRMSG("Can't allocate memory for the 1st bitmaps's buffer. %s\n",
> 			       strerror(errno));
>@@ -6352,7 +6352,7 @@ prepare_bitmap2_buffer(void)
> 		       strerror(errno));
> 		return FALSE;
> 	}
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		if ((info->bitmap2->buf = (char *)malloc(BUFSIZE_BITMAP)) == NULL) {
> 			ERRMSG("Can't allocate memory for the 2nd bitmaps's buffer. %s\n",
> 			       strerror(errno));
>@@ -7582,7 +7582,7 @@ kdump_thread_function_cyclic(void *arg) {
>
> 	fd_memory = FD_MEMORY_PARALLEL(kdump_thread_args->thread_num);
>
>-	if (info->fd_bitmap) {
>+	if (info->fd_bitmap >= 0) {
> 		bitmap_parallel.buf = malloc(BUFSIZE_BITMAP);
> 		if (bitmap_parallel.buf == NULL){
> 			ERRMSG("Can't allocate memory for bitmap_parallel.buf. %s\n",
>@@ -7628,7 +7628,7 @@ kdump_thread_function_cyclic(void *arg) {
> 			pthread_mutex_lock(&info->current_pfn_mutex);
> 			for (pfn = info->current_pfn; pfn < cycle->end_pfn; pfn++) {
> 				dumpable = is_dumpable(
>-					info->fd_bitmap ? &bitmap_parallel : info->bitmap2,
>+					info->fd_bitmap >= 0 ? &bitmap_parallel : info->bitmap2,
> 					pfn,
> 					cycle);
> 				if (dumpable)
>@@ -7723,7 +7723,7 @@ next:
> 	retval = NULL;
>
> fail:
>-	if (bitmap_memory_parallel.fd > 0)
>+	if (bitmap_memory_parallel.fd >= 0)
> 		close(bitmap_memory_parallel.fd);
> 	if (bitmap_parallel.buf != NULL)
> 		free(bitmap_parallel.buf);
>@@ -8461,7 +8461,7 @@ out:
>
> int
> write_kdump_bitmap1(struct cycle *cycle) {
>-	if (info->bitmap1->fd) {
>+	if (info->bitmap1->fd >= 0) {
> 		return write_kdump_bitmap1_file();
> 	} else {
> 		return write_kdump_bitmap1_buffer(cycle);
>@@ -8470,7 +8470,7 @@ write_kdump_bitmap1(struct cycle *cycle) {
>
> int
> write_kdump_bitmap2(struct cycle *cycle) {
>-	if (info->bitmap2->fd) {
>+	if (info->bitmap2->fd >= 0) {
> 		return write_kdump_bitmap2_file();
> 	} else {
> 		return write_kdump_bitmap2_buffer(cycle);
>@@ -8597,9 +8597,10 @@ close_vmcoreinfo(void)
> void
> close_dump_memory(void)
> {
>-	if ((info->fd_memory = close(info->fd_memory)) < 0)
>+	if (close(info->fd_memory) < 0)
> 		ERRMSG("Can't close the dump memory(%s). %s\n",
> 		    info->name_memory, strerror(errno));
>+	info->fd_memory = -1;
> }
>
> void
>@@ -8608,9 +8609,10 @@ close_dump_file(void)
> 	if (info->flag_flatten)
> 		return;
>
>-	if ((info->fd_dumpfile = close(info->fd_dumpfile)) < 0)
>+	if (close(info->fd_dumpfile) < 0)
> 		ERRMSG("Can't close the dump file(%s). %s\n",
> 		    info->name_dumpfile, strerror(errno));
>+	info->fd_dumpfile = -1;
> }
>
> void
>@@ -8620,9 +8622,10 @@ close_dump_bitmap(void)
> 	    && !info->flag_sadump && !info->flag_mem_usage)
> 		return;
>
>-	if ((info->fd_bitmap = close(info->fd_bitmap)) < 0)
>+	if (close(info->fd_bitmap) < 0)
> 		ERRMSG("Can't close the bitmap file(%s). %s\n",
> 		    info->name_bitmap, strerror(errno));
>+	info->fd_bitmap = -1;
> 	free(info->name_bitmap);
> 	info->name_bitmap = NULL;
> }
>@@ -8631,16 +8634,18 @@ void
> close_kernel_file(void)
> {
> 	if (info->name_vmlinux) {
>-		if ((info->fd_vmlinux = close(info->fd_vmlinux)) < 0) {
>+		if (close(info->fd_vmlinux) < 0) {
> 			ERRMSG("Can't close the kernel file(%s). %s\n",
> 			    info->name_vmlinux, strerror(errno));
> 		}
>+		info->fd_vmlinux = -1;
> 	}
> 	if (info->name_xen_syms) {
>-		if ((info->fd_xen_syms = close(info->fd_xen_syms)) < 0) {
>+		if (close(info->fd_xen_syms) < 0) {
> 			ERRMSG("Can't close the kernel file(%s). %s\n",
> 			    info->name_xen_syms, strerror(errno));
> 		}
>+		info->fd_xen_syms = -1;
> 	}
> }
>
>@@ -10202,7 +10207,7 @@ reassemble_kdump_header(void)
>
> 	ret = TRUE;
> out:
>-	if (fd > 0)
>+	if (fd >= 0)
> 		close(fd);
> 	free(buf_bitmap);
>
>@@ -10212,7 +10217,7 @@ out:
> int
> reassemble_kdump_pages(void)
> {
>-	int i, fd = 0, ret = FALSE;
>+	int i, fd = -1, ret = FALSE;
> 	off_t offset_first_ph, offset_ph_org, offset_eraseinfo;
> 	off_t offset_data_new, offset_zero_page = 0;
> 	mdf_pfn_t pfn, start_pfn, end_pfn;
>@@ -10336,7 +10341,7 @@ reassemble_kdump_pages(void)
> 			offset_data_new += pd.size;
> 		}
> 		close(fd);
>-		fd = 0;
>+		fd = -1;
> 	}
> 	if (!write_cache_bufsz(&cd_pd))
> 		goto out;
>@@ -10379,7 +10384,7 @@ reassemble_kdump_pages(void)
> 		size_eraseinfo += SPLITTING_SIZE_EI(i);
>
> 		close(fd);
>-		fd = 0;
>+		fd = -1;
> 	}
> 	if (size_eraseinfo) {
> 		if (!write_cache_bufsz(&cd_data))
>@@ -10400,7 +10405,7 @@ out:
>
> 	if (data)
> 		free(data);
>-	if (fd > 0)
>+	if (fd >= 0)
> 		close(fd);
>
> 	return ret;
>@@ -10973,6 +10978,11 @@ main(int argc, char *argv[])
> 		    strerror(errno));
> 		goto out;
> 	}
>+	info->fd_vmlinux = -1;
>+	info->fd_xen_syms = -1;
>+	info->fd_memory = -1;
>+	info->fd_dumpfile = -1;
>+	info->fd_bitmap = -1;
> 	initialize_tables();
>
> 	/*
>@@ -11268,11 +11278,11 @@ out:
> 				free(info->bitmap_memory->buf);
> 			free(info->bitmap_memory);
> 		}
>-		if (info->fd_memory)
>+		if (info->fd_memory >= 0)
> 			close(info->fd_memory);
>-		if (info->fd_dumpfile)
>+		if (info->fd_dumpfile >= 0)
> 			close(info->fd_dumpfile);
>-		if (info->fd_bitmap)
>+		if (info->fd_bitmap >= 0)
> 			close(info->fd_bitmap);
> 		if (vt.node_online_map != NULL)
> 			free(vt.node_online_map);
>diff --git a/makedumpfile.h b/makedumpfile.h
>index 533e5b8..1814139 100644
>--- a/makedumpfile.h
>+++ b/makedumpfile.h
>@@ -1955,7 +1955,7 @@ is_dumpable_file(struct dump_bitmap *bitmap, mdf_pfn_t pfn)
> static inline int
> is_dumpable(struct dump_bitmap *bitmap, mdf_pfn_t pfn, struct cycle *cycle)
> {
>-	if (bitmap->fd == 0) {
>+	if (bitmap->fd < 0) {
> 		return is_dumpable_buffer(bitmap, pfn, cycle);
> 	} else {
> 		return is_dumpable_file(bitmap, pfn);
>diff --git a/sadump_info.c b/sadump_info.c
>index 5ff5595..f77a020 100644
>--- a/sadump_info.c
>+++ b/sadump_info.c
>@@ -1853,6 +1853,7 @@ sadump_add_diskset_info(char *name_memory)
> 	}
>
> 	si->diskset_info[si->num_disks - 1].name_memory = name_memory;
>+	si->diskset_info[si->num_disks - 1].fd_memory = -1;
>
> 	return TRUE;
> }
>@@ -1917,7 +1918,7 @@ free_sadump_info(void)
> 		int i;
>
> 		for (i = 1; i < si->num_disks; ++i) {
>-			if (si->diskset_info[i].fd_memory)
>+			if (si->diskset_info[i].fd_memory >= 0)
> 				close(si->diskset_info[i].fd_memory);
> 			if (si->diskset_info[i].sph_memory)
> 				free(si->diskset_info[i].sph_memory);
>--
>2.6.6


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

[PATCH v2 1/3] open_dump_bitmap: open bitmap file in non-cyclic case

[Martin Wilck]

The logic of set_bitmap() requires that a bitmap fd exists in the
non-cyclic case.
---
 makedumpfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index d168dfd..6164468 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1365,7 +1365,7 @@ open_dump_bitmap(void)
 
 	/* Unnecessary to open */
 	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	    && !info->flag_sadump && !info->flag_mem_usage && info->flag_cyclic)
 		return TRUE;
 
 	tmpname = getenv("TMPDIR");
-- 
2.9.3


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

[PATCH v2 2/3] move call to open_dump_bitmap() to after call to initial()

[Martin Wilck]

When open_files_for_creating_dumpfile() is called, we don't have all
necessary information to determine whether a bitmap file is actually
needed. In particular, we don't know whether info->flag_cyclic will
ultimately be set. This patch moves the call to open_dump_bitmap()
to after initialize() when all flags are known.

For the dump_dmesg() path, no bitmap file is needed.
---
 makedumpfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 6164468..30e1fa8 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1480,9 +1480,6 @@ open_files_for_creating_dumpfile(void)
 	if (!open_dump_memory())
 		return FALSE;
 
-	if (!open_dump_bitmap())
-		return FALSE;
-
 	return TRUE;
 }
 
@@ -9747,6 +9744,9 @@ create_dumpfile(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	/* create an array of translations from pfn to vmemmap pages */
 	if (info->flag_excludevm) {
 		if (find_vmemmap() == FAILED) {
@@ -10917,6 +10917,9 @@ int show_mem_usage(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	if (!prepare_bitmap_buffer())
 		return FALSE;
 
-- 
2.9.3


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

[PATCH v2 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

The boolean expression replicates the logic of open_dump_bitmap().
It's simpler and less error-prone to simply check if fd_bitmap is
valid.
---
 makedumpfile.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 30e1fa8..d46777c 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8615,8 +8615,7 @@ close_dump_file(void)
 void
 close_dump_bitmap(void)
 {
-	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	if (!info->fd_bitmap)
 		return;
 
 	if (close(info->fd_bitmap) < 0)
-- 
2.9.3


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

[PATCH v3 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

The boolean expression replicates the logic of open_dump_bitmap().
It's simpler and less error-prone to simply check if fd_bitmap is
valid.

(I forgot to apply the very change that Petr had asked for in V2 of
this patch. I'm very sorry. Please apply V3).

---
 makedumpfile.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 30e1fa8..0f5be7f 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8615,8 +8615,7 @@ close_dump_file(void)
 void
 close_dump_bitmap(void)
 {
-	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	if (info->fd_bitmap < 0)
 		return;
 
 	if (close(info->fd_bitmap) < 0)
-- 
2.9.3


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

RE: [PATCH v3 3/3] close_dump_bitmap: simplify logic

[Atsushi Kumagai]

Hello Martin,

>The boolean expression replicates the logic of open_dump_bitmap().
>It's simpler and less error-prone to simply check if fd_bitmap is
>valid.
>
>(I forgot to apply the very change that Petr had asked for in V2 of
>this patch. I'm very sorry. Please apply V3).

This version looks good, so could you add your Signed-off-by ?

Thanks,
Atsushi Kumagai

>---
> makedumpfile.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 30e1fa8..0f5be7f 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -8615,8 +8615,7 @@ close_dump_file(void)
> void
> close_dump_bitmap(void)
> {
>-	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
>-	    && !info->flag_sadump && !info->flag_mem_usage)
>+	if (info->fd_bitmap < 0)
> 		return;
>
> 	if (close(info->fd_bitmap) < 0)
>--
>2.9.3

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

Re: [PATCH v3 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

Kumagai-san,
On Tue, 2016-09-20 at 09:43 +0000, Atsushi Kumagai wrote:
> 
> This version looks good, so could you add your Signed-off-by ?

OK, I'll resend the whole v3 series.

Martin



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

[PATCH v3 1/3] open_dump_bitmap: open bitmap file in non-cyclic case

[Martin Wilck]

The logic of set_bitmap() requires that a bitmap fd exists in the
non-cyclic case.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index d168dfd..6164468 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1365,7 +1365,7 @@ open_dump_bitmap(void)
 
 	/* Unnecessary to open */
 	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	    && !info->flag_sadump && !info->flag_mem_usage && info->flag_cyclic)
 		return TRUE;
 
 	tmpname = getenv("TMPDIR");
-- 
2.9.3


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

[PATCH v3 2/3] move call to open_dump_bitmap() to after call to initial()

[Martin Wilck]

When open_files_for_creating_dumpfile() is called, we don't have all
necessary information to determine whether a bitmap file is actually
needed. In particular, we don't know whether info->flag_cyclic will
ultimately be set. This patch moves the call to open_dump_bitmap()
to after initialize() when all flags are known.

For the dump_dmesg() path, no bitmap file is needed.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 6164468..30e1fa8 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1480,9 +1480,6 @@ open_files_for_creating_dumpfile(void)
 	if (!open_dump_memory())
 		return FALSE;
 
-	if (!open_dump_bitmap())
-		return FALSE;
-
 	return TRUE;
 }
 
@@ -9747,6 +9744,9 @@ create_dumpfile(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	/* create an array of translations from pfn to vmemmap pages */
 	if (info->flag_excludevm) {
 		if (find_vmemmap() == FAILED) {
@@ -10917,6 +10917,9 @@ int show_mem_usage(void)
 	if (!initial())
 		return FALSE;
 
+	if (!open_dump_bitmap())
+		return FALSE;
+
 	if (!prepare_bitmap_buffer())
 		return FALSE;
 
-- 
2.9.3


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

[PATCH v3 3/3] close_dump_bitmap: simplify logic

[Martin Wilck]

The boolean expression replicates the logic of open_dump_bitmap().
It's simpler and less error-prone to simply check if fd_bitmap is
valid.

Signed-off-by: Martin Wilck <mwilck@suse.de>
---
 makedumpfile.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 30e1fa8..0f5be7f 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -8615,8 +8615,7 @@ close_dump_file(void)
 void
 close_dump_bitmap(void)
 {
-	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
-	    && !info->flag_sadump && !info->flag_mem_usage)
+	if (info->fd_bitmap < 0)
 		return;
 
 	if (close(info->fd_bitmap) < 0)
-- 
2.9.3


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

RE: [PATCH v3 1/3] open_dump_bitmap: open bitmap file in non-cyclic case

[Atsushi Kumagai]

Hello Martin,

>The logic of set_bitmap() requires that a bitmap fd exists in the
>non-cyclic case.
>
>Signed-off-by: Martin Wilck <mwilck@suse.de>

I'll merge this version into v1.6.1, thanks for your work.

Regards,
Atsushi Kumagai

>---
> makedumpfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index d168dfd..6164468 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -1365,7 +1365,7 @@ open_dump_bitmap(void)
>
> 	/* Unnecessary to open */
> 	if (!info->working_dir && !info->flag_reassemble && !info->flag_refiltering
>-	    && !info->flag_sadump && !info->flag_mem_usage)
>+	    && !info->flag_sadump && !info->flag_mem_usage && info->flag_cyclic)
> 		return TRUE;
>
> 	tmpname = getenv("TMPDIR");
>--
>2.9.3


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