IA-32 support patch: fcntl64 calls are broken

PATCH Applied Arun Sharma 5 participants 10 messages Jun 24, 2003 – Jul 15, 2003

Share & tools

Arun Sharma

author

2003-06-24 22:51 UTC #0 lore raw

Currently, sys32_fcntl64() is broken, because it passes F_*64 commands to sys_fcntl(), which it doesn't understand. The F_XXX64 commands need to be converted to F_XXX before calling sys_fcntl().

We also eliminated a copy, because the 32 bit struct flock64 and the 64 bit struct flock seem to be identical.

This patch is relevant only to 2.4, as it has already been taken care of in 2.5.

-Arun

arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +3 -8

@@ -3618,14 +3595,9 @@
 	      case F_GETLK64:
 	      case F_SETLK64:
 	      case F_SETLKW64:
-		if (ia32_get_flock(&f, arg))
-			return -EFAULT;
-		old_fs = get_fs();
-		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
-		set_fs(old_fs);
-		if (cmd == F_GETLK && ia32_put_flock(&f, arg))
-			return -EFAULT;
+		ret = sys_fcntl(fd, (cmd == F_GETLK64) ? F_GETLK :
+			((cmd == F_SETLK64) ? F_SETLK : F_SETLKW),
+			(unsigned long)arg);
 		break;
 
 	      default:

Bjorn Helgaas

2003-06-25 16:53 UTC #1 lore raw

Quoted 5 lines · Arun Sharma

Currently, sys32_fcntl64() is broken, because it passes F_*64 commands to sys_fcntl(), which it doesn't understand. The F_XXX64 commands need to be converted to F_XXX before calling sys_fcntl().
We also eliminated a copy, because the 32 bit struct flock64 and the 64 bit struct flock seem to be identical.
This patch is relevant only to 2.4, as it has already been taken care of in 2.5.

2.5 has this code, which looks suspicious to me (do we really mean to call sys_fcntl() twice?):

        case F_GETLK64:
        case F_SETLK64:
        case F_SETLKW64:
                ret = get_compat_flock64(&f, compat_ptr(arg));
                if (ret != 0)
                        break;
                old_fs = get_fs();
                set_fs(KERNEL_DS);
                ret = sys_fcntl(fd, F_GETLK, (unsigned long)&f);
                ret = sys_fcntl(fd, (cmd = F_GETLK64) ? F_GETLK :
                                ((cmd = F_SETLK64) ? F_SETLK : F_SETLKW),
                                (unsigned long)&f);

Arun Sharma

author

2003-06-25 18:18 UTC #2 lore raw

In reply to Arun Sharma

Quoted 2 lines · Bjorn Helgaas

2.5 has this code, which looks suspicious to me (do we really mean to call sys_fcntl() twice?):

Cc'ing the author of this patch in 2.5. Stephen, did you mean to call sys_fcntl() twice ?

-Arun

Quoted 14 lines

        case F_GETLK64:
        case F_SETLK64:
        case F_SETLKW64:
                ret = get_compat_flock64(&f, compat_ptr(arg));
                if (ret != 0)
                        break;
                old_fs = get_fs();
                set_fs(KERNEL_DS);
                ret = sys_fcntl(fd, F_GETLK, (unsigned long)&f);
                ret = sys_fcntl(fd, (cmd = F_GETLK64) ? F_GETLK :
                                ((cmd = F_SETLK64) ? F_SETLK : F_SETLKW),
                                (unsigned long)&f);

Bjorn Helgaas

2003-06-25 21:06 UTC #3 lore raw

In reply to Arun Sharma

Quoted 1 line · Arun Sharma

Currently, sys32_fcntl64() is broken, because it passes F_*64 commands to sys_fcntl(), which it doesn't understand. The F_XXX64 commands need to be converted to F_XXX before calling sys_fcntl().

As far as I can tell, this is almost equivalent to, but uglier than, the mips64 version:

	sys32_fcntl64(unsigned int fd, unsigned int cmd, unsigned long arg)
	{
	        switch (cmd) {
	        case F_GETLK64:
	                return sys_fcntl(fd, F_GETLK, arg);
	        case F_SETLK64:
	                return sys_fcntl(fd, F_SETLK, arg);
	        case F_SETLKW64:
	                return sys_fcntl(fd, F_SETLKW, arg);
	        }

	        return sys32_fcntl(fd, cmd, arg);
	}

parisc, ppc64, s390, and sparc64 have this, which I think is also equivalent, but uglier:

	asmlinkage long sys32_fcntl64(unsigned int fd, unsigned int cmd, unsigned long arg)
	{
	        if (cmd >= F_GETLK64 && cmd <= F_SETLKW64)
	                return sys_fcntl(fd, cmd + F_GETLK - F_GETLK64, arg);
	        return sys32_fcntl(fd, cmd, arg);
	}

I'd like to see the ia64 version be similar to the mips64 version. The remaining difference is the int/long argument; maybe you can resolve that too...

Bjorn

Stephen Rothwell

2003-06-26 01:27 UTC #4 lore raw

In reply to Arun Sharma

