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

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

On 05/23/2013 09:22 PM, Mike Frysinger wrote:
> 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

OK, I buy that.

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

Right, which is still wrong, which is why I want convenience macros
so I don't have to think very hard when it's past my bed time.

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

Sorry, I didn't imply that you were at fault here the PROJECT is at fault.
We absolutely need convenience macros for this.

I am saddened by bespoke implementations of things like ALIGN_UP and 
ALIGN_DOWN :-(

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

It needs more review then. I think we're close to a solution, but it needs
to have the edges filed off :-)

Cheers,
Carlos.