Re: [Qemu-devel] [PATCH] fcntl64 fix

[Stuart Anderson <anderson@netsweng.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2297 bytes --]


OK, I think I finally have it all sorted out. Sorry if I sounded dense
along the way.. there were multiple variable, which increases the number
of possible combinations quickly.

The patch from Kirill is needed, and makes things better. One thing I
notice with it is that we now handle TARGET_F_GETLK64 in two places,
first in the case for TARGET_NR_fcntl64 (around line 4300), and then
again in do_fcntl(), which is called in the default case of the first
location. Once difference between the two locations is wether or not
the case for EABI is handled.

In addition to Kirill's patch, my original patch for target_eabi_flock64
is still needed as well as an expanded version of the revised patch I
sent later that does target->host strcture mapping for the F_GETLK*
cases.

I have used the fcntl test sets out of the Linux Test Projects to
measure with an without the different parts of these patches. With
the entire set (Which is attached), 16 of the 18 test sets pass
completely, and a significant portion of test14 (one of the two that
don't pass completely) passes as well. The tests in test14 that fail
may be do to a problem with a syscall other than fcntl(), but I haven't
completely resulved it yet. Without my portion of the patch, the results
are much worse (maybe half-ish are passing).

There is something interesting about test18 (the other one that doesn't
pass). It intentionally passes in a bad value (-1) as the 3rd argument
to fcntl(). It is testing wether it will get EFAULT. With these fixes,
qemu will SEGV as it tries to convert the struct flock (or struct
flock64) from target->host, and encounters the bad address that was
passed in. The initial SEGV is caught, but the handler for it then
SEGVs again. Ideally, we could detect that we are inside an emulated
system call, and be able to just return the EFAULT.

I ran the LTP tests for both old ABI and EABI, and got the same results.


Attached is the combined patch for fcntl().




                                 Stuart

Stuart R. Anderson                               anderson@netsweng.com
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                  BD03 0A62 E534 37A7 9149

[-- Attachment #2: fcntl() combined patch --]
[-- Type: TEXT/PLAIN, Size: 4205 bytes --]

--- linux-user/syscall_defs.h.orig	2007-02-23 15:44:47.000000000 -0500
+++ linux-user/syscall_defs.h	2007-02-23 15:44:26.000000000 -0500
@@ -1414,7 +1414,9 @@
 struct target_eabi_flock64 {
 	short  l_type;
 	short  l_whence;
+#if HOST_LONG_BITS == 32
         int __pad;
+#endif
 	unsigned long long l_start;
 	unsigned long long l_len;
 	int  l_pid;
Index: linux-user/syscall.c
===================================================================
--- linux-user/syscall.c.orig	2007-03-20 16:19:11.000000000 -0400
+++ linux-user/syscall.c	2007-03-20 17:04:40.000000000 -0400
@@ -2107,6 +2107,13 @@
 
     switch(cmd) {
     case TARGET_F_GETLK:
+        lock_user_struct(target_fl, arg, 1);
+        fl.l_type = tswap16(target_fl->l_type);
+        fl.l_whence = tswap16(target_fl->l_whence);
+        fl.l_start = tswapl(target_fl->l_start);
+        fl.l_len = tswapl(target_fl->l_len);
+        fl.l_pid = tswapl(target_fl->l_pid);
+        unlock_user_struct(target_fl, arg, 0);
         ret = fcntl(fd, cmd, &fl);
         if (ret == 0) {
             lock_user_struct(target_fl, arg, 0);
@@ -2132,6 +2139,13 @@
         break;
         
     case TARGET_F_GETLK64:
+        lock_user_struct(target_fl64, arg, 1);
+        fl64.l_type = tswap16(target_fl64->l_type) >> 1;
+        fl64.l_whence = tswap16(target_fl64->l_whence);
+        fl64.l_start = tswapl(target_fl64->l_start);
+        fl64.l_len = tswapl(target_fl64->l_len);
+        fl64.l_pid = tswap16(target_fl64->l_pid);
+        unlock_user_struct(target_fl64, arg, 0);
         ret = fcntl(fd, cmd >> 1, &fl64);
         if (ret == 0) {
             lock_user_struct(target_fl64, arg, 0);
@@ -4201,15 +4215,47 @@
 #if TARGET_LONG_BITS == 32
     case TARGET_NR_fcntl64:
     {
+	int cmd;
 	struct flock64 fl;
 	struct target_flock64 *target_fl;
 #ifdef TARGET_ARM
 	struct target_eabi_flock64 *target_efl;
 #endif
 
+       switch(arg2){
+       case TARGET_F_GETLK64:
+           cmd = F_GETLK64;
+       case TARGET_F_SETLK64:
+           cmd = F_SETLK64;
+       case TARGET_F_SETLKW64:
+           cmd = F_SETLKW64;
+       default:
+           cmd = arg2;
+       }
+
         switch(arg2) {
-        case F_GETLK64:
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+        case TARGET_F_GETLK64:
+#ifdef TARGET_ARM
+            if (((CPUARMState *)cpu_env)->eabi) {
+                lock_user_struct(target_efl, arg3, 1);
+                fl.l_type = tswap16(target_efl->l_type);
+                fl.l_whence = tswap16(target_efl->l_whence);
+                fl.l_start = tswap64(target_efl->l_start);
+                fl.l_len = tswap64(target_efl->l_len);
+                fl.l_pid = tswapl(target_efl->l_pid);
+                unlock_user_struct(target_efl, arg3, 0);
+            } else
+#endif
+            {
+                lock_user_struct(target_fl, arg3, 1);
+                fl.l_type = tswap16(target_fl->l_type);
+                fl.l_whence = tswap16(target_fl->l_whence);
+                fl.l_start = tswap64(target_fl->l_start);
+                fl.l_len = tswap64(target_fl->l_len);
+                fl.l_pid = tswapl(target_fl->l_pid);
+                unlock_user_struct(target_fl, arg3, 0);
+            }
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM
                 if (((CPUARMState *)cpu_env)->eabi) {
@@ -4234,8 +4280,8 @@
 	    }
 	    break;
 
-        case F_SETLK64:
-        case F_SETLKW64:
+        case TARGET_F_SETLK64:
+        case TARGET_F_SETLKW64:
 #ifdef TARGET_ARM
             if (((CPUARMState *)cpu_env)->eabi) {
                 lock_user_struct(target_efl, arg3, 1);
@@ -4256,10 +4302,10 @@
                 fl.l_pid = tswapl(target_fl->l_pid);
                 unlock_user_struct(target_fl, arg3, 0);
             }
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    break;
         default:
-            ret = get_errno(do_fcntl(arg1, arg2, arg3));
+            ret = get_errno(do_fcntl(arg1, cmd, arg3));
             break;
         }
 	break;