[Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

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

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 linux-user/syscall.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ec9e4fc..b2da432 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -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);
-- 
1.9.3

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Peter Maydell]

On 25 January 2015 at 12:06, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
> 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".
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

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

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

On 1/25/15 20:49, Peter Maydell wrote:
> On 25 January 2015 at 12:06, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>> 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".
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
 
Thank you for your work about the several patches.


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

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Peter Maydell]

On 25 January 2015 at 21:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
> On 1/25/15 20:49, Peter Maydell wrote:
>> 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:

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

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

On 1/26/15 06:10, Peter Maydell wrote:
> On 25 January 2015 at 21:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>> On 1/25/15 20:49, Peter Maydell wrote:
>>> 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:
> 
> That's fine. I'm definitely not asking you to do this work.

OK, thanks. :-)

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

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Peter Maydell]

On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
> On 1/26/15 06:10, Peter Maydell wrote:
>> 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.

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

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

On 1/26/15 23:01, Peter Maydell wrote:
> On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>> On 1/26/15 06:10, Peter Maydell wrote:
>>> 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.
> 
> 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. 
 
> 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.

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Riku Voipio]

Hi,

First of all, thanks Chen for taking time to improve the linux-user
codebase in qemu!

On Mon, Jan 26, 2015 at 03:01:52PM +0000, Peter Maydell wrote:
> On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
> > On 1/26/15 06:10, Peter Maydell wrote:
> >> 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.

> 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

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

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.

On 1/28/15 22:27, Riku Voipio wrote:
> Hi,
> 
> First of all, thanks Chen for taking time to improve the linux-user
> codebase in qemu!
> 
> On Mon, Jan 26, 2015 at 03:01:52PM +0000, Peter Maydell wrote:
>> On 26 January 2015 at 14:59, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>>> On 1/26/15 06:10, Peter Maydell wrote:
>>>> 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.
> 
>> 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
> 
> 

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Peter Maydell]

On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>  - 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

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

On 1/29/15 06:36, Peter Maydell wrote:
> On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>>  - 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.
> 

OK, thanks.

For coding and test, is linux-user a good starting position for me? (I
guess it is).


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: Let all lock_user_struct() and unlock_user_struct() paired with each other

[Chen Gang S]

On 1/29/15 09:37, Chen Gang S wrote:
> On 1/29/15 06:36, Peter Maydell wrote:
>> On 28 January 2015 at 22:09, Chen Gang S <gang.chen@sunrus.com.cn> wrote:
>>>  - 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.
>>
> 
> 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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed