[PATCH] parisc: fix re-mmap() of shared file to same fixed address

[PATCH] parisc: fix re-mmap() of shared file to same fixed address

[Helge Deller]

locale-gen from glibc showed a strange problem on parisc: 
mmap2(NULL, 536870912, PROT_NONE, MAP_SHARED, 3, 0) = 0x42a54000
mmap2(0x42a54000, 103860, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)

Basically it was just trying to re-mmap() a file at the same address
which it was given by a previous mmap() call. But this remapping failed
with EINVAL.

The problem is, that when MAP_FIXED and MAP_SHARED flags were used, we didn't
included the mapping-based offset when we verified the alignment of the given
fixed address against the offset which we calculated it in the previous call.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable....

diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
index 5dfd248..0d3a9d4 100644
--- a/arch/parisc/kernel/sys_parisc.c
+++ b/arch/parisc/kernel/sys_parisc.c
@@ -61,8 +61,15 @@ static int get_offset(struct address_space *mapping)
 	return (unsigned long) mapping >> 8;
 }
 
-static unsigned long get_shared_area(struct address_space *mapping,
-		unsigned long addr, unsigned long len, unsigned long pgoff)
+static unsigned long shared_align_offset(struct file *filp, unsigned long pgoff)
+{
+	struct address_space *mapping = filp ? filp->f_mapping : NULL;
+
+	return (get_offset(mapping) + pgoff) << PAGE_SHIFT;
+}
+
+static unsigned long get_shared_area(struct file *filp, unsigned long addr,
+		unsigned long len, unsigned long pgoff)
 {
 	struct vm_unmapped_area_info info;
 
@@ -71,7 +78,7 @@ static unsigned long get_shared_area(struct address_space *mapping,
 	info.low_limit = PAGE_ALIGN(addr);
 	info.high_limit = TASK_SIZE;
 	info.align_mask = PAGE_MASK & (SHMLBA - 1);
-	info.align_offset = (get_offset(mapping) + pgoff) << PAGE_SHIFT;
+	info.align_offset = shared_align_offset(filp, pgoff);
 	return vm_unmapped_area(&info);
 }
 
@@ -82,20 +89,18 @@ unsigned long arch_get_unmapped_area(struct file *filp, unsigned long addr,
 		return -ENOMEM;
 	if (flags & MAP_FIXED) {
 		if ((flags & MAP_SHARED) &&
-		    (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1))
+		    (addr - shared_align_offset(filp, pgoff)) & (SHMLBA - 1))
 			return -EINVAL;
 		return addr;
 	}
 	if (!addr)
 		addr = TASK_UNMAPPED_BASE;
 
-	if (filp) {
-		addr = get_shared_area(filp->f_mapping, addr, len, pgoff);
-	} else if(flags & MAP_SHARED) {
-		addr = get_shared_area(NULL, addr, len, pgoff);
-	} else {
+	if (filp || (flags & MAP_SHARED))
+		addr = get_shared_area(filp, addr, len, pgoff);
+	else
 		addr = get_unshared_area(addr, len);
-	}
+
 	return addr;
 }
 

Re: [PATCH] parisc: fix re-mmap() of shared file to same fixed address

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1273 bytes --]

On Wednesday 20 November 2013 17:07:42 Helge Deller wrote:
> locale-gen from glibc showed a strange problem on parisc:

pedantic: locale-gen is from distros while glibc itself provides localedef.

> mmap2(NULL, 536870912, PROT_NONE, MAP_SHARED, 3, 0) = 0x42a54000
> mmap2(0x42a54000, 103860, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED, 3, 0)
> = -1 EINVAL (Invalid argument)
> 
> Basically it was just trying to re-mmap() a file at the same address
> which it was given by a previous mmap() call. But this remapping failed
> with EINVAL.
> 
> The problem is, that when MAP_FIXED and MAP_SHARED flags were used, we
> didn't included the mapping-based offset when we verified the alignment of
> the given fixed address against the offset which we calculated it in the
> previous call.

hmm, which version of glibc were you trying ?  we landed this change recently:
https://sourceware.org/git/?p=glibc.git;a=commitdiff_plain;h=17db6e8d6b12f55e312fcab46faf5d332c806fb6

it's part of glibc-2.18.  we know earlier versions of localedef failed on 
systems like parisc, although it seemed to really only creep up on 64bit 
kernels reliably.

based on your description, it sounds like independent bugs, but it also seems 
a little coincidental ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] parisc: fix re-mmap() of shared file to same fixed address

[John David Anglin]

On 24-Nov-13, at 11:41 PM, Mike Frysinger wrote:

> On Wednesday 20 November 2013 17:07:42 Helge Deller wrote:
>> locale-gen from glibc showed a strange problem on parisc:
>
> pedantic: locale-gen is from distros while glibc itself provides  
> localedef.
>
>> mmap2(NULL, 536870912, PROT_NONE, MAP_SHARED, 3, 0) = 0x42a54000
>> mmap2(0x42a54000, 103860, PROT_READ|PROT_WRITE, MAP_SHARED| 
>> MAP_FIXED, 3, 0)
>> = -1 EINVAL (Invalid argument)
>>
>> Basically it was just trying to re-mmap() a file at the same address
>> which it was given by a previous mmap() call. But this remapping  
>> failed
>> with EINVAL.
>>
>> The problem is, that when MAP_FIXED and MAP_SHARED flags were used,  
>> we
>> didn't included the mapping-based offset when we verified the  
>> alignment of
>> the given fixed address against the offset which we calculated it  
>> in the
>> previous call.
>
> hmm, which version of glibc were you trying ?  we landed this change  
> recently:
> https://sourceware.org/git/?p=glibc.git;a=commitdiff_plain;h=17db6e8d6b12f55e312fcab46faf5d332c806fb6
>
> it's part of glibc-2.18.  we know earlier versions of localedef  
> failed on
> systems like parisc, although it seemed to really only creep up on  
> 64bit
> kernels reliably.

