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

[Mike Frysinger <vapier at gentoo dot org>]

On Thursday 23 May 2013 19:11:14 Carlos O'Donell wrote:
> 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?

i don't think we need to block this fix on making sure SHMLBA is defined 
correctly for all targets.  no arch defines it less than page size, so there'd 
be no difference

> > +	  /* 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.

it's not even that :).  SHMLBA will usually have just one bit set, so it'll be 
masking off just that one.  i meant to use % here.

> > +	  *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 :-)

i did a `git grep` for some ALIGN macro and was a bit surprised they didn't 
already exist.  but i get the feeling that glibc historically wasn't big on 
providing convenience macros and instead preferred to open code everything.

> >    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:

i did a bit of tracing and things looked ok, but i could have easily missed 
something since i've never looked at this code, and it's a bit convoluted.
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.