SHMLBA and compat tasks

SHMLBA and compat tasks

[Arun Sharma]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

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:

> If shmaddr isn’t NULL and SHM_RND is asserted  in  shmflg,  the  attach
> occurs at the address equal to shmaddr rounded down to the nearest mul-
> tiple of SHMLBA.  Otherwise shmaddr must be a page-aligned  address  at
> which the attach occurs.

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

[-- Attachment #2: shmat.patch --]
[-- Type: text/plain, Size: 339 bytes --]

--- linux/ipc/shm.c.orig	2003-04-28 11:19:24.000000000 -0700
+++ linux/ipc/shm.c	2003-05-09 13:26:13.000000000 -0700
@@ -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;

Re: SHMLBA and compat tasks

[David S. Miller]

On Fri, 27 Feb 2004 17:41:28 -0800
Arun Sharma <arun.sharma@intel.com> wrote:

> man page:
> 
> > If shmaddr isn’t NULL and SHM_RND is asserted  in  shmflg,  the  attach
> > occurs at the address equal to shmaddr rounded down to the nearest mul-
> > tiple of SHMLBA.  Otherwise shmaddr must be a page-aligned  address  at
> > which the attach occurs.

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.

Re: SHMLBA and compat tasks

[Arun Sharma]

On Sat, Feb 28, 2004 at 03:55:29PM -0800, David S. Miller wrote:
> 
> 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".

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

Re: SHMLBA and compat tasks

[David S. Miller]

On Sat, 28 Feb 2004 18:11:06 -0800
Arun Sharma <arun.sharma@intel.com> wrote:

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

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.

Re: SHMLBA and compat tasks

[Arun Sharma]

On Sun, Feb 29, 2004 at 09:57:52PM -0800, David S. Miller wrote:
> 
> Why don't we declare that SHM_LBA must be abided by on all platforms?

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;

Re: SHMLBA and compat tasks

[David Mosberger]

>>>>> On Mon, 1 Mar 2004 11:33:08 -0800, Arun Sharma <arun.sharma@intel.com> said:

  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

Re: SHMLBA and compat tasks

[David S. Miller]

On Mon, 1 Mar 2004 11:41:01 -0800
David Mosberger <davidm@napali.hpl.hp.com> wrote:

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

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

Re: SHMLBA and compat tasks

[Arun Sharma]

On Mon, Mar 01, 2004 at 11:41:01AM -0800, David Mosberger wrote:
> 
> I assume you're asking me here (too many David's!).

Virtual name cache aliasing ? :)

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

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

	-Arun

Re: SHMLBA and compat tasks

[David Mosberger]

>>>>> On Mon, 1 Mar 2004 12:11:22 -0800, "David S. Miller" <davem@redhat.com> said:

  DaveM> On Mon, 1 Mar 2004 11:41:01 -0800
  DaveM> David Mosberger <davidm@napali.hpl.hp.com> wrote:

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

  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

Re: SHMLBA and compat tasks

[Matthew Wilcox]

On Mon, Mar 01, 2004 at 12:16:13PM -0800, Arun Sharma wrote:
> Sounds like we have sufficient grounds for using different alignment
> rules for different architectures ?

#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

Re: SHMLBA and compat tasks

[Arun Sharma]

On Mon, Mar 01, 2004 at 08:55:10PM +0000, Matthew Wilcox wrote:
> On Mon, Mar 01, 2004 at 12:16:13PM -0800, Arun Sharma wrote:
> > Sounds like we have sufficient grounds for using different alignment
> > rules for different architectures ?
> 
> #ifndef COMPAT_SHMLBA
> #define COMPAT_SHMLBA SHMLBA
> #endif
> 
> should be enough, no?

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

Re: SHMLBA and compat tasks

[Arun Sharma]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Tue, Mar 02, 2004 at 11:43:02AM -0800, Arun Sharma wrote:
> 
> 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.
> 

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

	-Arun


[-- Attachment #2: shmat.patch --]
[-- Type: text/plain, Size: 3064 bytes --]

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
--- linux-2.6-cvs/include/asm-mips/shmparam.h	23 Jun 2003 19:07:17 -0000	1.3
+++ linux-2.6-cvs/include/asm-mips/shmparam.h	3 Mar 2004 22:06:48 -0000
@@ -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
--- linux-2.6-cvs/include/asm-parisc/shmparam.h	30 Oct 2002 23:00:20 -0000	1.3
+++ linux-2.6-cvs/include/asm-parisc/shmparam.h	3 Mar 2004 22:06:55 -0000
@@ -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
--- linux-2.6-cvs/include/asm-sparc/shmparam.h	5 Feb 2002 17:40:40 -0000	1.2
+++ linux-2.6-cvs/include/asm-sparc/shmparam.h	3 Mar 2004 22:07:08 -0000
@@ -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
--- linux-2.6-cvs/include/asm-sparc64/shmparam.h	5 Feb 2002 20:20:15 -0000	1.3
+++ linux-2.6-cvs/include/asm-sparc64/shmparam.h	3 Mar 2004 22:07:01 -0000
@@ -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
--- linux-2.6-cvs/ipc/shm.c	31 Aug 2003 18:32:59 -0000	1.32
+++ linux-2.6-cvs/ipc/shm.c	3 Mar 2004 22:04:43 -0000
@@ -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 {

Re: SHMLBA and compat tasks

[David S. Miller]

On Fri, 5 Mar 2004 18:37:40 -0800
Arun Sharma <arun.sharma@intel.com> wrote:

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

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