| Message ID | 1455033431-24034-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le 09/02/2016 16:57, Peter Maydell a écrit : > Our implementation of shmat() and shmdt() for linux-user was > using "zero guest address" as its marker for "entry in the > shm_regions[] array is not in use". This meant that if the > guest did a shmdt(0) we would match on an unused array entry Is shmdt(0) valid ? I mean, if shmat() is called with shmaddr equal to 0: "the system chooses a suitable (unused) address at which to attach the segment." and "The to-be-detached segment must be currently attached with shmaddr equal to the value returned by the attaching shmat() call." Did you check shmat() can return 0 ? (I think our mmap_find_vma() cannot return 0) Why don't you fail on shmdt(0) (EINVAL) ? > and call page_set_flags() with both start and end addresses zero, > which causes an assertion failure. > > Use an explicit in_use flag to manage the shm_regions[] array, > so that we avoid this problem. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Pavel Shamis <pasharesearch@gmail.com> > --- > linux-user/syscall.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 54ce14a..f46abf7 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr) > #define N_SHM_REGIONS 32 > > static struct shm_region { > - abi_ulong start; > - abi_ulong size; > + abi_ulong start; > + abi_ulong size; > + bool in_use; > } shm_regions[N_SHM_REGIONS]; > > struct target_semid_ds > @@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg) > ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE)); > > for (i = 0; i < N_SHM_REGIONS; i++) { > - if (shm_regions[i].start == 0) { > + if (!shm_regions[i].in_use) { > + shm_regions[i].in_use = true; > shm_regions[i].start = raddr; > shm_regions[i].size = shm_info.shm_segsz; > break; > @@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr) > int i; > > for (i = 0; i < N_SHM_REGIONS; ++i) { > - if (shm_regions[i].start == shmaddr) { > - shm_regions[i].start = 0; > + if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) { > + shm_regions[i].in_use = false; > page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0); > break; > } >
On 10 February 2016 at 18:39, Laurent Vivier <laurent@vivier.eu> wrote: > > > Le 09/02/2016 16:57, Peter Maydell a écrit : >> Our implementation of shmat() and shmdt() for linux-user was >> using "zero guest address" as its marker for "entry in the >> shm_regions[] array is not in use". This meant that if the >> guest did a shmdt(0) we would match on an unused array entry > > Is shmdt(0) valid ? It's valid in the sense of "should detach" if shmat() ever returned 0 (which I suspect it will never do but have not attempted to determine). It's valid in the sense of "should not cause an assert" anyway. > I mean, if shmat() is called with shmaddr equal to 0: > "the system chooses a suitable (unused) address at which > to attach the segment." > > and > > "The to-be-detached segment must be currently attached with shmaddr > equal to the value returned by the attaching shmat() call." > > Did you check shmat() can return 0 ? > (I think our mmap_find_vma() cannot return 0) Not wanting to try to figure this out is why I switched to having an extra in_use flag in the shm_regions[] array. 0 is now not any kind of special value as far as addresses go -- if shmat() returned 0 as a valid address then we'll record it in the array, and shmdt() will work. If it never did, then shmdt() won't find any valid entries, we'll call the host with shmdt() on something that wasn't an attached segment and the host kernel will fail the syscall as it should. > Why don't you fail on shmdt(0) (EINVAL) ? We let the host kernel do the error checking and return the errno for us, at which point it will indeed fail EINVAL. thanks -- PMM
Le 09/02/2016 16:57, Peter Maydell a écrit : > Our implementation of shmat() and shmdt() for linux-user was > using "zero guest address" as its marker for "entry in the > shm_regions[] array is not in use". This meant that if the > guest did a shmdt(0) we would match on an unused array entry > and call page_set_flags() with both start and end addresses zero, > which causes an assertion failure. > > Use an explicit in_use flag to manage the shm_regions[] array, > so that we avoid this problem. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Pavel Shamis <pasharesearch@gmail.com> Reviewed-by: Laurent Vivier <laurent@vivier.eu> > --- > linux-user/syscall.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 54ce14a..f46abf7 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr) > #define N_SHM_REGIONS 32 > > static struct shm_region { > - abi_ulong start; > - abi_ulong size; > + abi_ulong start; > + abi_ulong size; > + bool in_use; > } shm_regions[N_SHM_REGIONS]; > > struct target_semid_ds > @@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg) > ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE)); > > for (i = 0; i < N_SHM_REGIONS; i++) { > - if (shm_regions[i].start == 0) { > + if (!shm_regions[i].in_use) { > + shm_regions[i].in_use = true; > shm_regions[i].start = raddr; > shm_regions[i].size = shm_info.shm_segsz; > break; > @@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr) > int i; > > for (i = 0; i < N_SHM_REGIONS; ++i) { > - if (shm_regions[i].start == shmaddr) { > - shm_regions[i].start = 0; > + if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) { > + shm_regions[i].in_use = false; > page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0); > break; > } >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 54ce14a..f46abf7 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr) #define N_SHM_REGIONS 32 static struct shm_region { - abi_ulong start; - abi_ulong size; + abi_ulong start; + abi_ulong size; + bool in_use; } shm_regions[N_SHM_REGIONS]; struct target_semid_ds @@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg) ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE)); for (i = 0; i < N_SHM_REGIONS; i++) { - if (shm_regions[i].start == 0) { + if (!shm_regions[i].in_use) { + shm_regions[i].in_use = true; shm_regions[i].start = raddr; shm_regions[i].size = shm_info.shm_segsz; break; @@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr) int i; for (i = 0; i < N_SHM_REGIONS; ++i) { - if (shm_regions[i].start == shmaddr) { - shm_regions[i].start = 0; + if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) { + shm_regions[i].in_use = false; page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0); break; }
Our implementation of shmat() and shmdt() for linux-user was using "zero guest address" as its marker for "entry in the shm_regions[] array is not in use". This meant that if the guest did a shmdt(0) we would match on an unused array entry and call page_set_flags() with both start and end addresses zero, which causes an assertion failure. Use an explicit in_use flag to manage the shm_regions[] array, so that we avoid this problem. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reported-by: Pavel Shamis <pasharesearch@gmail.com> --- linux-user/syscall.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)