SHMLBA and compat tasks

The current Linux implementation of shmat() insists on SHMLBA alignment even when shmflg & SHM_RND == 0. This is not consistent with the man pages and the single UNIX spec, which require only a page-aligned address.

man page:

This is not a problem for many platforms because SHMLBA == PAGE_SIZE. It's also not a problem for 64 bit programs running on 64 bit platforms, because arch_get_unmapped_area() typically guarantees SHMLBA alignment, even when SHM_RND is not asserted.

However, for 32 bit compatiblity programs, an innocuous sequence such as:

p = shmat(id, 0, shmflg) /* shmflag & SHM_RND == 0 */ /* p is page-aligned, but may not be SHMLBA aligned */ shmdt(p)
shmat(id, p, shmflg) == -EINVAL ??

can fail. The attached patch fixes the problem for ia64. Can other arch maintainers confirm that it doesn't break anything for you ?

-Arun

@@ -600,7 +600,7 @@
 		if (addr & (SHMLBA-1)) {
 			if (shmflg & SHM_RND)
 				addr &= ~(SHMLBA-1);	   /* round down */
-			else
+			else if (addr & ~PAGE_MASK)
 				return -EINVAL;
 		}
 		flags = MAP_SHARED | MAP_FIXED;

When the user specifies a specific address, things get real interesting because of cache aliasing issues.

Platforms with virtually indexed caches use SHMLBA of a size such that it is the largest cache virtual aliasing factor. In this way, processes with different SHM mappings of the shared writable memory will immediately see writes done by other processes and aliases cannot enter the cache due to virtual addresses being "just right".

Otherwise we'll need to special case now this SHM_RND thing and mark PTE's as non-cacheable and other horrible things like this, and in fact this particular scheme is not even possible on some architectures.

So your proposal is going to break things for some people, sparc64, mips, parisc, and probably a slew of others.

I read that to mean that there is a correctness problem, apart from performance issues on some platforms.

How about something like:

else
#ifndef ARCH_HAS_VCACHE
if (addr & ~PAGE_MASK)
#endif
return -EINVAL;

So that platforms with virtually indexed caches are not affected ?

-Arun

By your own admittance this will break the conformance in question on such architectures, and it's also bad to have different rules like this in general.

Why don't we declare that SHM_LBA must be abided by on all platforms? POSIX is not a high and mighty law which we must follow blindly, to me this restriction is nonsensicle and supporting it fully will cause more grief than just declaring that we don't support it.

I assume you're proposing changing the man page to say that SHMLBA will be forced, in all cases (even though SHM_RND is not specified). Yes, that would make the implementation consistent with the man page, so it'd be an improvement over the current situation.

But ia32 on ia64 doesn't match this declaration today.

shmat(id, 0, ...)

can return an address that's PAGE_SIZE aligned, but not SHMLBA aligned.

David, are you ok with changing this code to force SHMLBA alignment for both 32 and 64 bit tasks ?

-Arun

arch/ia64/kernel/sys_ia64.c:

arch_get_unmapped_area(...)
{
...
if (map_shared && (TASK_SIZE > 0xfffffffful))
/*
* For 64-bit tasks, align shared segments to 1MB to avoid potential
* performance penalty due to virtual aliasing (see ASDM). For 32-bit
* tasks, we prefer to avoid exhausting the address space too quickly by
* limiting alignment to a single page.
*/
align_mask = SHMLBA - 1;

Arun> David, are you ok with changing this code to force SHMLBA Arun> alignment for both 32 and 64 bit tasks ?

I assume you're asking me here (too many David's!). The answer is that it's NOT OK to force 1MB alignment for IA-32 tasks. It'll eat up address space like there is no tomorrow. In fact, the reason the code is there is because some versions of the open-office installer used to crash due to address-space exhaustion. Probably a bug in open-office, but the point is that for ia32-emulation purposes, we want/need to be bug-for-bug compatible with x86.

	--david

Why exactly is SHMLBA 1MB, something to do with the ia64 address space segments or something?

DaveM> On Mon, 1 Mar 2004 11:41:01 -0800

DaveM> Why exactly is SHMLBA 1MB, something to do with the ia64 address DaveM> space segments or something?

