Skip to contentSkip to conversation

move O_LARGEFILE forcing to filp_open()

PATCH William Lee Irwin III 3 participants 13 messages Jul 4, 2004

Subsystems (6)
Memory management VFS MIPS s390 +2 more subsystems
lore
Share & tools
lore
#0 lore raw

Internal kernel open() of files barfs in important contexts, for instance, using strict non-overcommit with enough swap for large commitments. This is carried out through the entrypoint filp_open(), not sys_open(). sys_open() in turn calls filp_open(). So merely moving the forcing of the flag on 64-bit resolves this situation there, though not for 32-bit, whose solution is to appear in the sequel.

-- wli

fs/open.c +2 -3
Index: mm5-2.6.7/fs/open.c
===================================================================
@@ -755,6 +755,8 @@
 	int namei_flags, error;
 	struct nameidata nd;
 
+	if (BITS_PER_LONG > 32)
+		flags |= O_LARGEFILE;
 	namei_flags = flags;
 	if ((namei_flags+1) & O_ACCMODE)
 		namei_flags++;
@@ -943,9 +945,6 @@
 	char * tmp;
 	int fd, error;
 
-#if BITS_PER_LONG != 32
-	flags |= O_LARGEFILE;
-#endif
 	tmp = getname(filename);
 	fd = PTR_ERR(tmp);
 	if (!IS_ERR(tmp)) {
#1 lore raw

force O_LARGEFILE in sys_swapon() and sys_swapoff()

Quoted 6 lines · William Lee Irwin III

Internal kernel open() of files barfs in important contexts, for instance, using strict non-overcommit with enough swap for large commitments. This is carried out through the entrypoint filp_open(), not sys_open(). sys_open() in turn calls filp_open(). So merely moving the forcing of the flag on 64-bit resolves this situation there, though not for 32-bit, whose solution is to appear in the sequel.

For 32-bit, one quickly discovers that swapon() is not given an fd already opened with O_LARGEFILE to act upon and the forcing of O_LARGEFILE for 64-bit is irrelevant, as the system call's argument is a path. So this patch manually forces it for swapon() and swapoff().

-- wli

mm/swapfile.c +2 -2
Index: mm5-2.6.7/mm/swapfile.c
===================================================================
@@ -1085,7 +1085,7 @@
 	if (IS_ERR(pathname))
 		goto out;
 
-	victim = filp_open(pathname, O_RDWR, 0);
+	victim = filp_open(pathname, O_RDWR|O_LARGEFILE, 0);
 	putname(pathname);
 	err = PTR_ERR(victim);
 	if (IS_ERR(victim))
@@ -1354,7 +1354,7 @@
 		name = NULL;
 		goto bad_swap_2;
 	}