Quoted 7 lines · On Wed, 25 Jun 2003 11:18:17 -0700 Arun Sharma <[email protected]> wrote:

Quoted 2 lines · Bjorn Helgaas
2.5 has this code, which looks suspicious to me (do we really mean to call sys_fcntl() twice?):
Cc'ing the author of this patch in 2.5. Stephen, did you mean to call sys_fcntl() twice ?

No. Rats! :-( Cut and paste error (I assume). I will submit a patch to Linus.

Signature · 3 lines

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

Bjorn Helgaas

2003-06-26 05:02 UTC #5 lore raw

In reply to Arun Sharma

Quoted 5 lines · On Wednesday 25 June 2003 3:06 pm, I wrote:

Quoted 1 line · Arun Sharma
Currently, sys32_fcntl64() is broken, because it passes F_*64 commands to sys_fcntl(), which it doesn't understand. The F_XXX64 commands need to be converted to F_XXX before calling sys_fcntl().
As far as I can tell, this is almost equivalent to, but uglier than, the mips64 version:

Never mind. Besides being unnecessarily rude, I had obviouisly forgotten the fragment from 2.5 that I just quoted. Your patch looks very much like the 2.5 code, which I think is the best approach. Even though I think the switch is more readable than the nested ternary expressions, it's better to follow 2.5. I apologize.

Bjorn

Arun Sharma

2003-07-08 18:57 UTC #6 lore raw

In reply to Arun Sharma

Quoted 5 lines · Arun Sharma

Currently, sys32_fcntl64() is broken, because it passes F_*64 commands to sys_fcntl(), which it doesn't understand. The F_XXX64 commands need to be converted to F_XXX before calling sys_fcntl().
We also eliminated a copy, because the 32 bit struct flock64 and the 64 bit struct flock seem to be identical.

Turns out that the structures are not identical (even though the data types of all the members have the same size) due to alignment issues. So the copy can not be eliminated.

Quoted 1 line

This patch is relevant only to 2.4, as it has already been taken care of in 2.5.

A new patch is attached.

-Arun

arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +4 -2

@@ -3622,9 +3622,11 @@
 			return -EFAULT;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
+                ret = sys_fcntl(fd, (cmd == F_GETLK64) ? F_GETLK :
+                        ((cmd == F_SETLK64) ? F_SETLK : F_SETLKW),
+                        (unsigned long) &f);
 		set_fs(old_fs);
-		if (cmd == F_GETLK && ia32_put_flock(&f, arg))
+		if (cmd == F_GETLK64 && ia32_put_flock(&f, arg))
 			return -EFAULT;
 		break;

Matthew Wilcox

2003-07-08 19:16 UTC #7 lore raw

In reply to Arun Sharma

arch/ia64/ia32/sys_ia32.c line 3625

Quoted 8 lines · Arun Sharma

arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +1 -1

@@ -3622,9 +3622,11 @@
 			return -EFAULT;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
+                ret = sys_fcntl(fd, (cmd = F_GETLK64) ? F_GETLK :

looks like you're using spaces instead of tabs. tsk.

Signature · 3 lines

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Arun Sharma

author

2003-07-08 21:51 UTC #8 lore raw

In reply to Arun Sharma

Quoted 14 lines · Matthew Wilcox

arch/ia64/ia32/sys_ia32.c line 3625
Quoted 8 lines · Arun Sharma
arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +1 -1
@@ -3622,9 +3622,11 @@
 			return -EFAULT;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
+                ret = sys_fcntl(fd, (cmd == F_GETLK64) ? F_GETLK :
looks like you're using spaces instead of tabs. tsk.

I admit to cutting and pasting from 2.5 :) An updated patch with tabs is attached.

-Arun

arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +4 -2

@@ -3622,9 +3622,11 @@
 			return -EFAULT;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
+		ret = sys_fcntl(fd, (cmd == F_GETLK64) ? F_GETLK :
+			((cmd == F_SETLK64) ? F_SETLK : F_SETLKW),
+			(unsigned long) &f);
 		set_fs(old_fs);
-		if (cmd == F_GETLK && ia32_put_flock(&f, arg))
+		if (cmd == F_GETLK64 && ia32_put_flock(&f, arg))
 			return -EFAULT;
 		break;

Bjorn Helgaas

Applied

2003-07-15 19:26 UTC #9 lore raw

In reply to Arun Sharma

Quoted 16 lines · Arun Sharma

Quoted 14 lines · Matthew Wilcox
arch/ia64/ia32/sys_ia32.c line 3625
Quoted 8 lines · Arun Sharma
arch/ia64/ia32/sys_ia32.c renamed from arch/ia64/ia32/sys_ia32.c- +1 -1
@@ -3622,9 +3622,11 @@
 			return -EFAULT;
 		old_fs = get_fs();
 		set_fs(KERNEL_DS);
-		ret = sys_fcntl(fd, cmd, (unsigned long) &f);
+                ret = sys_fcntl(fd, (cmd = F_GETLK64) ? F_GETLK :
looks like you're using spaces instead of tabs. tsk.

Quoted 1 line

I admit to cutting and pasting from 2.5 :) An updated patch with tabs is attached.

Applied this to 2.4. Whew :-)