Commit 6319194e authored by Al Viro's avatar Al Viro
Browse files

Unify the primitives for file descriptor closing 



Currently we have 3 primitives for removing an opened file from descriptor
table - pick_file(), __close_fd_get_file() and close_fd_get_file().  Their
calling conventions are rather odd and there's a code duplication for no
good reason.  They can be unified -

1) have __range_close() cap max_fd in the very beginning; that way
we don't need separate way for pick_file() to report being past the end
of descriptor table.

2) make {__,}close_fd_get_file() return file (or NULL) directly, rather
than returning it via struct file ** argument.  Don't bother with
(bogus) return value - nobody wants that -ENOENT.

3) make pick_file() return NULL on unopened descriptor - the only caller
that used to care about the distinction between descriptor past the end
of descriptor table and finding NULL in descriptor table doesn't give
a damn after (1).

4) lift ->files_lock out of pick_file()

That actually simplifies the callers, as well as the primitives themselves.
Code duplication is also gone...

Reviewed-by: default avatarChristian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: default avatarAl Viro <viro@zeniv.linux.org.uk>
parent 81132a39

drivers/android/binder.c

+1 −1
Original line number Diff line number Diff line
@@ -1855,7 +1855,7 @@ static void binder_deferred_fd_close(int fd) 
	if (!twcb)

		return;

	init_task_work(&twcb->twork, binder_do_fd_close);

	close_fd_get_file(fd, &twcb->file);

	twcb->file = close_fd_get_file(fd);

	if (twcb->file) {

		filp_close(twcb->file, current->files);

		task_work_add(current, &twcb->twork, TWA_RESUME);

fs/file.c

+29 −48
Original line number Diff line number Diff line
@@ -630,32 +630,23 @@ EXPORT_SYMBOL(fd_install); 
 * @files: file struct to retrieve file from

 * @fd: file descriptor to retrieve file for

 *

 * If this functions returns an EINVAL error pointer the fd was beyond the

 * current maximum number of file descriptors for that fdtable.

 * Context: files_lock must be held.

 *

 * Returns: The file associated with @fd, on error returns an error pointer.

 * Returns: The file associated with @fd (NULL if @fd is not open)

 */

static struct file *pick_file(struct files_struct *files, unsigned fd)

{

	struct fdtable *fdt = files_fdtable(files);

	struct file *file;

	struct fdtable *fdt;



	spin_lock(&files->file_lock);

	fdt = files_fdtable(files);

	if (fd >= fdt->max_fds) {

		file = ERR_PTR(-EINVAL);

		goto out_unlock;

	}

	if (fd >= fdt->max_fds)

		return NULL;



	file = fdt->fd[fd];

	if (!file) {

		file = ERR_PTR(-EBADF);

		goto out_unlock;

	}

	if (file) {

		rcu_assign_pointer(fdt->fd[fd], NULL);

		__put_unused_fd(files, fd);



out_unlock:

	spin_unlock(&files->file_lock);

	}

	return file;

}
@@ -664,8 +655,10 @@ int close_fd(unsigned fd) 
	struct files_struct *files = current->files;

	struct file *file;



	spin_lock(&files->file_lock);

	file = pick_file(files, fd);

	if (IS_ERR(file))

	spin_unlock(&files->file_lock);

	if (!file)

		return -EBADF;



	return filp_close(file, files);
@@ -702,20 +695,25 @@ static inline void __range_cloexec(struct files_struct *cur_fds, 
static inline void __range_close(struct files_struct *cur_fds, unsigned int fd,

				 unsigned int max_fd)

{

	unsigned n;



	rcu_read_lock();

	n = last_fd(files_fdtable(cur_fds));

	rcu_read_unlock();

	max_fd = min(max_fd, n);



	while (fd <= max_fd) {

		struct file *file;



		spin_lock(&cur_fds->file_lock);

		file = pick_file(cur_fds, fd++);

		if (!IS_ERR(file)) {

		spin_unlock(&cur_fds->file_lock);



		if (file) {

			/* found a valid file to close */

			filp_close(file, cur_fds);

			cond_resched();

			continue;

		}



		/* beyond the last fd in that table */

		if (PTR_ERR(file) == -EINVAL)

			return;

	}

}
@@ -795,26 +793,9 @@ int __close_range(unsigned fd, unsigned max_fd, unsigned int flags) 
 * See close_fd_get_file() below, this variant assumes current->files->file_lock

 * is held.

 */

int __close_fd_get_file(unsigned int fd, struct file **res)

struct file *__close_fd_get_file(unsigned int fd)

{

	struct files_struct *files = current->files;

	struct file *file;

	struct fdtable *fdt;



	fdt = files_fdtable(files);

	if (fd >= fdt->max_fds)

		goto out_err;

	file = fdt->fd[fd];

	if (!file)

		goto out_err;

	rcu_assign_pointer(fdt->fd[fd], NULL);

	__put_unused_fd(files, fd);

	get_file(file);

	*res = file;

	return 0;

out_err:

	*res = NULL;

	return -ENOENT;

	return pick_file(current->files, fd);

}



/*
@@ -822,16 +803,16 @@ int __close_fd_get_file(unsigned int fd, struct file **res) 
 * The caller must ensure that filp_close() called on the file, and then

 * an fput().

 */

int close_fd_get_file(unsigned int fd, struct file **res)

struct file *close_fd_get_file(unsigned int fd)

{

	struct files_struct *files = current->files;

	int ret;

	struct file *file;



	spin_lock(&files->file_lock);

	ret = __close_fd_get_file(fd, res);

	file = pick_file(files, fd);

	spin_unlock(&files->file_lock);



	return ret;

	return file;

}



void do_close_on_exec(struct files_struct *files)

fs/internal.h

+1 −1
Original line number Diff line number Diff line
@@ -125,7 +125,7 @@ extern struct file *do_file_open_root(const struct path *, 
		const char *, const struct open_flags *);

extern struct open_how build_open_how(int flags, umode_t mode);

extern int build_open_flags(const struct open_how *how, struct open_flags *op);

extern int __close_fd_get_file(unsigned int fd, struct file **res);

extern struct file *__close_fd_get_file(unsigned int fd);



long do_sys_ftruncate(unsigned int fd, loff_t length, int small);

int chmod_common(const struct path *path, umode_t mode);

fs/io_uring.c

+2 −5
Original line number Diff line number Diff line
@@ -5137,13 +5137,10 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags) 
		return -EAGAIN;

	}



	ret = __close_fd_get_file(close->fd, &file);

	file = __close_fd_get_file(close->fd);

	spin_unlock(&files->file_lock);

	if (ret < 0) {

		if (ret == -ENOENT)

			ret = -EBADF;

	if (!file)

		goto err;

	}



	/* No ->flush() or already async, safely close from here */

	ret = filp_close(file, current->files);

include/linux/fdtable.h

+1 −1
Original line number Diff line number Diff line
@@ -125,7 +125,7 @@ int iterate_fd(struct files_struct *, unsigned, 


extern int close_fd(unsigned int fd);

extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);

extern int close_fd_get_file(unsigned int fd, struct file **res);

extern struct file *close_fd_get_file(unsigned int fd);

extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds,

		      struct files_struct **new_fdp);