-	swap_file = filp_open(name, O_RDWR, 0);
+	swap_file = filp_open(name, O_RDWR|O_LARGEFILE, 0);
 	error = PTR_ERR(swap_file);
 	if (IS_ERR(swap_file)) {
 		swap_file = NULL;
#2 lore raw

Re: force O_LARGEFILE in sys_swapon() and sys_swapoff()

Quoted 5 lines · William Lee Irwin III

For 32-bit, one quickly discovers that swapon() is not given an fd already opened with O_LARGEFILE to act upon and the forcing of O_LARGEFILE for 64-bit is irrelevant, as the system call's argument is a path. So this patch manually forces it for swapon() and swapoff().

This one looks good, thanks. I'm not so sure of your more general patch to open, others know better on that.

I doubt huge amounts of swap work at all well when used, but you're really concerned with commit at present: of course it was silly not to have used O_LARGEFILE here.

Hugh

#3 lore raw

Re: force O_LARGEFILE in sys_swapon() and sys_swapoff()

Quoted 4 lines · William Lee Irwin III
Quoted 4 lines

For 32-bit, one quickly discovers that swapon() is not given an fd already opened with O_LARGEFILE to act upon and the forcing of O_LARGEFILE for 64-bit is irrelevant, as the system call's argument is a path. So this patch manually forces it for swapon() and swapoff().

Quoted 5 lines · Hugh Dickins

This one looks good, thanks. I'm not so sure of your more general patch to open, others know better on that.
I doubt huge amounts of swap work at all well when used, but you're really concerned with commit at present: of course it was silly not to have used O_LARGEFILE here.

The story on the more general one is that it silently broke -EFBIG returns for 32-bit emulation. Arnd posted a patch to consolidate 32-bit open() emulation, which I followed up with a hoist of the O_LARGEFILE check to there above filp_open(). The forcing of O_LARGEFILE is still needed for other, non-open()-related situations, and I may need to sweep 32-bit emulation for other syscalls. Of course, it's a lose-lose situation, as 64-bit now can't do internal kernel filp_open() on files larger than 64-bit unsafe 32-bit apps can. In short, a large headache.

-- wli

#4 lore raw
In reply to William Lee Irwin III
Quoted 3 lines · William Lee Irwin III

sys_open() in turn calls filp_open(). So merely
moving the forcing of the flag on 64-bit resolves this situation there, though not for 32-bit, whose solution is to appear in the sequel.

Unfortunately, this will also cause problems for 32-bit emulation, where sys32_open currently calls filp_open without forcing O_LARGEFILE for 32 bit applications.
I also noticed that this behavior currently is not implemented on all architectures, which in turn would need the patch below.

Maybe you can find a way to fix both problems.

	Arnd <><
 arch/ia64/ia32/ia32_entry.S         |    2 +-
 arch/ia64/ia32/sys_ia32.c           |   31 -------------------------------
 arch/mips/kernel/scall64-o32.S      |    2 +-
 arch/parisc/kernel/syscall_table.S  |    2 +-
 arch/ppc64/kernel/misc.S            |    2 +-
 arch/ppc64/kernel/sys_ppc32.c       |   31 -------------------------------
 arch/s390/kernel/compat_wrapper.S   |    2 +-
 arch/sparc64/kernel/sparc64_ksyms.c |    2 --
 arch/sparc64/kernel/sys32.S         |    2 +-
 arch/sparc64/kernel/sys_sparc32.c   |   32 --------------------------------
 arch/sparc64/kernel/sys_sunos32.c   |    4 +---
 arch/sparc64/solaris/fs.c           |    4 +---
 arch/x86_64/ia32/ia32entry.S        |    2 +-
 arch/x86_64/ia32/sys_ia32.c         |   24 ------------------------
 fs/compat.c                         |   31 +++++++++++++++++++++++++++++++
 include/linux/compat.h              |    4 ++++
 16 files changed, 44 insertions(+), 133 deletions(-)

===== arch/ia64/ia32/ia32_entry.S 1.39 vs edited =====

arch/ia64/ia32/ia32_entry.S +1 -1
@@ -213,7 +213,7 @@
 	data8 sys32_fork
 	data8 sys_read
 	data8 sys_write
-	data8 sys32_open	  /* 5 */
+	data8 compat_sys_open	  /* 5 */
 	data8 sys_close
 	data8 sys32_waitpid
 	data8 sys_creat

===== arch/ia64/ia32/sys_ia32.c 1.100 vs edited =====

arch/ia64/ia32/sys_ia32.c +0 -31
@@ -2450,37 +2450,6 @@
 	return ret;
 }
 
