This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [BZ #10283] localedef: align fixed maps to SHMLBA


On 05/23/2013 12:47 PM, Mike Frysinger wrote:
> Many Linux arches require fixed mmaps to be aligned higher than pagesize,
> so use the SHMLBA define as it represents this quantity exactly.
> 
> This fixes spurious errors seen on those arches like:
> cannot map archive header: Invalid argument

I had a deeper think about this issue and I don't know
that you can rely on glibc's SHMLBA to do this correctly.

See:
http://lkml.indiana.edu/hypermail/linux/kernel/0309.1/1005.html

Which says that the kernel and glibc values for SHMLBA don't
match.

We need correct SHMLBA values in order to ensure that the kernel
will be less likely to return EINVAL for an mmap with a fixed
address.

Can you confirm that ARM, m68k, and SH have valid matching
SHMLBA values in the Linux headers?

Note: This has little to do with coherency issues because
we aren't writing to the localearchive mapping. However, the
kernel for certain arches might reject a mapping under
certain conditions which are sufficiently esoteric to
determine that we want to just try to use SHMLBA to avoid
any problems with the code that checks for aliasing.

> URL: http://sourceware.org/bugzilla/show_bug.cgi?id=10283
> Reported-by: CHIKAMA Masaki <masaki.chikama@gmail.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> 2013-05-23  Mike Frysinger  <vapier@gentoo.org>
> 

BZ number missing.

> 	* locale/programs/locarchive.c: Include sys/shm.h.
> 	(prepare_address_space): Align p to SHMLBA.
> 	(file_data_available_p): Replace pagesz with SHMLBA.
> ---
> note: seems to be passing on x86_64, but need to do a build on an affected
> 	arch to make sure things are OK.
> 
>  locale/programs/locarchive.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/locale/programs/locarchive.c b/locale/programs/locarchive.c
> index d31472d..09bd4c8 100644
> --- a/locale/programs/locarchive.c
> +++ b/locale/programs/locarchive.c
> @@ -37,6 +37,7 @@
>  #include <stdint.h>
>  #include <sys/mman.h>
>  #include <sys/param.h>
> +#include <sys/shm.h>
>  #include <sys/stat.h>
>  
>  #include "../../crypt/md5.h"
> @@ -85,11 +86,14 @@ prepare_address_space (int fd, size_t total, size_t *reserved, int *xflags)
>      {
>        void *p = mmap64 (NULL, RESERVE_MMAP_SIZE, PROT_NONE, MAP_SHARED, fd, 0);
>        if (p != MAP_FAILED)
> -        {
> -          *reserved = RESERVE_MMAP_SIZE;
> -          *xflags = MAP_FIXED;
> -          return p;
> -        }
> +	{

This math doesn't look correct.

> +	  /* Some arches require fixed mappings to be aligned higher than
> +	     pagesize, and SHMLBA represents that size.  */
> +	  size_t align_adjust = (uintptr_t)p & SHMLBA;

This is just the number of bytes since the last address, below p,
which was aligned to SHMLBA, on some arches this is just
going to be zero, but on others it won't.

You want:

size_t aligned_p = ALIGN_UP(p, SHMLBA);
size_t align_adjust = aligned_p - p;

> +	  *reserved = RESERVE_MMAP_SIZE - align_adjust;

It's possible that one day we make RESERVER_MMAP_SIZE smaller
and this becomes negative, can you assert that it's bigger
than ((SHMLBA - 1) + total) which is the most size we need
accounting for alignment?

> +	  *xflags = MAP_FIXED;
> +	  return p + align_adjust;

return aligned_p;

For ALIGN_UP see:
http://www.sourceware.org/ml/libc-alpha/2012-04/msg00006.html
http://sourceware.org/ml/libc-alpha/2012-09/msg00426.html

I would *IMMEDIATELY* approve any patch to add ALIGN_UP and ALIGN_DOWN
macros to make this mess of bespoke mangling go away :-)

> +	}
>      }
>  
>    *reserved = total;
> @@ -271,8 +275,7 @@ file_data_available_p (struct locarhandle *ah, uint32_t offset, uint32_t size)
>    if (st.st_size > ah->reserved)
>      return false;
>  
> -  const size_t pagesz = getpagesize ();
> -  size_t start = ah->mmaped & ~(pagesz - 1);
> +  size_t start = ah->mmaped & ~(SHMLBA - 1);

OK.

>    void *p = mmap64 (ah->addr + start, st.st_size - start,
>  		    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
>  		    ah->fd, start);
> 

One problem. In the old code ah->addr and p were the same, now they
are not. Now when the code tries to enlarge the archive via:

 333   else
 334     {
 335       munmap (ah->addr, ah->reserved);
 336       ah->addr = mmap64 (NULL, st.st_size, PROT_READ | PROT_WRITE,
 337                          MAP_SHARED, ah->fd, 0);
 338       ah->reserved = st.st_size;
 339       head = ah->addr;
 340     }

We are going to leave a small stub of align_adjust still mapped?

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]