[PATCH v3 17/17] fs: close files from the highest descriptor down

[Christian Brauner <brauner@kernel.org>]

close_files(), __range_close() and close_cloexec_files() walk the
descriptor table from the lowest descriptor up. They used to call
filp_close(), which left the final __fput() to task work. task work is a
LIFO list, so the releases ran after the walk had finished and in the
opposite direction, highest descriptor first.

This dumb ordering is relevant for a bunch of broken but long-standing
cases. It matters whenever the ->flush() or ->release() of one file
waits for something that only the release of another file of the same
table provides. Then one of the two orders deadlocks and the other one
doesn't:

    exit, fd 3 is one end of a pipe    peer
    -------------------------------    ----
                                       splice(socket -> pipe)
                                         pipe_lock()
                                         waits for data or EOF
    close_files()
      fd 3: pipe_release()
        mutex_lock(&pipe->mutex)         held by the peer
      fd 5: the socket, not reached      would be the peer's EOF

Programs create the thing that guards or wakes another thing first.
Hence, it gets the lower descriptor which is the layout that breaks:

(1) a tap device released before the AF_LLC socket that holds a
    reference to it
(2) an unlinked fsdax file evicted before the pipe that holds its
    vmspliced pages
(3) an overlayfs directory whose release queues up behind an unlink that
    waits for a splice into the same directory

The exiting task is unkillable in all of them. All of that crap can
obviously also become a bug if you reorder the file descriptors.

Continue walking all three tables from the highest descriptor down. That
restores the order the deferred puts had. ->flush() moves with the
release. So it now runs highest descriptor first as well.

None of this fixes the underlying defects. For every one of these pairs
the mirrored layout deadlocked before and deadlocks
again now:

(1') splice() holding pipe->mutex across unbounded socket and tty I/O
(2') AF_LLC keeping a netdev reference without a NETDEV_UNREGISTER handler
(3') uninterruptible wait in dax_break_layout_final()
(4') ovl_splice_write() sleeping under the inode lock

It all predates the synchronous close and each should really get fixed.

Reported-by: Chris Mason <mason@kernel.org>
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/file.c | 55 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 76e328edf630..7f8d0afd8807 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -497,24 +497,21 @@ static struct fdtable *close_files(struct files_struct *files)
 	 * files structure.
 	 */
 	struct fdtable *fdt = rcu_dereference_raw(files->fdt);
-	unsigned int i, j = 0;
+	unsigned int j = fdt->max_fds / BITS_PER_LONG;
+
+	/* Highest fd first, the order the deferred puts ran in. */
+	while (j--) {
+		unsigned long set = fdt->open_fds[j];
 
-	for (;;) {
-		unsigned long set;
-		i = j * BITS_PER_LONG;
-		if (i >= fdt->max_fds)
-			break;
-		set = fdt->open_fds[j++];
 		while (set) {
-			if (set & 1) {
-				struct file *file = fdt->fd[i];
-				if (file) {
-					filp_close_sync(file, files);
-					cond_resched();
-				}
+			unsigned int bit = __fls(set);
+			struct file *file = fdt->fd[j * BITS_PER_LONG + bit];
+
+			set ^= 1UL << bit;
+			if (file) {
+				filp_close_sync(file, files);
+				cond_resched();
 			}
-			i++;
-			set >>= 1;
 		}
 	}
 
@@ -801,10 +798,14 @@ static inline void __range_close(struct files_struct *files, unsigned int fd,
 	n = last_fd(fdt);
 	max_fd = min(max_fd, n);
 
-	for (fd = find_next_bit(fdt->open_fds, max_fd + 1, fd);
-	     fd <= max_fd;
-	     fd = find_next_bit(fdt->open_fds, max_fd + 1, fd + 1)) {
-		file = file_close_fd_locked(files, fd);
+	/* Highest fd first, see close_files(). */
+	for (n = max_fd + 1; n > fd; ) {
+		unsigned int cur = find_last_bit(fdt->open_fds, n);
+
+		if (cur >= n || cur < fd)
+			break;
+		n = cur;
+		file = file_close_fd_locked(files, cur);
 		if (file) {
 			spin_unlock(&files->file_lock);
 			filp_close_sync(file, files);
@@ -908,20 +909,22 @@ void close_cloexec_files(struct files_struct *files)
 
 	/* exec unshares first */
 	spin_lock(&files->file_lock);
-	for (i = 0; ; i++) {
+	fdt = files_fdtable(files);
+	/* Highest fd first, see close_files(). */
+	for (i = fdt->max_fds / BITS_PER_LONG; i--; ) {
 		unsigned long set;
-		unsigned fd = i * BITS_PER_LONG;
+
 		fdt = files_fdtable(files);
-		if (fd >= fdt->max_fds)
-			break;
 		set = fdt->close_on_exec[i];
 		if (!set)
 			continue;
 		fdt->close_on_exec[i] = 0;
-		for ( ; set ; fd++, set >>= 1) {
+		while (set) {
+			unsigned int bit = __fls(set);
+			unsigned fd = i * BITS_PER_LONG + bit;
 			struct file *file;
-			if (!(set & 1))
-				continue;
+
+			set ^= 1UL << bit;
 			file = fdt->fd[fd];
 			if (!file)
 				continue;

-- 
2.53.0