Messages in this thread | | | From | Todd Kjos <> | Date | Tue, 19 Apr 2022 11:24:13 -0700 | Subject | Re: [PATCH] binder: hold fd_install until allocating fds first |
| |
On Fri, Mar 25, 2022 at 4:25 PM Carlos Llamas <cmllamas@google.com> wrote: > > Al noted in [1] that fd_install can't be undone, so it must come last in > the fd translation sequence, only after we've successfully reserved all > descriptors and copied them into the transaction buffer. > > This patch takes Al's proposed fix in [2] and makes a few tweaks to fold > the traversal of t->fd_fixups during release. > > [1] https://lore.kernel.org/driverdev-devel/YHnJwRvUhaK3IM0l@zeniv-ca.linux.org.uk > [2] https://lore.kernel.org/driverdev-devel/YHo6Ln9VI1T7RmLK@zeniv-ca.linux.org.uk > > Cc: Christian Brauner <christian.brauner@ubuntu.com> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
Acked-by: Todd Kjos <tkjos@google.com>
> --- > drivers/android/binder.c | 34 ++++++++++++------------------- > drivers/android/binder_internal.h | 2 ++ > 2 files changed, 15 insertions(+), 21 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 8351c5638880..bfadc0c4a442 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -1481,6 +1481,8 @@ static void binder_free_txn_fixups(struct binder_transaction *t) > > list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) { > fput(fixup->file); > + if (fixup->target_fd >= 0) > + put_unused_fd(fixup->target_fd); > list_del(&fixup->fixup_entry); > kfree(fixup); > } > @@ -2220,6 +2222,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset, > } > fixup->file = file; > fixup->offset = fd_offset; > + fixup->target_fd = -1; > trace_binder_transaction_fd_send(t, fd, fixup->offset); > list_add_tail(&fixup->fixup_entry, &t->fd_fixups); > > @@ -4067,10 +4070,9 @@ static int binder_wait_for_work(struct binder_thread *thread, > * 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. > + * new fds first and only then install the files. > * > - * If we fail to allocate an fd, then free the resources by > - * fput'ing files that have not been processed and ksys_close'ing > + * If we fail to allocate an fd, skip the install and release > * any fds that have already been allocated. > */ > static int binder_apply_fd_fixups(struct binder_proc *proc, > @@ -4087,41 +4089,31 @@ static int binder_apply_fd_fixups(struct binder_proc *proc, > "failed fd fixup txn %d fd %d\n", > t->debug_id, fd); > ret = -ENOMEM; > - break; > + goto err; > } > 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; > + goto err; > } > } > 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); > - } > + fd_install(fixup->target_fd, fixup->file); > list_del(&fixup->fixup_entry); > kfree(fixup); > } > > return ret; > + > +err: > + binder_free_txn_fixups(t); > + return ret; > } > > static int binder_thread_read(struct binder_proc *proc, > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h > index d6b6b8cb7346..cf70a104594d 100644 > --- a/drivers/android/binder_internal.h > +++ b/drivers/android/binder_internal.h > @@ -515,6 +515,7 @@ struct binder_thread { > * @fixup_entry: list entry > * @file: struct file to be associated with new fd > * @offset: offset in buffer data to this fixup > + * @target_fd: fd to use by the target to install @file > * > * List element for fd fixups in a transaction. Since file > * descriptors need to be allocated in the context of the > @@ -525,6 +526,7 @@ struct binder_txn_fd_fixup { > struct list_head fixup_entry; > struct file *file; > size_t offset; > + int target_fd; > }; > > struct binder_transaction { > -- > 2.35.1.1021.g381101b075-goog >
| |