-/*
- * Exactly like fs/open.c:sys_open(), except that it doesn't set the O_LARGEFILE flag.
- */
-asmlinkage long
-sys32_open (const char * filename, int flags, int mode)
-{
-	char * tmp;
-	int fd, error;
-
-	tmp = getname(filename);
-	fd = PTR_ERR(tmp);
-	if (!IS_ERR(tmp)) {
-		fd = get_unused_fd();
-		if (fd >= 0) {
-			struct file *f = filp_open(tmp, flags, mode);
-			error = PTR_ERR(f);
-			if (IS_ERR(f))
-				goto out_error;
-			fd_install(fd, f);
-		}
-out:
-		putname(tmp);
-	}
-	return fd;
-
-out_error:
-	put_unused_fd(fd);
-	fd = error;
-	goto out;
-}
-
 /* Structure for ia32 emulation on ia64 */
 struct epoll_event32
 {

===== arch/mips/kernel/scall64-o32.S 1.6 vs edited =====

arch/mips/kernel/scall64-o32.S +1 -1
@@ -263,7 +263,7 @@
 	sys	sys_fork	0
 	sys	sys_read	3
 	sys	sys_write	3
-	sys	sys_open	3			/* 4005 */
+	sys	compat_sys_open	3			/* 4005 */
 	sys	sys_close	1
 	sys	sys_waitpid	3
 	sys	sys_creat	2

===== arch/parisc/kernel/syscall_table.S 1.8 vs edited =====

arch/parisc/kernel/syscall_table.S +1 -1
@@ -65,7 +65,7 @@
 	ENTRY_SAME(fork_wrapper)
 	ENTRY_SAME(read)
 	ENTRY_SAME(write)
-	ENTRY_SAME(open)		/* 5 */
+	ENTRY_COMP(open)		/* 5 */
 	ENTRY_SAME(close)
 	ENTRY_SAME(waitpid)
 	ENTRY_SAME(creat)

===== arch/ppc64/kernel/misc.S 1.83 vs edited =====

arch/ppc64/kernel/misc.S +1 -1
@@ -612,7 +612,7 @@
 	.llong .ppc_fork
 	.llong .sys_read
 	.llong .sys_write
-	.llong .sys32_open		/* 5 */
+	.llong .compat_sys_open		/* 5 */
 	.llong .sys_close
 	.llong .sys32_waitpid
 	.llong .sys32_creat

===== arch/ppc64/kernel/sys_ppc32.c 1.95 vs edited =====

arch/ppc64/kernel/sys_ppc32.c +0 -31
@@ -905,37 +905,6 @@
 	return sys_lseek(fd, (int)offset, origin);
 }
 
-/*
- * This is just a version for 32-bit applications which does
- * not force O_LARGEFILE on.
- */
-asmlinkage long sys32_open(const char __user * filename, int flags, int mode)
-{
-	char * tmp;
-	int fd, error;
-
-	tmp = getname(filename);
-	fd = PTR_ERR(tmp);
-	if (!IS_ERR(tmp)) {
-		fd = get_unused_fd();
-		if (fd >= 0) {
-			struct file * f = filp_open(tmp, flags, mode);
-			error = PTR_ERR(f);
-			if (IS_ERR(f))
-				goto out_error;
-			fd_install(fd, f);
-		}
-out:
-		putname(tmp);
-	}
-	return fd;
-
-out_error:
-	put_unused_fd(fd);
-	fd = error;
-	goto out;
-}
-
 /* Note: it is necessary to treat bufsiz as an unsigned int,
  * with the corresponding cast to a signed int to insure that the 
  * proper conversion (sign extension) between the register representation of a signed int (msr in 32-bit mode)

===== arch/s390/kernel/compat_wrapper.S 1.16 vs edited =====

arch/s390/kernel/compat_wrapper.S +1 -1
@@ -32,7 +32,7 @@
 	llgtr	%r2,%r2			# const char *
 	lgfr	%r3,%r3			# int
 	lgfr	%r4,%r4			# int
-	jg	sys_open		# branch to system call
+	jg	compat_sys_open		# branch to system call
 
 	.globl  sys32_close_wrapper 
 sys32_close_wrapper:

===== arch/sparc64/kernel/sparc64_ksyms.c 1.70 vs edited =====

arch/sparc64/kernel/sparc64_ksyms.c +0 -2
@@ -89,7 +89,6 @@
 extern int svr4_setcontext(svr4_ucontext_t *uc, struct pt_regs *regs);
 extern int compat_sys_ioctl(unsigned int fd, unsigned int cmd, u32 arg);
 extern int (*handle_mathemu)(struct pt_regs *, struct fpustate *);
-extern long sparc32_open(const char * filename, int flags, int mode);
 extern int io_remap_page_range(struct vm_area_struct *vma, unsigned long from, unsigned long offset, unsigned long size, pgprot_t prot, int space);
 extern void (*prom_palette)(int);
 
@@ -328,7 +327,6 @@
 EXPORT_SYMBOL(svr4_getcontext);
 EXPORT_SYMBOL(svr4_setcontext);
 EXPORT_SYMBOL(compat_sys_ioctl);
-EXPORT_SYMBOL(sparc32_open);
 EXPORT_SYMBOL(sys_close);
 #endif
 

===== arch/sparc64/kernel/sys32.S 1.9 vs edited =====

arch/sparc64/kernel/sys32.S +1 -1
@@ -106,7 +106,7 @@
 SIGN2(sys32_kill, sys_kill, %o0, %o1)
 SIGN1(sys32_nice, sys_nice, %o0)
 SIGN1(sys32_lseek, sys_lseek, %o1)
-SIGN2(sys32_open, sparc32_open, %o1, %o2)
+SIGN2(sys32_open, compat_sys_open, %o1, %o2)
 SIGN1(sys32_readlink, sys_readlink, %o2)
 SIGN1(sys32_sched_get_priority_max, sys_sched_get_priority_max, %o0)
 SIGN1(sys32_sched_get_priority_min, sys_sched_get_priority_min, %o0)

===== arch/sparc64/kernel/sys_sparc32.c 1.102 vs edited =====

arch/sparc64/kernel/sys_sparc32.c +0 -32
@@ -1680,38 +1680,6 @@
 	return ret;
 }
 
-/* This is just a version for 32-bit applications which does
- * not force O_LARGEFILE on.
- */
-
-asmlinkage long sparc32_open(const char __user *filename,
-			     int flags, int mode)
-{
-	char * tmp;
-	int fd, error;
-
-	tmp = getname(filename);
-	fd = PTR_ERR(tmp);
-	if (!IS_ERR(tmp)) {
-		fd = get_unused_fd();
-		if (fd >= 0) {
-			struct file * f = filp_open(tmp, flags, mode);
-			error = PTR_ERR(f);
-			if (IS_ERR(f))
-				goto out_error;
-			fd_install(fd, f);
-		}
-out:
-		putname(tmp);
-	}
-	return fd;
-
-out_error:
-	put_unused_fd(fd);
-	fd = error;
-	goto out;
-}
-
 extern unsigned long do_mremap(unsigned long addr,
 	unsigned long old_len, unsigned long new_len,
 	unsigned long flags, unsigned long new_addr);

