linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other
PATCH 1/1 linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other
lock_user_struct() and unlock_user_struct() need always be paired with each other, or will cause resource leak.
Also remove redundant check for 'target_mb' in abi_long do_msgrcv().
Also match the coding styles found by "./scripts/checkpatch.pl".
--- linux-user/syscall.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)
linux-user/syscall.c +19 -8
@@ -2518,8 +2518,10 @@ static inline abi_long target_to_host_semid_ds(struct semid_ds *host_sd,if (!lock_user_struct (VERIFY_READ , target_sd, target_addr,1 ))return -TARGET_EFAULT ; -if (target_to_host_ipc_perm (&(host_sd->sem_perm),target_addr)) +if (target_to_host_ipc_perm (&(host_sd->sem_perm), target_addr)) { +unlock_user_struct (target_sd, target_addr,0 );return -TARGET_EFAULT ; + } host_sd->sem_nsems =tswapal (target_sd->sem_nsems); host_sd->sem_otime =tswapal (target_sd->sem_otime); host_sd->sem_ctime =tswapal (target_sd->sem_ctime);
@@ -2534,8 +2536,10 @@ static inline abi_long host_to_target_semid_ds(abi_ulong target_addr,if (!lock_user_struct (VERIFY_WRITE , target_sd, target_addr,0 ))return -TARGET_EFAULT ; -if (host_to_target_ipc_perm (target_addr,&(host_sd->sem_perm))) +if (host_to_target_ipc_perm (target_addr, &(host_sd->sem_perm))) { +unlock_user_struct (target_sd, target_addr,0 );return -TARGET_EFAULT ; + } target_sd->sem_nsems =tswapal (host_sd->sem_nsems); target_sd->sem_otime =tswapal (host_sd->sem_otime); target_sd->sem_ctime =tswapal (host_sd->sem_ctime);
@@ -2796,8 +2800,10 @@ static inline abi_long target_to_host_msqid_ds(struct msqid_ds *host_md,if (!lock_user_struct (VERIFY_READ , target_md, target_addr,1 ))return -TARGET_EFAULT ; -if (target_to_host_ipc_perm (&(host_md->msg_perm),target_addr)) +if (target_to_host_ipc_perm (&(host_md->msg_perm), target_addr)) { +unlock_user_struct (target_md, target_addr,0 );return -TARGET_EFAULT ; + } host_md->msg_stime =tswapal (target_md->msg_stime); host_md->msg_rtime =tswapal (target_md->msg_rtime); host_md->msg_ctime =tswapal (target_md->msg_ctime);
@@ -2817,8 +2823,10 @@ static inline abi_long host_to_target_msqid_ds(abi_ulong target_addr,if (!lock_user_struct (VERIFY_WRITE , target_md, target_addr,0 ))return -TARGET_EFAULT ; -if (host_to_target_ipc_perm (target_addr,&(host_md->msg_perm))) +if (host_to_target_ipc_perm (target_addr, &(host_md->msg_perm))) { +unlock_user_struct (target_md, target_addr,0 );return -TARGET_EFAULT ; + } target_md->msg_stime =tswapal (host_md->msg_stime); target_md->msg_rtime =tswapal (host_md->msg_rtime); target_md->msg_ctime =tswapal (host_md->msg_ctime);
@@ -2953,8 +2961,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp, target_mb->mtype =tswapal (host_mb->mtype); end: -if (target_mb) -unlock_user_struct (target_mb, msgp,1 ); +unlock_user_struct (target_mb, msgp,1 );g_free (host_mb);return ret; }
@@ -2966,8 +2973,10 @@ static inline abi_long target_to_host_shmid_ds(struct shmid_ds *host_sd,if (!lock_user_struct (VERIFY_READ , target_sd, target_addr,1 ))return -TARGET_EFAULT ; -if (target_to_host_ipc_perm (&(host_sd->shm_perm), target_addr)) +if (target_to_host_ipc_perm (&(host_sd->shm_perm), target_addr)) { +unlock_user_struct (target_sd, target_addr,0 );return -TARGET_EFAULT ; + }__get_user (host_sd->shm_segsz, &target_sd->shm_segsz);__get_user (host_sd->shm_atime, &target_sd->shm_atime);__get_user (host_sd->shm_dtime, &target_sd->shm_dtime);
@@ -2986,8 +2995,10 @@ static inline abi_long host_to_target_shmid_ds(abi_ulong target_addr,if (!lock_user_struct (VERIFY_WRITE , target_sd, target_addr,0 ))return -TARGET_EFAULT ; -if (host_to_target_ipc_perm (target_addr, &(host_sd->shm_perm))) +if (host_to_target_ipc_perm (target_addr, &(host_sd->shm_perm))) { +unlock_user_struct (target_sd, target_addr,0 );return -TARGET_EFAULT ; + }__put_user (host_sd->shm_segsz, &target_sd->shm_segsz);__put_user (host_sd->shm_atime, &target_sd->shm_atime);__put_user (host_sd->shm_dtime, &target_sd->shm_dtime);
Signature · 1 line
1.9.3
Quoted 8 lines · Chen Gang S
lock_user_struct()andunlock_user_struct()need always be paired with each other, or will cause resource leak.Also remove redundant check for '
target_mb' inabi_longdo_msgrcv().Also match the coding styles found by "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>
Are you claiming that you've reviewed *all* the code in this file for mismatched lock/unlock calls? If so, it would be nice to say so explicitly in the commit message. If not, it would be nice if the commit message was clearer about what areas of the code it applied to. The code changes are correct, though.
thanks
-- PMM
Quoted 4 lines · Peter Maydell
Quoted 3 lines · Chen Gang S
lock_user_struct()andunlock_user_struct()need always be paired with each other, or will cause resource leak.
Quoted 8 lines
Quoted 5 lines
Also remove redundant check for '
target_mb' inabi_longdo_msgrcv().Also match the coding styles found by "
./scripts/checkpatch.pl".Signed-off-by Chen Gang <[email protected]>Reviewed-by Peter Maydell <[email protected]>
Thank you for your work about the several patches.
Quoted 6 lines
Are you claiming that you've reviewed *all* the code in this file for mismatched lock/unlock calls? If so, it would be nice to say so explicitly in the commit message. If not, it would be nice if the commit message was clearer about what areas of the code it applied to. The code changes are correct, though.
At present, I finished all lock_user_struct() and unlock_user_struct() in "linux-user/syscall.c". For me, after this patch, they are all OK.
But for all lock/unlock in "linux-user/syscall.c", for me, I am doubting several areas, but I did not send patch for them:
- I need check them carefully again to be sure they are really issue:
Read the related code again and again, if I really treat it as an issue, I shall make related patch (and pass compiling, at least).
- I have no enough time resources on it:
In week end, I have to spend some time resource on my son (check his homework, play with him). I also need spend time resources on Linux kernel and gcc/binutils.
In week day, I may work overtime sometimes, also I need spend 4 hours on subway between my home and office per day (come and go).
- So it is really necessary for me to make a schedule (the tasks/steps, the time point, and the risks), and also need consider about:
How to use the time resources efficiently: e.g. fully use the time resources on the subway between home and office, in work day; fully use the time resources on holidays (e.g. Chinese Spring Festival).
How to fully use the human resources: e.g. try my best to always keep well communication with all related open source members (which will give much valuable help for the related tasks).
Welcome any members' ideas, suggestions, and completions, also welcome any members to help check "linux-user/syscall.c" (it will save my time
resource: now for qemu, my highest priority task is for tile qemu).
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 14 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
Are you claiming that you've reviewed *all* the code in this file for mismatched lock/unlock calls? If so, it would be nice to say so explicitly in the commit message. If not, it would be nice if the commit message was clearer about what areas of the code it applied to. The code changes are correct, though.
At present, I finished all
lock_user_struct()andunlock_user_struct()in "linux-user/syscall.c". For me, after this patch, they are all OK.But for all lock/unlock in "
linux-user/syscall.c", for me, I am doubting several areas, but I did not send patch for them:
Quoted 6 lines
- I need check them carefully again to be sure they are really issue:
Read the related code again and again, if I really treat it as an issue, I shall make related patch (and pass compiling, at least).
- I have no enough time resources on it:
That's fine. I'm definitely not asking you to do this work. I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
thanks
-- PMM
Quoted 15 lines · Peter Maydell
Quoted 14 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
Are you claiming that you've reviewed *all* the code in this file for mismatched lock/unlock calls? If so, it would be nice to say so explicitly in the commit message. If not, it would be nice if the commit message was clearer about what areas of the code it applied to. The code changes are correct, though.
At present, I finished all
lock_user_struct()andunlock_user_struct()in "linux-user/syscall.c". For me, after this patch, they are all OK.But for all lock/unlock in "
linux-user/syscall.c", for me, I am doubting several areas, but I did not send patch for them:
Quoted 8 lines
Quoted 6 lines
- I need check them carefully again to be sure they are really issue:
Read the related code again and again, if I really treat it as an issue, I shall make related patch (and pass compiling, at least).
- I have no enough time resources on it:
That's fine. I'm definitely not asking you to do this work.
OK, thanks. :-)
Quoted 6 lines
I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
OK, thanks.
I am not quite familiar with this file, so I describe the modification by function name, e.g. lock_user_struct() and unlick_user_struct() in the patch subject.
Welcome to help improve the patch comments. If necessary to send patch v2, please let me know, I shall try.
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 8 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
Quoted 5 lines
OK, thanks.
I am not quite familiar with this file, so I describe the modification by function name, e.g.
lock_user_struct()andunlick_user_struct()in the patch subject.
In a big file I think it's often more useful to describe the functions which are being changed. My suggested subject would be:
"Fix mismatched lock/unlock calls in IPC struct conversion functions"
Riku can decide if he wants a v2 or will just fix it up as he applies it to his linux-user tree.
-- PMM
Quoted 11 lines · Peter Maydell
Quoted 10 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
OK, thanks.
Quoted 9 lines
Quoted 3 lines
I am not quite familiar with this file, so I describe the modification by function name, e.g.
lock_user_struct()andunlick_user_struct()in the patch subject.In a big file I think it's often more useful to describe the functions which are being changed. My suggested subject would be:
"Fix mismatched lock/unlock calls in IPC struct conversion functions"
What you said above sounds reasonable to me.
Quoted 3 lines
Riku can decide if he wants a v2 or will just fix it up as he applies it to his linux-user tree.
OK, thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Hi,
First of all, thanks Chen for taking time to improve the linux-user codebase in qemu!
Quoted 15 lines · Peter Maydell
Quoted 8 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
Quoted 5 lines
OK, thanks.
I am not quite familiar with this file, so I describe the modification by function name, e.g.
lock_user_struct()andunlick_user_struct()in the patch subject.
Quoted 7 lines
In a big file I think it's often more useful to describe the functions which are being changed. My suggested subject would be:
"Fix mismatched lock/unlock calls in IPC struct conversion functions"
Riku can decide if he wants a v2 or will just fix it up as he applies it to his linux-user tree.
No need for v2, I've change the title in my tree.
Riku
Thank you for your work, too.
Next month, I shall start tile qemu, I guess, for coding, I shall start from linux-user (which is mainly for cpu emulation).
- For each patch, I should make at least a valuable change, and pass related test.
- Each month, I should make 3 patches at least.
- Hope I can finish tile for linux-user within 2015-06-30: finish all tile cpu common instructions emulation, and pass all related test.
Welcome any ideas, suggestions, and completions, e.g.
- Is what I said above really correct (e.g. is linux-user really mainly for cpu emulation)?.
- For each patch, are there additional information, requirements or suggestions which I should notice about?
- If I can finish 3 patches per month, is it possible to finish tile cpu common instructions emulation for linux-user within 2015-06-30?
Thanks.
Quoted 34 lines · Riku Voipio
Hi,
First of all, thanks Chen for taking time to improve the linux-user codebase in qemu!
Quoted 15 lines · Peter Maydell
Quoted 8 lines · Chen Gang S
Quoted 6 lines · Peter Maydell
I would just like the commit message to be clear about the scope of the work the patch covers. If the patch is just "Fix mismatched lock/unlock calls in IPC struct conversion functions" then that's fine, but the commit message should say that. At the moment the commit message is very vague.
Quoted 5 lines
OK, thanks.
I am not quite familiar with this file, so I describe the modification by function name, e.g.
lock_user_struct()andunlick_user_struct()in the patch subject.Quoted 7 lines
In a big file I think it's often more useful to describe the functions which are being changed. My suggested subject would be:
"Fix mismatched lock/unlock calls in IPC struct conversion functions"
Riku can decide if he wants a v2 or will just fix it up as he applies it to his linux-user tree.
No need for v2, I've change the title in my tree.
Riku
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 2 lines · Chen Gang S
- Is what I said above really correct (e.g. is linux-user really mainly for cpu emulation)?.
Not really. linux-user is mainly for running single Linux binaries. It has a secondary use for running gcc test binaries which think they are "bare metal" but actually use some kind of semihosting API. (You should check whether tile has one of those.)
As well as linux-user mode, QEMU has system emulation mode, where we emulate a complete machine.
Both modes need CPU emulation.
-- PMM
Quoted 9 lines · Peter Maydell
Quoted 2 lines · Chen Gang S
- Is what I said above really correct (e.g. is linux-user really mainly for cpu emulation)?.
Not really. linux-user is mainly for running single Linux binaries. It has a secondary use for running gcc test binaries which think they are "bare metal" but actually use some kind of semihosting API. (You should check whether tile has one of those.)
Quoted 5 lines
As well as linux-user mode, QEMU has system emulation mode, where we emulate a complete machine.
Both modes need CPU emulation.
OK, thanks.
For coding and test, is linux-user a good starting position for me? (I guess it is).
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed
Quoted 13 lines · Chen Gang S
Quoted 12 lines · Peter Maydell
Quoted 2 lines · Chen Gang S
- Is what I said above really correct (e.g. is linux-user really mainly for cpu emulation)?.
Not really. linux-user is mainly for running single Linux binaries. It has a secondary use for running gcc test binaries which think they are "bare metal" but actually use some kind of semihosting API. (You should check whether tile has one of those.)
As well as linux-user mode, QEMU has system emulation mode, where we emulate a complete machine.
Quoted 8 lines
Quoted 2 lines
Both modes need CPU emulation.
OK, thanks.
For coding and test, is linux-user a good starting position for me? (I guess it is).
At present, I make a static program which will print "Hello world" for microblaze architecture, and can excute successfully.
[root@localhost qemu]# cat test.c
#include <stdio.h>
int main()
{
printf("Hello world!\n");
return 0;
}
[root@localhost qemu]# /upstream/release/bin/microblaze-gchen-linux-gcc -Wall -O2 -static -o test.mb test.c
[root@localhost qemu]# file ./test.mb
./test.mb: ELF 32-bit MSB executable, Xilinx MicroBlaze 32-bit RISC, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, not stripped
[root@localhost qemu]# ls -l ./test.mb
-rwxr-xr-x. 1 root root 3224540 Feb 5 06:39 ./test.mb
[root@localhost qemu]# ./microblaze-linux-user/qemu-microblaze ./test.mb
Hello world!And I also generate tile "Hello world" static program:
[root@localhost qemu]# /upstream/release-tile/bin/tilegx-gchen-linux-gcc -Wall -O2 -static -o test.tgx test.c [root@localhost qemu]# file ./test.tgx
./test.tgx: ELF 64-bit LSB executable, Tilera TILE-Gx, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, not stripped [root@localhost qemu]# ls -l ./test.tgx
-rwxr-xr-x. 1 root root 3611376 Feb 5 06:42 ./test.tgx
I shall try to let tile "Hello world" static program run successfully within this month:
- 1st patch: can run an empty elf64 tile executable in linux-user. (try to finish within 2015-02-15).
- 2nd patch: can run "Hello world" elf64 tile program in linux-user. (try to finish within 2015-02-25, it seems hard to finish in time).
- 3rd patch: for completion, or fixing issues, or documentations. (try to finish within 2015-02-28).
Welcome any ideas, suggestions, and completions.
Thanks.
Signature · 3 lines
Chen Gang Open, share, and attitude like air, water, and life which God blessed