fcntl64 fix

TARGET_F_*64 should be used instead of F_*64, because on 64-bit host systems F_GETLK == F_GETLK64(same for SETLK and SETLKW), so we cannot determinate if it's a long lock or not on a target 32-bit system. Patch in the attachment.

P.S. Please, review my privious patches, which I have added description recently. Or should I repost it?

diff -uNr qemu-0.9.0.cvs20070304.orig/linux-user/syscall.c qemu-0.9.0.cvs20070304/linux-user/syscall.c
@@ -4063,7 +4063,7 @@
 #endif
 
         switch(arg2) {
-        case F_GETLK64:
+        case TARGET_F_GETLK64:
             ret = get_errno(fcntl(arg1, arg2, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM

@@ -4089,8 +4089,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);

My initial fix was before I started using LTP, and just took care of a single case that was holding me up. Now I have run the fcntl tests in LTP on ARM (both oABI and EABI) and there are a lot of failures indicating that there is a lot more work to be done yet on fcntl().

I'll take a look into it, and probably resubmit a bigger patch later.

Stuart

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

One more small fix to repack a structure from taget -> host before using it clears up most of the fcntl() errors that showed up in LTP. This is one of those that probably doesn't happen when runngin 32 on 32, but I'm running 32 on 64.

Stuart

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

@@ -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);

Yep. You're right. Fixed patch in the attachment.

diff -uNr qemu-0.9.0.cvs20070320.orig/linux-user/syscall.c qemu-0.9.0.cvs20070320/linux-user/syscall.c
@@ -4058,15 +4058,27 @@
 #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_SETLK64;
+        default:
+            cmd = arg2;
+        }
+
         switch(arg2) {
-        case F_GETLK64:
-            ret = get_errno(fcntl(arg1, arg2, &fl));
+        case TARGET_F_GETLK64:
+            ret = get_errno(fcntl(arg1, cmd, &fl));
 	    if (ret == 0) {
 #ifdef TARGET_ARM
                 if (((CPUARMState *)cpu_env)->eabi) {

@@ -4091,8 +4103,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);

@@ -4113,10 +4125,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;

I'm using arm(little-endian) on x86_64.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

That blows that theory 8-).

What are you using as a test app? I think that remapping the constants is needed, but I'm just curious how we seem to be coming up with different parts of the fix when we have the same target/host combination.

Stuart

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

:)

I got error when runing Debian's apt-get and tried to fix it.

I'm not sure that I understand you...

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

OK, that's what got me started on this one, but I switched to using the ltp-kernel-test package for a more comprehensive set of tests once I got past that first eabi structure change.

On the arm/x86_64 combination, I think the host & target cmd values are the same, so the remapping is a noop. It may be needed for other combinations though. Some architectures have very different values for constants like this in their ABI.

I was trying to understand how your fix made apt-get/dpkg happy, or if you were just using a different app that was hitting a different case for fcntl().

Stuart

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

No. Remap is needed:

$ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
x86_64
5

$ uname -m; echo -e '#include <fcntl.h>\nF_GETLK64' | cpp | tail -1
armv5l
12

Same for F_SETLK64 and F_SETLKW64.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

You are right. I had previously printed out the non-64 versions, and they are the same, but we not using those.

TARGET_F_GETLK 5
F_GETLK 5
TARGET_F_GETLK64 12
F_GETLK64 5

My confusion...

Stuart

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

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                               [email protected]
Network & Software Engineering                   http://www.netsweng.com/
1024D/37A79149:                                  0791 D3B8 9A4C 2CDC A31F
                                                 BD03 0A62 E534 37A7 9149

@@ -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;

Stuart Anderson wrote:
[snip]

It worked better, in that it fixed a problem that let me continue on to fix other issues. After revisiting fcntl() and coming up with the more comprehensive patch, this change now seems to have no effect, so I must conceed that it is not needed, please drop that part of the patch.

Now that the dust has settled, I see where the change is probably a no-op anyway. A quick little test program indicates that on x86_64, l_start will have an offset of 8 wether the structure is packed or not, and wether the __pad member is present or not. The unsigned long long is always going to be aligned to a 8 byte boundary.

Stuart

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

It architecture specific I think.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

Its' architecture specific I think.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

The __pad member is essential. Your logic is wrong is two ways:

a) The struct is packed. This overrides normal alignment and ensures the structure contains no padding.
b) long long has whatever alignment the host feels like giving it. There's no guarantee it's going to be 8 byte aligned.

Paul

And in this case, it does remove some tail padding at the end of the structure.

No there isn't. This was just an observation of what occurs when building a simple test case on x86_64.

Stuart

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

Primarily, I also thought that problem is in padding, because, without the patch F_GETLK, on 32-bit target recognises as F_GETLK64 on 64-bit host. It's happen because on 64-bit host and 32-bit target F_GETLK == F_GETLK64 == TARGET_F_GETLK. So if you're using qemu-arm on 64-bit host and target eabi program calls fcntl(fd,F_GETLK,...), target_eabi_flock64 will be used by mistake. Disabling padding can helps in some trivial cases to pass pseudo-correct args to fcntl.

Stuart, am I right?

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

Primarily, I also thought that problem is in padding, because, without the patch F_GETLK, on 32-bit target recognises as F_GETLK64 on 64-bit host. It's happen because on 64-bit host and 32-bit target F_GETLK == F_GETLK64 == TARGET_F_GETLK. So if you're using qemu-arm on 64-bit host and a target eabi program calls fcntl(fd,F_GETLK,...), target_eabi_flock64 will be used by mistake. Disabling padding can helps in some trivial cases to pass pseudo-correct args to fcntl. I guess this part of patch wrong.

Stuart, am I right?

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

Yes, this is a good summary of the trap I initially fell into.

Stuart

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

Patch that fixes dummy bug in the attachment. Sorry.

Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + Velesys LLC, http://www.velesys.com/
 + ALT Linux Team, http://www.altlinux.com/

index 5ff364d..a23f3f7 100644
@@ -3882,10 +3882,13 @@ long do_syscall(void *cpu_env, int num, long arg1, long arg2, long arg3,
         switch(arg2){
         case TARGET_F_GETLK64:
             cmd = F_GETLK64;
+            break;
         case TARGET_F_SETLK64:
             cmd = F_SETLK64;
+            break;
         case TARGET_F_SETLKW64:
             cmd = F_SETLK64;
+            break;
         default:
             cmd = arg2;
         }