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

["Carlos O'Donell" <carlos at redhat dot com>]

On 05/25/2013 02:41 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

This version is looking much better, one more iteration and we're
done.

> 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 #10283]
> 	* include/sys/cdefs.h (ALIGN_DOWN): New helper macro.
> 	(ALIGN_UP): Likewise.
> 	(PTR_ALIGN_DOWN): Likewise.
> 	(PTR_ALIGN_UP): Likewise.
> 	* locale/locarchive.h (struct locarhandle): Add mmap_base and mmap_len.
> 	* locale/programs/locarchive.c: Include sys/shm.h.
> 	(prepare_address_space): Take two new outputs (the mmap base and len).
> 	Align p to SHMLBA.  Set mmap base and len to the right values.
> 	(create_archive): Declare new mmap base and len values for
> 	prepare_address_space, and store the result in ah.
> 	(file_data_available_p): Replace pagesz with SHMLBA.
> 	(enlarge_archive): If ah->mmap_base is not NULL, use that and
> 	ah->mmap_len to unmap rather than ah->addr and ah->reserved.
> 	Declare new mmap base and len values for
> 	prepare_address_space, and store the result in new_ah.
> 	(open_archive): Declare new mmap base and len values for
> 	prepare_address_space, and store the result in ah.
> 	(close_archive): If ah->mmap_base is not NULL, use that and
> 	ah->mmap_len to unmap rather than ah->addr and ah->reserved.
> ---
> v2
> 	- tested on x86_64 by forcing SHMLBA to larger values and seems to work
> 
>  include/sys/cdefs.h          | 18 +++++++++++++
>  locale/locarchive.h          |  2 ++
>  locale/programs/locarchive.c | 62 ++++++++++++++++++++++++++++++++------------
>  3 files changed, 66 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sys/cdefs.h b/include/sys/cdefs.h
> index 524fe57..52a2667 100644
> --- a/include/sys/cdefs.h
> +++ b/include/sys/cdefs.h
> @@ -15,4 +15,22 @@ rtld_hidden_proto (__chk_fail)
>  
>  #endif
>  
> +/* Align a value by rounding down to closest size.
> +   e.g. Using size of 4096, we get this behavior:
> +	{4095, 4096, 4097} = {0, 4096, 4096}.  */
> +#define ALIGN_DOWN(base, size)	((base) & ~((size) - 1))
> +
> +/* Align a value by rounding up to closest size.
> +   e.g. Using size of 4096, we get this behavior:
> +	{4095, 4096, 4097} = {4096, 4096, 8192}.  */
> +#define ALIGN_UP(base, size)	ALIGN_DOWN((base) + (size) - 1, (size))
> +
> +/* Same as ALIGN_DOWN(), but automatically casts when base is a pointer.  */
> +#define PTR_ALIGN_DOWN(base, size) \
> +  (void *) ALIGN_DOWN ((uintptr_t) (base), (size))
> +
> +/* Same as ALIGN_DOWN_UP(), but automatically casts when base is a pointer.  */

Cut-and-pasto "Same as ALIGN_UP(), ..."

> +#define PTR_ALIGN_UP(base, size) \
> +  (void *) ALIGN_UP ((uintptr_t) (base), (size))

Almost done.

We install cdefs.h and thus these macros need __ to avoid being 
in the user's namespace.

The benefit is that we'll be able to use them *everywhere*, 
even in macros in headers we expose to userspace, which
is what we want.

Please split this out as patch 1/2, repost and I'll ACK.

> +
>  #endif
> diff --git a/locale/locarchive.h b/locale/locarchive.h
> index db05603..7cdeda6 100644
> --- a/locale/locarchive.h
> +++ b/locale/locarchive.h
> @@ -81,9 +81,11 @@ struct locrecent
>  struct locarhandle
>  {
>    int fd;
> +  void *mmap_base;

Needs a comment about what this is.

>    void *addr;
>    size_t mmaped;
>    size_t reserved;
> +  size_t mmap_len;

Likewise.

>  };
>  
>  
> diff --git a/locale/programs/locarchive.c b/locale/programs/locarchive.c
> index d31472d..53bea55 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"
> @@ -79,21 +80,31 @@ static const char *locnames[] =
>     mapping affects the address selection.  So do this mapping from the
>     actual file, even though it's only a dummy to reserve address space.  */
>  static void *
> -prepare_address_space (int fd, size_t total, size_t *reserved, int *xflags)
> +prepare_address_space (int fd, size_t total, size_t *reserved, int *xflags,
> +		       void **mmap_base, size_t *mmap_len)
>  {
>    if (total < RESERVE_MMAP_SIZE)
>      {
>        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;
> -        }
> +	{
> +	  /* Some arches require fixed mappings to be aligned higher than
> +	     pagesize, and SHMLBA represents that size.  */
> +	  void *aligned_p = PTR_ALIGN_UP (p, SHMLBA);
> +	  size_t align_adjust = aligned_p - p;
> +	  *mmap_base = p;
> +	  *mmap_len = RESERVE_MMAP_SIZE;
> +	  assert (align_adjust < RESERVE_MMAP_SIZE);
> +	  *reserved = RESERVE_MMAP_SIZE - align_adjust;
> +	  *xflags = MAP_FIXED;
> +	  return aligned_p;
> +	}

OK.

>      }
>  
>    *reserved = total;
>    *xflags = 0;
> +  *mmap_base = NULL;
> +  *mmap_len = 0;