===== arch/sparc64/kernel/sys_sunos32.c 1.44 vs edited =====

arch/sparc64/kernel/sys_sunos32.c +1 -3
@@ -1175,13 +1175,11 @@
 	return rval;
 }
 
-extern asmlinkage long sparc32_open(const char * filename, int flags, int mode);
-
 asmlinkage int sunos_open(u32 fname, int flags, int mode)
 {
 	const char *filename = (const char *)(long)fname;
 
-	return sparc32_open(filename, flags, mode);
+	return compat_sys_open(filename, flags, mode);
 }
 
 #define SUNOS_EWOULDBLOCK 35

===== arch/sparc64/solaris/fs.c 1.19 vs edited =====

arch/sparc64/solaris/fs.c +1 -3
@@ -525,8 +525,6 @@
 	return error;
 }
 
-extern asmlinkage long sparc32_open(const char * filename, int flags, int mode);
-
 asmlinkage int solaris_open(u32 fname, int flags, u32 mode)
 {
 	const char *filename = (const char *)(long)fname;
@@ -542,7 +540,7 @@
 	if (flags & 0x800) fl |= O_NOCTTY;
 	flags = fl;
 
-	return sparc32_open(filename, flags, mode);
+	return compat_sys_open(filename, flags, mode);
 }
 
 #define SOL_F_SETLK	6

===== arch/x86_64/ia32/ia32entry.S 1.37 vs edited =====

arch/x86_64/ia32/ia32entry.S +1 -1
@@ -310,7 +310,7 @@
 	.quad stub32_fork
 	.quad sys_read
 	.quad sys_write
-	.quad sys32_open		/* 5 */
+	.quad compat_sys_open		/* 5 */
 	.quad sys_close
 	.quad sys32_waitpid
 	.quad sys_creat

===== arch/x86_64/ia32/sys_ia32.c 1.62 vs edited =====

arch/x86_64/ia32/sys_ia32.c +0 -24
@@ -1249,30 +1249,6 @@
 	return ret;
 } 
 
