Re: bugs in page colouring code

Paul Mundt
2012-06-14T08:43:08Z

On Wed, Jun 13, 2012 at 03:29:36PM -0400, Rik van Riel wrote:
> ARM & MIPS seem to share essentially the same page colouring code, with
> these two bugs:
>
> 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;
> }
>
> The fix would be to return an address that is a whole shm_align_mask
> lower: (((base - shm_align_mask) & ~shm_align_mask) + off

'addr' in this case is already adjusted by callers of COLOUR_ALIGN_DOWN(), so
this shouldn't be an issue, unless I'm missing something?

> The second bug relates to MAP_FIXED mappings of files. In the
> MAP_FIXED conditional, arch_get_unmapped_area(_topdown) checks
> whether the mapping is colour aligned, but only for MAP_SHARED
> mappings.
>
> /*
> * We do not accept a shared mapping if it would violate
> * cache aliasing constraints.
> */
> if ((flags & MAP_SHARED) &&
> ((addr - (pgoff << PAGE_SHIFT)) & shm_align_mask))
> return -EINVAL;
>
These observations hold true for other architectures, too. I modelled the
SH implementation off of both MIPS and sparc, where these same patterns
exist. I would be surprised if there are any architectures that do
colouring in a different way.

The logic is such that in the MAP_FIXED case we can't align addr on to some
other boundary, and so anything that violates the aliasing constraints fails.
This is a departure from POSIX, and does occasionally lead to people sending in
patches to "correct" the behaviour for the LTP mmap01 testcase which does
iterative MAP_FIXED|MAP_SHARED PAGE_SIZE apart.

> This fails to take into account that the same file might be mapped
> MAP_SHARED from some programs, and MAP_PRIVATE from another. The
> fix could be a simple as always enforcing colour alignment when we
> are mmapping a file (filp is non-zero).
>
If that combination is possible then defaulting to colour alignment seems
reasonable. Whether that combination is reasonable or not is another matter.