I'm sure the patch fixes it as you say on arm-linux.  What I'd
like to know is, does it still work correctly on the other
supported linuxes ({x86,amd64,ppc32,ppc64}-linux) ?

I'm pretty sure the shmctl bit of the patch is an out and out bug in our current code on all linux platforms to be honest.

If VKI_IPC_64 is defined then we pass a vki_shmid64_ds struct to the kernel but we don't set the VKI_IPC_64 flag in the command so the kernel will actually return a vki_shmid_ds struct which (unless we are very lucky) will not have the segment size in the right place.

(In reply to comment #2)
Ok, so .. you reckon we should land it as-is, then?

The second half of the patch, yes. The first half is obviously a lot nastier (although also clearly ARM only).

Looks like AMD64 uses vki_shmid64_ds struct by default but does not accept VKI_IPC_64 flag. So this patch will break shmat on AMD64 as it would try to pass IPC_64 flag to shmctl which will not accept it.

So should I guard my changes in syswrap-generic.c with #ifdef VGA_arm ? Or is there a better solution?

Why do you think that? I looked at ipc/shm.c in the kernel and IPC_64 appears to be supported everywhere...

Hmm... I see, it's down to ipc_parse_version() which always returns IPC_64 on amd64 as __ARCH_WANT_IPC_PARSE_VERSION is not defined.

That also means that the bit is not cleared, which means the command demultiplexor would fail if it was set.

So on amd64 we don't want IPC_64 but on x86, ppc32 and ppc64 we do, or we will get back an old style 32 bit structure which isn't what we're expecting.

Created attachment 40853 [details]
fix shmat for arm target v2

Ok, so it should look like this. I've tried this patch on a simple program using shmat on {x86,amd64,ppc32,ppc64,arm}-linux.

My changes are under "#ifdef __NR_shmctl" which is true only for arm-linux, amd64-linux and darwin, so {x86,ppc32,ppc64}-linux should not be affected. Unfortunately I have no darwin targets around so I can not test this patch on it.

(In reply to comment #8)
> Created an attachment (id=40853) [details]

Ok for the second half of the patch (get_shm_size), although I
would change #ifdef VGA_amd64 to #ifdef VGP_amd64_linux, so we
guarantee not to change amd64-anythingelse (eg amd64-darwin).

For the first half (shmat) I think this is the wrong fix.  Your code
causes the return value from ML_(generic_PRE_sys_shmat) to be ignored
except in the error case.  This means that the address that goes to
the kernel is what the application specified, and is not controlled
by Valgrind's address space manager.

Can you instead try this:

    if (arg2tmp == 0)
       SET_STATUS_Failure( VKI_EINVAL );
    else {
       ARG2 = arg2tmp;
       ARG3 |= (VKI_SHM_RND | VKI_SHM_REMAP);
    }

that is, force the kernel to accept the address in arg2tmp.  Does
that work?

(In reply to comment #8)
One other thing ..
> Ok, so it should look like this. I've tried this patch on a simple program
> using shmat on {x86,amd64,ppc32,ppc64,arm}-linux.

Can you attach this test program so it can maybe be added to the test
suite?

Created attachment 41000 [details]
A sample program using shmat

(In reply to comment #10)

(In reply to comment #9)

> Does that work?
No, it does not. The sample program segfaults when I run it under valgrind with --tool=memcheck.

The reason is that SHM_RND flag tells kernel to round down address to nearest multiple of SHMLBA, not PAGE_SIZE. For arm SHMLBA is 4 * PAGE_SIZE (unlike most of the other architectures where SHMLBA and PAGE_SIZE are equal). SHM_REMAP flag forces the kernel to map shared memory even on already occupied region, and segmentation fault is a result of this.

Also __ARCH_FORCE_SHMLBA is defined for arm, so fixed address passed to shmat should be not just PAGE_SIZE-aligned but SHMLBA-aligned, though kernel itself can and often does attach shared memory to the PARG_SIZE-aligned addresses.

So my statement
> Kernel of some versions for ARM does not accept fixed adress shmat until
> both SHM_RND and SHM_REMAP are specified.
is probably not correct as it was based on the assumption that any address chosen for shmat by kernel is a valid one to pass as an argument, which is not correct.

I think the best way to fix the problem is to modify VG_(am_get_advisory) in such a way that it returns address with more strict alignment constrains than PAGE_SIZE-aligned when needed. Is there any way to do this without serious interference with other valgrind parts?

(In reply to comment #12)

Thanks for the testing/details.

> I think the best way to fix the problem is to modify VG_(am_get_advisory) in
> such a way that it returns address with more strict alignment constrains than
> PAGE_SIZE-aligned when needed. Is there any way to do this without serious
> interference with other valgrind parts?

The concept of PAGE_SIZE alignment is deeply wired into the address
space manager.  I don't fancy messing with it.

A different possibility is to mess with the arguments before
calling VG_(am_get_advisory), like this (not sure of the details,
this is just illustration):

PRE(wrap_sys_shmat)
{
   UWord arg2tmp;
   PRINT("wrap_sys_shmat ( %ld, %#lx, %ld )",ARG1,ARG2,ARG3);
   PRE_REG_READ3(long, "shmat",
                 int, shmid, const void *, shmaddr, int, shmflg);

   // impose more strict alignment constraints
   if (ARG3 & VKI_SHM_RND)
      ARG2 = VG_ROUNDDN(ARG2, VKI_SHMLBA);

   arg2tmp = ML_(generic_PRE_sys_shmat)(tid, ARG1,ARG2,ARG3);
   if (arg2tmp == 0)
      SET_STATUS_Failure( VKI_EINVAL );
   else
      ARG2 = arg2tmp;
}

Can you try something like that?

(In reply to comment #13)

> A different possibility is to mess with the arguments before
> calling VG_(am_get_advisory), like this (not sure of the details,
> this is just illustration):
I think this fixes another part of the same problem. It helps to properly control the case when client program specifies address to map shared memory at and sets flag SHM_RND. I have no examples of this part of the problem yet but they probably exist.

In my case ARG2 is NULL and Valgrind needs to find any valid address to map this shared memory at. This address is usually provided by VG_(am_get_advisory). Modifying NULL before VG_(am_get_advisory) is meaningless since we do not know the valid address yet. Modifying result of VG_(am_get_advisory) does not help either since the only thing we can do at that moment is to say if it is valid or not. I do not see how we can modify invalid address to make it valid at that moment. Rounding up or down may cause intersections with occupied memory.

Created attachment 41010 [details]
fix shmat for arm target v3

(In reply to comment #14)

> Rounding up or down may cause intersections with occupied memory.
We can safely round up if we ask Valgrind's memory manager to allocate extra (VKI_SHMLBA - VKI_PAGE_SIZE) bytes.

I've tried this patch on sample program from comment #11 on {arm,x86,amd64}-linux. It worked fine.

There are really two different bugs here:

* the need to pass VKI_IPC_64 some times

* the need to round to SHMLBA on arm-linux

For the VKI_IPC_64 question I committed the relevant part of
the patch as r11398.

Main part (the ARM stuff) committed as r11399, with cleanup as r11400.

Using AIX 5.3 64bit platform. Using Valgrind 3.4.1 .
shmat(id , non zero value, 0) fails under Valgrind.

Interestingly also fails when linked with electricfence !

Closing.  I'm not sure why this didn't get closed before.

Really, really closing this time...