-asmlinkage long sys32_open(const char __user * filename, int flags, int mode)
-{
-	char * tmp;
-	int fd, error;
-
-	/* don't force O_LARGEFILE */
-	tmp = getname(filename);
-	fd = PTR_ERR(tmp);
-	if (!IS_ERR(tmp)) {
-		fd = get_unused_fd();
-		if (fd >= 0) {
-			struct file *f = filp_open(tmp, flags, mode);
-			error = PTR_ERR(f);
-			if (unlikely(IS_ERR(f))) {
-				put_unused_fd(fd); 
-				fd = error;
-			} else
-				fd_install(fd, f);
-		}
-		putname(tmp);
-	}
-	return fd;
-}
-
 struct sigevent32 { 
 	u32 sigev_value;
 	u32 sigev_signo; 

===== fs/compat.c 1.32 vs edited =====

fs/compat.c +31 -0
@@ -143,6 +143,37 @@
 }
 
 /*
+ * Exactly like fs/open.c:sys_open(), except that it doesn't set the O_LARGEFILE flag.
+ */
+asmlinkage long
+compat_sys_open(const char __user * filename, int flags, int mode)
+{
+	char * tmp;
+	int fd, error;
+
+	tmp = getname(filename);
+	fd = PTR_ERR(tmp);
+	if (!IS_ERR(tmp)) {
+		fd = get_unused_fd();
+		if (fd >= 0) {
+			struct file *f = filp_open(tmp, flags, mode);
+			error = PTR_ERR(f);
+			if (IS_ERR(f))
+				goto out_error;
+			fd_install(fd, f);
+		}
+out:
+		putname(tmp);
+	}
+	return fd;
+
+out_error:
+	put_unused_fd(fd);
+	fd = error;
+	goto out;
+}
+
+/*
  * The following statfs calls are copies of code from fs/open.c and
  * should be checked against those from time to time
  */

===== include/linux/compat.h 1.24 vs edited =====

include/linux/compat.h +4 -0
@@ -130,5 +130,9 @@
 		compat_ulong_t __user *outp, compat_ulong_t __user *exp,
 		struct compat_timeval __user *tvp);
 
+asmlinkage long compat_sys_open(const char __user * filename,
+				int flags, int mode);
+
+
 #endif /* CONFIG_COMPAT */
 #endif /* _LINUX_COMPAT_H */
#5 lore raw
Quoted 3 lines · William Lee Irwin III
Quoted 3 lines

sys_open() in turn calls filp_open(). So merely
moving the forcing of the flag on 64-bit resolves this situation there, though not for 32-bit, whose solution is to appear in the sequel.

Quoted 6 lines · Arnd Bergmann

Unfortunately, this will also cause problems for 32-bit emulation, where sys32_open currently calls filp_open without forcing O_LARGEFILE for 32 bit applications.
I also noticed that this behavior currently is not implemented on all architectures, which in turn would need the patch below. Maybe you can find a way to fix both problems.

Your patch is also necessary; thanks for covering these cases.

-- wli

#6 lore raw
Quoted 1 line · William Lee Irwin III

Your patch is also necessary; thanks for covering these cases.

I'm not sure if you understood the intention of compat_sys_open right. Old 32 bit applications assume they are not using O_LARGEFILE, so you can't switch it on unconditionally in filp_open() for those cases. With your patch applied, sys_open and compat_sys_open would be identical again, which reverses the point of my patch.

What is need is a way to turn on O_LARGEFILE on 64 bit archs for every use of filp_open _except_ from compat_sys_open.

	Arnd <><
#7 lore raw
Quoted 1 line · William Lee Irwin III
Quoted 1 line

Your patch is also necessary; thanks for covering these cases.

Quoted 7 lines · Arnd Bergmann

I'm not sure if you understood the intention of compat_sys_open right. Old 32 bit applications assume they are not using O_LARGEFILE, so you can't switch it on unconditionally in filp_open() for those cases. With your patch applied, sys_open and compat_sys_open would be identical again, which reverses the point of my patch. What is need is a way to turn on O_LARGEFILE on 64 bit archs for every use of filp_open _except_ from compat_sys_open.

Oh, that's easy, just shove the MAX_NON_LFS check into compat_sys_open().

-- wli

#8 lore raw
Quoted 1 line · William Lee Irwin III
Quoted 1 line
Quoted 1 line

