[PATCH] kexec: fix mmap return code handling

[PATCH] kexec: fix mmap return code handling

[Michael Holzheu]

Hi Simon,

I made a mistake in my mmap patch, sorry for that!

When mmap fails, MAP_FAILED (that is, (void *) -1) is returned. Currently
we assume that NULL is returned. Fix this and add the MAP_FAILED check.

Fixes: 95741713e790 ("kexec/s390x: use mmap instead of read for slurp_file")
Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
---
 kexec/kexec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kexec/kexec.c b/kexec/kexec.c
index cf6e03d..02285fb 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -568,6 +568,8 @@ static char *slurp_file_generic(const char *filename, off_t *r_size,
 		if (use_mmap) {
 			buf = mmap(NULL, size, PROT_READ|PROT_WRITE,
 				   MAP_PRIVATE, fd, 0);
+			if (buf == MAP_FAILED)
+				buf = NULL;
 			nread = size;
 		} else {
 			buf = slurp_fd(fd, filename, size, &nread);
-- 
2.3.9


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

[PATCH v2] kexec: fix mmap return code handling

[Michael Holzheu]

Hi Simon again,

After a bit more thinking: In theory mmap() could also return NULL.
Therefore the following fix is probably the better one ...
---
Subject: [PATCH] kexec: fix mmap return code handling

When mmap fails, MAP_FAILED (that is, (void *) -1) is returned. Currently
we assume that NULL is returned. Fix this and add the MAP_FAILED check.

Fixes: 95741713e790 ("kexec/s390x: use mmap instead of read for slurp_file")
Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>

diff --git a/kexec/kexec.c b/kexec/kexec.c
index cf6e03d..f0bd527 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -573,7 +573,7 @@ static char *slurp_file_generic(const char *filename, off_t *r_size,
 			buf = slurp_fd(fd, filename, size, &nread);
 		}
 	}
-	if (!buf)
+	if ((use_mmap && (buf == MAP_FAILED)) || (!use_mmap && (buf == NULL)))
 		die("Cannot read %s", filename);
 
 	if (nread != size)
-- 
2.3.9


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

Re: [PATCH v2] kexec: fix mmap return code handling

[Petr Tesarik]

On Thu, 26 Nov 2015 18:32:31 +0100
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

> Hi Simon again,
> 
> After a bit more thinking: In theory mmap() could also return NULL.
> Therefore the following fix is probably the better one ...

No, if you let the kernel choose the address (i.e. call mmap with NULL
addr), it will return at least PAGE_SIZE (and a higher limit is usually
enforced by sys.vm.mmap_min_addr sysctl). Admittedly the limit is set
in arch-specific code, so theoretically there can be an architecture
which sets the limit to 0, but I doubt it, because it would break too
many assumptions in user space (for example gcc assumes that
dereferencing a NULL pointer terminates a process).

In short, this other fix is just as good as the previous one.

Regards,
Petr Tesarik

> ---
> Subject: [PATCH] kexec: fix mmap return code handling
> 
> When mmap fails, MAP_FAILED (that is, (void *) -1) is returned. Currently
> we assume that NULL is returned. Fix this and add the MAP_FAILED check.
> 
> Fixes: 95741713e790 ("kexec/s390x: use mmap instead of read for slurp_file")
> Signed-off-by: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> 
> diff --git a/kexec/kexec.c b/kexec/kexec.c
> index cf6e03d..f0bd527 100644
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -573,7 +573,7 @@ static char *slurp_file_generic(const char *filename, off_t *r_size,
>  			buf = slurp_fd(fd, filename, size, &nread);
>  		}
>  	}
> -	if (!buf)
> +	if ((use_mmap && (buf == MAP_FAILED)) || (!use_mmap && (buf == NULL)))
>  		die("Cannot read %s", filename);
>  
>  	if (nread != size)


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

Re: [PATCH v2] kexec: fix mmap return code handling

[Michael Holzheu]

On Thu, 26 Nov 2015 19:02:28 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> On Thu, 26 Nov 2015 18:32:31 +0100
> Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> 
> > Hi Simon again,
> > 
> > After a bit more thinking: In theory mmap() could also return NULL.
> > Therefore the following fix is probably the better one ...
> 
> No, if you let the kernel choose the address (i.e. call mmap with NULL
> addr), it will return at least PAGE_SIZE (and a higher limit is usually
> enforced by sys.vm.mmap_min_addr sysctl). Admittedly the limit is set
> in arch-specific code, so theoretically there can be an architecture
> which sets the limit to 0, but I doubt it, because it would break too
> many assumptions in user space (for example gcc assumes that
> dereferencing a NULL pointer terminates a process).
>
> In short, this other fix is just as good as the previous one.

Hi Petr,

Thanks for clarification! I still would vote for the second one ;-)

Michael


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

Re: [PATCH v2] kexec: fix mmap return code handling

[Simon Horman]

On Thu, Nov 26, 2015 at 08:02:35PM +0100, Michael Holzheu wrote:
> On Thu, 26 Nov 2015 19:02:28 +0100
> Petr Tesarik <ptesarik@suse.cz> wrote:
> 
> > On Thu, 26 Nov 2015 18:32:31 +0100
> > Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> > 
> > > Hi Simon again,
> > > 
> > > After a bit more thinking: In theory mmap() could also return NULL.
> > > Therefore the following fix is probably the better one ...
> > 
> > No, if you let the kernel choose the address (i.e. call mmap with NULL
> > addr), it will return at least PAGE_SIZE (and a higher limit is usually
> > enforced by sys.vm.mmap_min_addr sysctl). Admittedly the limit is set
> > in arch-specific code, so theoretically there can be an architecture
> > which sets the limit to 0, but I doubt it, because it would break too
> > many assumptions in user space (for example gcc assumes that
> > dereferencing a NULL pointer terminates a process).
> >
> > In short, this other fix is just as good as the previous one.
> 
> Hi Petr,
> 
> Thanks for clarification! I still would vote for the second one ;-)

Thanks, I have applied that version.

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