Re: bugs in page colouring code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/14/2012 09:20 AM, Russell King - ARM Linux wrote:
On Wed, Jun 13, 2012 at 03:29:36PM -0400, Rik van Riel wrote:
COLOUR_ALIGN_DOWN can use the pgoff % shm_align_mask either positively
    or negatively, depending on the address initially found by
    get_unmapped_area

static inline unsigned long COLOUR_ALIGN_DOWN(unsigned long addr,
                                               unsigned long pgoff)
{
         unsigned long base = addr&  ~shm_align_mask;
         unsigned long off = (pgoff<<  PAGE_SHIFT)&  shm_align_mask;

         if (base + off<= addr)
                 return base + off;

         return base - off;
}

Yes, that is bollocks code, introduced by this commit:

commit 7dbaa466780a754154531b44c2086f6618cee3a8
Author: Rob Herring<rob.herring@xxxxxxxxxxx>
Date:   Tue Nov 22 04:01:07 2011 +0100

It's not just ARM that has this bug. It appears to
be cut'n'pasted from other architectures (MIPS? SPARC?).

The fix would be to return an address that is a whole shm_align_mask
lower: (((base - shm_align_mask)&  ~shm_align_mask) + off

Yes, agreed.

I will make sure the arch-independent colouring
code does that.

This brings up the question: should a MAP_PRIVATE mapping see updates
to the backing file made via a shared mapping and/or writing the file
directly?  After all, a r/w MAP_PRIVATE mapping which has been CoW'd
won't see the updates.

So I'd argue that a file mapped MAP_SHARED must be mapped according to
the colour rules, but a MAP_PRIVATE is free not to be so.

OK, fair enough.

Secondly, MAP_FIXED never checks for page colouring alignment.
I assume the cache aliasing on AMD Bulldozer is merely a performance
issue, and we can simply ignore page colouring for MAP_FIXED?
That will be easy to get right in an architecture-independent
implementation.

There's a whole bunch of issues with MAP_FIXED, specifically address
space overflow has been discussed previously, and resulted in this patch:

[PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

Turns out, get_unmapped_area_prot (the function
that calls arch_get_unmapped_area) checks for
these overflows, so we should be fine.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]