Getting rid of get_unused_fd()

Hi,

Please find the 9th revision of my patchset to remove get_unused_fd() macro.

In linux-next, tag next-20141013, they're currently:

- 33 calls to fd_install()
with one call part of anon_inode_getfd()
- 27 calls to get_unused_fd_flags()
with one call part of anon_inode_getfd() with another part of get_unused_fd() macro
- 13 calls to anon_inode_getfd()
- 8 calls to anon_inode_getfile()
with one call part of anon_inode_getfd()
- 5 calls to get_unused_fd()

The following patchset replaces the 5 last calls to get_unused_fd() by calls to get_unused_fd_flags(0) and remove the macro so that it won't be used in newer code.

For some detailed background information, please have a look at my previous patchset's cover letter[1].

Changes from patchset v8[1]
- fanotify: enable close-on-exec on events' fd when requested in fanotify_init()
DROPPED: applied upstream, commit 0b37e097a648.
- reduce the amount of explanation in cover letter

[1] http://lkml.kernel.org/r/[email protected]

Yann Droneaud (5):
ia64: trivial: replace get_unused_fd() by get_unused_fd_flags(0) ppc/cell: trivial: replace get_unused_fd() by get_unused_fd_flags(0)
binfmt_misc: trivial: replace get_unused_fd() by

    get_unused_fd_flags(0)

file: trivial: replace get_unused_fd() by get_unused_fd_flags(0)
file: remove get_unused_fd() macro

 arch/ia64/kernel/perfmon.c                | 2 +-
 arch/powerpc/platforms/cell/spufs/inode.c | 4 ++--
 fs/binfmt_misc.c                          | 2 +-
 fs/file.c                                 | 2 +-
 include/linux/file.h                      | 1 -
 5 files changed, 5 insertions(+), 6 deletions(-)

1.9.3

This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code.

In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace.

---
 arch/ia64/kernel/perfmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5845ffea67c3..dc063fe6646a 100644
@@ -2662,7 +2662,7 @@ pfm_context_create(pfm_context_t *ctx, void *arg, int count, struct pt_regs *reg
 
 	ret = -ENOMEM;
 
-	fd = get_unused_fd();
+	fd = get_unused_fd_flags(0);
 	if (fd < 0)
 		return fd;
 

1.9.3

This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code.

In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace.

---
 arch/powerpc/platforms/cell/spufs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 87ba7cf99cd7..51effcec30d8 100644
@@ -301,7 +301,7 @@ static int spufs_context_open(struct path *path)
 	int ret;
 	struct file *filp;
 
-	ret = get_unused_fd();
+	ret = get_unused_fd_flags(0);
 	if (ret < 0)
 		return ret;
 

@@ -518,7 +518,7 @@ static int spufs_gang_open(struct path *path)
 	int ret;
 	struct file *filp;
 
-	ret = get_unused_fd();
+	ret = get_unused_fd_flags(0);
 	if (ret < 0)
 		return ret;
 

1.9.3

This is fine by me, do you want an ack, or do you want us to take it via the powerpc tree?

If the former:

cheers

Hi,

The patch was added in -mm by Andrew, so I guess the patch will be merged sooner or later.

Anyway, you could investigate to check if O_CLOEXEC could be used instead of 0 in call to get_unused_fd_flags().

Thanks a lot.

Regards.

Yann Droneaud
OPTEYA

This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code.

In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace.

---
 fs/binfmt_misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fd8beb9657a2..1cc5377ba955 100644
@@ -153,7 +153,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
 		/* if the binary should be opened on behalf of the
 		 * interpreter than keep it open and assign descriptor
 		 * to it */
- 		fd_binary = get_unused_fd();
+		fd_binary = get_unused_fd_flags(0);
  		if (fd_binary < 0) {
  			retval = fd_binary;
  			goto _ret;

1.9.3

This patch replaces calls to get_unused_fd() with equivalent call to get_unused_fd_flags(0) to preserve current behavor for existing code.

In a further patch, get_unused_fd() will be removed so that new code start using get_unused_fd_flags(), with the hope O_CLOEXEC could be used, either by default or choosen by userspace.

---
 fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ab3eb6a88239..ee738ea028fa 100644
@@ -869,7 +869,7 @@ SYSCALL_DEFINE1(dup, unsigned int, fildes)
 	struct file *file = fget_raw(fildes);
 
 	if (file) {
-		ret = get_unused_fd();
+		ret = get_unused_fd_flags(0);
 		if (ret >= 0)
 			fd_install(ret, file);
 		else

1.9.3

Macro get_unused_fd() is used to allocate a file descriptor with default flags. Those default flags (0) don't enable close-on-exec.

This can be seen as an unsafe default: in most case close-on-exec should be enabled to not leak file descriptor across exec().

It would be better to have a "safer" default set of flags, eg. O_CLOEXEC must be used to enable close-on-exec.

Instead this patch removes get_unused_fd() so that out of tree modules won't be affect by a runtime behavor change which might introduce other kind of bugs: it's better to catch the change at build time, making it easier to fix.

Removing the macro will also promote use of get_unused_fd_flags() (or anon_inode_getfd()) with flags provided by userspace. Or, if flags cannot be given by userspace, with flags set to O_CLOEXEC by default.

---
 include/linux/file.h | 1 -
 1 file changed, 1 deletion(-)

index 4d69123377a2..f87d30882a24 100644
@@ -66,7 +66,6 @@ extern void set_close_on_exec(unsigned int fd, int flag);
 extern bool get_close_on_exec(unsigned int fd);
 extern void put_filp(struct file *);
 extern int get_unused_fd_flags(unsigned flags);
-#define get_unused_fd() get_unused_fd_flags(0)
 extern void put_unused_fd(unsigned int fd);
 
 extern void fd_install(unsigned int fd, struct file *file);

1.9.3