Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Apr 16, 2021 at 05:30:41PM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:58:15PM +0200, Christian Brauner wrote:
> 
> > They could probably refactor this but I'm not sure why they'd bother. If
> > they fail processing any of those files they end up aborting the
> > whole transaction.
> > (And the original code didn't check the error code btw.)
> 
> Wait a sec...  What does aborting the transaction do to descriptor table?
> <rereads>
> Oh, lovely...
> 
> binder_apply_fd_fixups() is deeply misguided.  What it should do is
> 	* go through t->fd_fixups, reserving descriptor numbers and
> putting them into t->buffer (and I'd probably duplicate them into
> struct binder_txn_fd_fixup).  Cleanup in case of failure: go through
> the list, releasing the descriptors we'd already reserved, doing
> fput() on fixup->file in all entries and freeing the entries as
> we go.
> 	* On success, go through the list, doing fd_install() and
> freeing the entries.

Something like this:

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..b0c5f7e625f3 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2195,6 +2195,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
 	fixup->offset = fd_offset;
 	trace_binder_transaction_fd_send(t, fd, fixup->offset);
 	list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
+	fixup->target_fd = -1;
 
 	return ret;
 
@@ -3707,25 +3708,10 @@ static int binder_wait_for_work(struct binder_thread *thread,
 	return ret;
 }
 
-/**
- * binder_apply_fd_fixups() - finish fd translation
- * @proc:         binder_proc associated @t->buffer
- * @t:	binder transaction with list of fd fixups
- *
- * Now that we are in the context of the transaction target
- * process, we can allocate and install fds. Process the
- * list of fds to translate and fixup the buffer with the
- * new fds.
- *
- * If we fail to allocate an fd, then free the resources by
- * fput'ing files that have not been processed and ksys_close'ing
- * any fds that have already been allocated.
- */
-static int binder_apply_fd_fixups(struct binder_proc *proc,
+static int binder_reserve_fds(struct binder_proc *proc,
 				  struct binder_transaction *t)
 {
-	struct binder_txn_fd_fixup *fixup, *tmp;
-	int ret = 0;
+	struct binder_txn_fd_fixup *fixup;
 
 	list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
 		int fd = get_unused_fd_flags(O_CLOEXEC);
@@ -3734,42 +3720,55 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
 			binder_debug(BINDER_DEBUG_TRANSACTION,
 				     "failed fd fixup txn %d fd %d\n",
 				     t->debug_id, fd);
-			ret = -ENOMEM;
-			break;
+			return -ENOMEM;
 		}
 		binder_debug(BINDER_DEBUG_TRANSACTION,
 			     "fd fixup txn %d fd %d\n",
 			     t->debug_id, fd);
 		trace_binder_transaction_fd_recv(t, fd, fixup->offset);
-		fd_install(fd, fixup->file);
-		fixup->file = NULL;
+		fixup->target_fd = fd;
 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
 						fixup->offset, &fd,
-						sizeof(u32))) {
-			ret = -EINVAL;
-			break;
-		}
+						sizeof(u32)))
+			return -EINVAL;
 	}
-	list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
-		if (fixup->file) {
-			fput(fixup->file);
-		} else if (ret) {
-			u32 fd;
-			int err;
-
-			err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
-							    t->buffer,
-							    fixup->offset,
-							    sizeof(fd));
-			WARN_ON(err);
-			if (!err)
-				binder_deferred_fd_close(fd);
+	return 0;
+}
+
+/**
+ * binder_apply_fd_fixups() - finish fd translation
+ * @proc:         binder_proc associated @t->buffer
+ * @t:	binder transaction with list of fd fixups
+ *
+ * Now that we are in the context of the transaction target
+ * process, we can allocate fds. Process the list of fds to
+ * translate and fixup the buffer with the new fds.
+ *
+ * If we fail to allocate an fd, then free the resources by
+ * releasing fds we'd allocated.  Otherwise transfer all files
+ * from fixups to the descriptors we'd allocated for them.
+ *
+ * In either case, finish with freeing the fixups.
+ */
+static int binder_apply_fd_fixups(struct binder_proc *proc,
+				  struct binder_transaction *t)
+{
+	struct binder_txn_fd_fixup *fixup;
+	int err = binder_reserve_fds(proc, t);
+
+	if (unlikely(err)) {
+		list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
+			if (fixup->target_fd >= 0)
+				put_unused_fd(fixup->target_fd);
+		}
+	} else {
+		list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
+			fd_install(fixup->target_fd, fixup->file);
+			fixup->file = NULL;
 		}
-		list_del(&fixup->fixup_entry);
-		kfree(fixup);
 	}
-
-	return ret;
+	binder_free_txn_fixups(t);
+	return err;
 }
 
 static int binder_thread_read(struct binder_proc *proc,
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 6cd79011e35d..16ffc5f748ce 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -497,6 +497,7 @@ struct binder_txn_fd_fixup {
 	struct list_head fixup_entry;
 	struct file *file;
 	size_t offset;
+	int target_fd;
 };
 
 struct binder_transaction {