Your patch is also necessary; thanks for covering these cases.

Quoted 7 lines · Arnd Bergmann
Quoted 7 lines

I'm not sure if you understood the intention of compat_sys_open right. Old 32 bit applications assume they are not using O_LARGEFILE, so you can't switch it on unconditionally in filp_open() for those cases. With your patch applied, sys_open and compat_sys_open would be identical again, which reverses the point of my patch. What is need is a way to turn on O_LARGEFILE on 64 bit archs for every use of filp_open _except_ from compat_sys_open.

Quoted 1 line · William Lee Irwin III

Oh, that's easy, just shove the MAX_NON_LFS check into compat_sys_open().

BTW, for some reason that's what I thought you were doing in your patch.

-- wli

#9 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 7 lines
Quoted 7 lines

I'm not sure if you understood the intention of compat_sys_open right. Old 32 bit applications assume they are not using O_LARGEFILE, so you can't switch it on unconditionally in filp_open() for those cases. With your patch applied, sys_open and compat_sys_open would be identical again, which reverses the point of my patch. What is need is a way to turn on O_LARGEFILE on 64 bit archs for every use of filp_open _except_ from compat_sys_open.

Quoted 1 line · William Lee Irwin III
Quoted 1 line

Oh, that's easy, just shove the MAX_NON_LFS check into compat_sys_open().

Quoted 1 line · William Lee Irwin III

BTW, for some reason that's what I thought you were doing in your patch.

How does this look as an implementation of that suggestion, incremental to your compat_sys_open() consolidation patch?

-- wli

fs/compat.c +6 -0
Index: mm5-2.6.7/fs/compat.c
===================================================================
@@ -160,6 +160,12 @@
 			error = PTR_ERR(f);
 			if (IS_ERR(f))
 				goto out_error;
+			if (!(filp->f_flags & O_LARGEFILE) &&
+				i_size_read(file->f_dentry->d_inode) > MAX_NON_LFS) {
+				error = -EFBIG;
+				filp_close(filp, current->files);
+				goto out_error;
+			}
 			fd_install(fd, f);
 		}
 out:
#10 lore raw
Quoted 2 lines · William Lee Irwin III

How does this look as an implementation of that suggestion, incremental to your compat_sys_open() consolidation patch?

At first sight, it looks like (filp->f_flags & O_LARGEFILE) will always be true after calling filp_open, but I don't have time to look closer today.

	Arnd <><
#11 lore raw
Quoted 2 lines · William Lee Irwin III
Quoted 2 lines

How does this look as an implementation of that suggestion, incremental to your compat_sys_open() consolidation patch?

Quoted 3 lines · Arnd Bergmann

At first sight, it looks like (filp->f_flags & O_LARGEFILE) will always be true after calling filp_open, but I don't have time to look closer today.

Quite right.

fs/compat.c +6 -0
Index: mm5-2.6.7/fs/compat.c
===================================================================
@@ -160,6 +160,12 @@
 			error = PTR_ERR(f);
 			if (IS_ERR(f))
 				goto out_error;
+			if (!(flags & O_LARGEFILE) &&
+				i_size_read(f->f_dentry->d_inode) > MAX_NON_LFS) {
+				error = -EFBIG;
+				filp_close(f, current->files);
+				goto out_error;
+			}
 			fd_install(fd, f);
 		}
 out:
#12 lore raw
Quoted 3 lines · Arnd Bergmann
Quoted 3 lines

At first sight, it looks like (filp->f_flags & O_LARGEFILE) will always be true after calling filp_open, but I don't have time to look closer today.

Quoted 1 line · William Lee Irwin III

Quite right.

[...]

fs/compat.c line 168

Quoted 6 lines
fs/compat.c +6 -0
+			if (!(flags & O_LARGEFILE) &&
+				i_size_read(f->f_dentry->d_inode) > MAX_NON_LFS) {
+				error = -EFBIG;
+				filp_close(f, current->files);
+				goto out_error;
+			}

With my stuff, the compat_sys_open() patch, and the compat_sys_open() MAX_NON_LFS patch in place, things pass quick manual backward compatibility testing (basically, one small app that does open() of a large file sees the right results with and without O_LARGEFILE where it didn't before).

-- wli