The architecture reserves the right to impose a performance penalty for aliases which are not congruent modulo 1MB. None of the current CPUs do that but future CPUs _could_ do that (well, I hope they never will, but we do want to be compliant with the architecture, just in case).

	--david

Virtual name cache aliasing ? :)

Sounds like we have sufficient grounds for using different alignment rules for different architectures ?

-Arun

#ifndef COMPAT_SHMLBA
#define COMPAT_SHMLBA SHMLBA
#endif

should be enough, no?

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

I'm concerned that we'll have to duplicate/inline much of sys_shmat() in ipc/compat.c and it might become a maintenance problem. Perhaps there is a way to do it in a better way, but I don't see it.

An ifdef in sys_shmat() that distinugishes between architectures that enforce SHMLBA in all cases and the ones which don't would be ideal for my purposes.

-Arun

Since nobody objected to this approach, I'd like to propose this patch.

-Arun

Index: linux-2.6-cvs/include/asm-mips/shmparam.h
===================================================================
RCS file: /home/adsharma/disk2/cvs/linux-2.5/include/asm-mips/shmparam.h,v retrieving revision 1.3

diff -d -u -r1.3 shmparam.h
@@ -6,6 +6,8 @@
 #ifndef _ASM_SHMPARAM_H
 #define _ASM_SHMPARAM_H
 
+#define __ARCH_FORCE_SHMLBA	1
+
 #define	SHMLBA 0x40000			/* attach addr a multiple of this */
 
 #endif /* _ASM_SHMPARAM_H */

Index: linux-2.6-cvs/include/asm-parisc/shmparam.h
===================================================================
RCS file: /home/adsharma/disk2/cvs/linux-2.5/include/asm-parisc/shmparam.h,v retrieving revision 1.3

diff -d -u -r1.3 shmparam.h
@@ -1,6 +1,8 @@
 #ifndef _ASMPARISC_SHMPARAM_H
 #define _ASMPARISC_SHMPARAM_H
 
+#define __ARCH_FORCE_SHMLBA 	1
+
 #define SHMLBA 0x00400000   /* attach addr needs to be 4 Mb aligned */
 
 #endif /* _ASMPARISC_SHMPARAM_H */

Index: linux-2.6-cvs/include/asm-sparc/shmparam.h
===================================================================
RCS file: /home/adsharma/disk2/cvs/linux-2.5/include/asm-sparc/shmparam.h,v retrieving revision 1.2

diff -d -u -r1.2 shmparam.h
@@ -2,6 +2,8 @@
 #ifndef _ASMSPARC_SHMPARAM_H
 #define _ASMSPARC_SHMPARAM_H
 
+#define __ARCH_FORCE_SHMLBA 	1
+
 extern int vac_cache_size;
 #define SHMLBA (vac_cache_size ? vac_cache_size : \
 		(sparc_cpu_model == sun4c ? (64 * 1024) : \

Index: linux-2.6-cvs/include/asm-sparc64/shmparam.h
===================================================================
RCS file: /home/adsharma/disk2/cvs/linux-2.5/include/asm-sparc64/shmparam.h,v retrieving revision 1.3

diff -d -u -r1.3 shmparam.h
@@ -4,6 +4,7 @@
 
 #include <asm/spitfire.h>
 
+#define __ARCH_FORCE_SHMLBA	1
 /* attach addr a multiple of this */
 #define	SHMLBA	((PAGE_SIZE > L1DCACHE_SIZE) ? PAGE_SIZE : L1DCACHE_SIZE)
 

Index: linux-2.6-cvs/ipc/shm.c
===================================================================
RCS file: /home/adsharma/disk2/cvs/linux-2.5/ipc/shm.c,v retrieving revision 1.32

diff -d -u -r1.32 shm.c
@@ -655,8 +655,11 @@
 		if (addr & (SHMLBA-1)) {
 			if (shmflg & SHM_RND)
 				addr &= ~(SHMLBA-1);	   /* round down */
-			else
-				return -EINVAL;
+			else 
+#ifndef __ARCH_FORCE_SHMLBA
+				if (addr & ~PAGE_MASK)
+#endif
+					return -EINVAL;
 		}
 		flags = MAP_SHARED | MAP_FIXED;
 	} else {

I'm not going to object to this patch, although I dearly wished all platforms would behave consistently.