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 {