OK.

>    return NULL;
>  }
>  
> @@ -151,9 +162,11 @@ create_archive (const char *archivefname, struct locarhandle *ah)
>        error (EXIT_FAILURE, errval, _("cannot resize archive file"));
>      }
>  
> -  size_t reserved;
> +  size_t reserved, mmap_len;
>    int xflags;
> -  void *p = prepare_address_space (fd, total, &reserved, &xflags);
> +  void *mmap_base;
> +  void *p = prepare_address_space (fd, total, &reserved, &xflags, &mmap_base,
> +				   &mmap_len);

OK.

>  
>    /* Map the header and all the administration data structures.  */
>    p = mmap64 (p, total, PROT_READ | PROT_WRITE, MAP_SHARED | xflags, fd, 0);
> @@ -199,6 +212,8 @@ create_archive (const char *archivefname, struct locarhandle *ah)
>      }
>  
>    ah->fd = fd;
> +  ah->mmap_base = mmap_base;
> +  ah->mmap_len = mmap_len;

OK.

>    ah->addr = p;
>    ah->mmaped = total;
>    ah->reserved = reserved;
> @@ -271,8 +286,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);

__ALIGN_DOWN(ah->mmaped, SHMLBA)?

i.e. We map in additional pages at multiples of SHMLBA, but starting below what we have mapped
     to ensure no gaps.

>    void *p = mmap64 (ah->addr + start, st.st_size - start,
>  		    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
>  		    ah->fd, start);
> @@ -332,10 +346,15 @@ enlarge_archive (struct locarhandle *ah, const struct locarhead *head)
>  		       MAP_SHARED | MAP_FIXED, ah->fd, 0);
>    else
>      {
> -      munmap (ah->addr, ah->reserved);
> +      if (ah->mmap_base)
> +	munmap (ah->mmap_base, ah->mmap_len);
> +      else
> +	munmap (ah->addr, ah->reserved);

OK.

>        ah->addr = mmap64 (NULL, st.st_size, PROT_READ | PROT_WRITE,
>  			 MAP_SHARED, ah->fd, 0);
>        ah->reserved = st.st_size;
> +      ah->mmap_base = NULL;
> +      ah->mmap_len = 0;

OK.

>        head = ah->addr;
>      }
>    if (ah->addr == MAP_FAILED)
> @@ -401,9 +420,11 @@ enlarge_archive (struct locarhandle *ah, const struct locarhead *head)
>        error (EXIT_FAILURE, errval, _("cannot resize archive file"));
>      }
>  
> -  size_t reserved;
> +  size_t reserved, mmap_len;
>    int xflags;
> -  void *p = prepare_address_space (fd, total, &reserved, &xflags);
> +  void *mmap_base;
> +  void *p = prepare_address_space (fd, total, &reserved, &xflags, &mmap_base,
> +				   &mmap_len);

OK.

>  
>    /* Map the header and all the administration data structures.  */
>    p = mmap64 (p, total, PROT_READ | PROT_WRITE, MAP_SHARED | xflags, fd, 0);
> @@ -423,6 +444,8 @@ enlarge_archive (struct locarhandle *ah, const struct locarhead *head)
>      }
>  
>    new_ah.mmaped = total;
> +  new_ah.mmap_base = mmap_base;
> +  new_ah.mmap_len = mmap_len;

OK.

>    new_ah.addr = p;
>    new_ah.fd = fd;
>    new_ah.reserved = reserved;
> @@ -606,9 +629,11 @@ open_archive (struct locarhandle *ah, bool readonly)
>    ah->fd = fd;
>    ah->mmaped = st.st_size;
>  
> -  size_t reserved;
> +  size_t reserved, mmap_len;
>    int xflags;
> -  void *p = prepare_address_space (fd, st.st_size, &reserved, &xflags);
> +  void *mmap_base;
> +  void *p = prepare_address_space (fd, st.st_size, &reserved, &xflags,
> +				   &mmap_base, &mmap_len);

OK.

>  
>    /* Map the entire file.  We might need to compare the category data
>       in the file with the newly added data.  */
> @@ -620,6 +645,8 @@ open_archive (struct locarhandle *ah, bool readonly)
>        error (EXIT_FAILURE, errno, _("cannot map archive header"));
>      }
>    ah->reserved = reserved;
> +  ah->mmap_base = mmap_base;
> +  ah->mmap_len = mmap_len;

OK.

>  }
>  
>  
> @@ -628,7 +655,10 @@ close_archive (struct locarhandle *ah)
>  {
>    if (ah->fd != -1)
>      {
> -      munmap (ah->addr, ah->reserved);
> +      if (ah->mmap_base)
> +	munmap (ah->mmap_base, ah->mmap_len);
> +      else
> +	munmap (ah->addr, ah->reserved);

OK.

>        close (ah->fd);
>      }
>  }
> 

Repost second half as 2/2 please.

Cheers,
Carlos.