Besides the above, our mmap allocater doesn't work well when we have a  
large number
of small allocations:
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119567

Effectively, we allocate 4 MB per locale and soon run out of memory.

Problem is here:

/*
  * We need to know the offset to use.  Old scheme was to look for
  * existing mapping and use the same offset.  New scheme is to use the
  * address of the kernel data structure as the seed for the offset.
  * We'll see how that works...
  *
  * The mapping is cacheline aligned, so there's no information in the  
bottom
  * few bits of the address.  We're looking for 10 bits (4MB / 4k), so  
let's
  * drop the bottom 8 bits and use bits 8-17.
  */
static int get_offset(struct address_space *mapping)
{
         return (unsigned long) mapping >> 8;
}

We rarely suceed in allocating two or more maps in the same 4 MB block
of memory.

Dave
--
John David Anglin	dave.anglin@bell.net

Re: [PATCH] parisc: fix re-mmap() of shared file to same fixed address

[Helge Deller]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Mike,

On 11/25/2013 05:41 AM, Mike Frysinger wrote:
> On Wednesday 20 November 2013 17:07:42 Helge Deller wrote:
>> locale-gen from glibc showed a strange problem on parisc:
> 
> pedantic: locale-gen is from distros while glibc itself provides localedef.

Ok.

>> mmap2(NULL, 536870912, PROT_NONE, MAP_SHARED, 3, 0) = 0x42a54000
>> mmap2(0x42a54000, 103860, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED, 3, 0)
>> = -1 EINVAL (Invalid argument)
>>
>> Basically it was just trying to re-mmap() a file at the same address
>> which it was given by a previous mmap() call. But this remapping failed
>> with EINVAL.
>>
>> The problem is, that when MAP_FIXED and MAP_SHARED flags were used, we
>> didn't included the mapping-based offset when we verified the alignment of
>> the given fixed address against the offset which we calculated it in the
>> previous call.
> 
> hmm, which version of glibc were you trying ?  we landed this change recently:
> https://sourceware.org/git/?p=glibc.git;a=commitdiff_plain;h=17db6e8d6b12f55e312fcab46faf5d332c806fb6
> 
> it's part of glibc-2.18.  we know earlier versions of localedef failed on 
> systems like parisc, although it seemed to really only creep up on 64bit 
> kernels reliably.

It's debian's glibc 2.17-96

> based on your description, it sounds like independent bugs, but it also seems 
> a little coincidental ...

It's at least related.
As long as in the second mmap(ptr,...) the ptr value given is the same as the ptr value
returned by a prior call to mmap(NULL,...) it works.
It's hard to read from the diff if this is the case with the new glibc patch...

Anyway, I think the main thing one should know is, that a mmap(NULL,..) call will in most
cases *not* return a value which is a multiple of SHMLBA. But the offset (ptr%SHMLBA) 
will stay constant as long as the same file handle is used.

Helge
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSk7Z5AAoJEKGDlV8wpRJBPeQP/RndkCBrSEwNbH5HWFS332Lw
CJ/l5QkG2rhZh1EYKywoxqlYeTR3vDveacCQEuOozeVm7zI50leZACFI8XromoTb
3MQ0oPy3X+ApP+X/+KYSO/5ew+bRMcJJ3bmid9jdZ8kA3U4h6nsdE1dd3tJl1sf0
10Jm/0OPz+x0mJBZU8V4cuZPllHsyvfk7IN/vnfhy3HdW/Jq2IPPE+XZhri+3SQu
qBSnZOVts6MS3tlC757GgKhGGLjer4hVrJaC4BQucSoAk3fEJZqXQzcs9HDIWTNd
k4kkan6fnUdvxpvZOO//TYeAYDZQvgMiYju/CMw35B+Iyk9Anma3oXxtTQTbaFh5
zjPlsrCuU+V7oUag6WiLKKtlmUg07sdX+G4Htf64mT9avdQvuGwWlJieORg7Xnjw
/32tWjyw/j/v6+Omdq5tMf+Si7NeDi+AbE2IaRFrGL+/1ANXj2XoTRP2o34gMj90
7OL6/ntV/p+D/ByGmSlGbvSL7RzKnYxHNS1/reeEys+tS2j3+C0M+7GDm1/r3BUV
hXVh0eFizr1p2tDsabCEbil3lEffVPXJ12mVY+HDhe7KGav66suNvhaqfcou07Yr
j9arKCVZyjbXkOmTjRXrGpgs697yqail100U3VuNXgrdBJz98oHbSB4t1jG5K/CW
TePZPuUjmHyjWTwyFRPp
=IeWC
-----END PGP SIGNATURE-----