On Wed, Nov 24, 2021 at 3:10 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Tue, Nov 23, 2021 at 11:17:37AM -0800, Todd Kjos wrote:
> > +/**
> > + * binder_do_deferred_txn_copies() - copy and fixup scatter-gather data
> > + * @alloc: binder_alloc associated with @buffer
> > + * @buffer: binder buffer in target process
> > + * @sgc_head: list_head of scatter-gather copy list
> > + * @pf_head: list_head of pointer fixup list
> > + *
> > + * Processes all elements of @sgc_head, applying fixups from @pf_head
> > + * and copying the scatter-gather data from the source process' user
> > + * buffer to the target's buffer. It is expected that the list creation
> > + * and processing all occurs during binder_transaction() so these lists
> > + * are only accessed in local context.
> > + *
> > + * Return: 0=success, else -errno
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This function is supposed to return negatives on error.
>
> > + */
> > +static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
> > + struct binder_buffer *buffer,
> > + struct list_head *sgc_head,
> > + struct list_head *pf_head)
> > +{
> > + int ret = 0;
> > + struct list_head *entry, *tmp;
> > + struct binder_ptr_fixup *pf =
> > + list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
> > + node);
> > +
> > + list_for_each_safe(entry, tmp, sgc_head) {
> > + size_t bytes_copied = 0;
> > + struct binder_sg_copy *sgc =
> > + container_of(entry, struct binder_sg_copy, node);
> > +
> > + while (bytes_copied < sgc->length) {
> > + size_t copy_size;
> > + size_t bytes_left = sgc->length - bytes_copied;
> > + size_t offset = sgc->offset + bytes_copied;
> > +
> > + /*
> > + * We copy up to the fixup (pointed to by pf)
> > + */
> > + copy_size = pf ? min(bytes_left, (size_t)pf->offset - offset)
> > + : bytes_left;
> > + if (!ret && copy_size)
> > + ret = binder_alloc_copy_user_to_buffer(
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "ret" is the number of bytes remaining to be copied.
Good catch. Thanks. Will fix.
>
>
> > + alloc, buffer,
> > + offset,
> > + sgc->uaddr + bytes_copied,
> > + copy_size);
> > + bytes_copied += copy_size;
> > + if (copy_size != bytes_left) {
> > + BUG_ON(!pf);
> > + /* we stopped at a fixup offset */
> > + if (pf->skip_size) {
> > + /*
> > + * we are just skipping. This is for
> > + * BINDER_TYPE_FDA where the translated
> > + * fds will be fixed up when we get
> > + * to target context.
> > + */
> > + bytes_copied += pf->skip_size;
> > + } else {
> > + /* apply the fixup indicated by pf */
> > + if (!ret)
> > + ret = binder_alloc_copy_to_buffer(
> > + alloc, buffer,
> > + pf->offset,
> > + &pf->fixup_data,
> > + sizeof(pf->fixup_data));
> > + bytes_copied += sizeof(pf->fixup_data);
> > + }
> > + list_del(&pf->node);
> > + kfree(pf);
> > + pf = list_first_entry_or_null(pf_head,
> > + struct binder_ptr_fixup, node);
> > + }
> > + }
> > + list_del(&sgc->node);
> > + kfree(sgc);
> > + }
> > + BUG_ON(!list_empty(pf_head));
> > + BUG_ON(!list_empty(sgc_head));
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * binder_cleanup_deferred_txn_lists() - free specified lists
> > + * @sgc_head: list_head of scatter-gather copy list
> > + * @pf_head: list_head of pointer fixup list
> > + *
> > + * Called to clean up @sgc_head and @pf_head if there is an
> > + * error.
> > + */
> > +static void binder_cleanup_deferred_txn_lists(struct list_head *sgc_head,
> > + struct list_head *pf_head)
> > +{
> > + struct list_head *entry, *tmp;
> > +
> > + list_for_each_safe(entry, tmp, sgc_head) {
> > + struct binder_sg_copy *sgc =
> > + container_of(entry, struct binder_sg_copy, node);
> > + list_del(&sgc->node);
> > + kfree(sgc);
> > + }
> > + list_for_each_safe(entry, tmp, pf_head) {
> > + struct binder_ptr_fixup *pf =
> > + container_of(entry, struct binder_ptr_fixup, node);
> > + list_del(&pf->node);
> > + kfree(pf);
> > + }
> > +}
> > +
> > +/**
> > + * binder_defer_copy() - queue a scatter-gather buffer for copy
> > + * @sgc_head: list_head of scatter-gather copy list
> > + * @offset: binder buffer offset in target process
> > + * @uaddr: user address in source process
> > + * @length: bytes to copy
> > + *
> > + * Specify a scatter-gather block to be copied. The actual copy must
> > + * be deferred until all the needed fixups are identified and queued.
> > + * Then the copy and fixups are done together so un-translated values
> > + * from the source are never visible in the target buffer.
> > + *
> > + * We are guaranteed that repeated calls to this function will have
> > + * monotonically increasing @offset values so the list will naturally
> > + * be ordered.
> > + *
> > + * Return: 0=success, else -errno
> > + */
> > +static int binder_defer_copy(struct list_head *sgc_head, binder_size_t offset,
> > + const void __user *uaddr, size_t length)
> > +{
> > + struct binder_sg_copy *bc = kzalloc(sizeof(*bc), GFP_KERNEL);
> > +
> > + if (!bc)
> > + return -ENOMEM;
> > +
> > + bc->offset = offset;
> > + bc->uaddr = uaddr;
> > + bc->length = length;
> > + INIT_LIST_HEAD(&bc->node);
> > +
> > + /*
> > + * We are guaranteed that the deferred copies are in-order
> > + * so just add to the tail.
> > + */
> > + list_add_tail(&bc->node, sgc_head);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * binder_add_fixup() - queue a fixup to be applied to sg copy
> > + * @pf_head: list_head of binder ptr fixup list
> > + * @offset: binder buffer offset in target process
> > + * @fixup: bytes to be copied for fixup
> > + * @skip_size: bytes to skip when copying (fixup will be applied later)
> > + *
> > + * Add the specified fixup to a list ordered by @offset. When copying
> > + * the scatter-gather buffers, the fixup will be copied instead of
> > + * data from the source buffer. For BINDER_TYPE_FDA fixups, the fixup
> > + * will be applied later (in target process context), so we just skip
> > + * the bytes specified by @skip_size. If @skip_size is 0, we copy the
> > + * value in @fixup.
> > + *
> > + * This function is called *mostly* in @offset order, but there are
> > + * exceptions. Since out-of-order inserts are relatively uncommon,
> > + * we insert the new element by searching backward from the tail of
> > + * the list.
> > + *
> > + * Return: 0=success, else -errno
> > + */
> > +static int binder_add_fixup(struct list_head *pf_head, binder_size_t offset,
> > + binder_uintptr_t fixup, size_t skip_size)
> > +{
> > + struct binder_ptr_fixup *pf = kzalloc(sizeof(*pf), GFP_KERNEL);
> > + struct list_head *tmp;
> > +
> > + if (!pf)
> > + return -ENOMEM;
> > +
> > + pf->offset = offset;
> > + pf->fixup_data = fixup;
> > + pf->skip_size = skip_size;
> > + INIT_LIST_HEAD(&pf->node);
> > +
> > + /* Fixups are *mostly* added in-order, but there are some
> > + * exceptions. Look backwards through list for insertion point.
> > + */
> > + if (!list_empty(pf_head)) {
>
> This condition is not required. If list is empty we add it to the tail,
> but when there is only one item on the list, the first and last item are
> going to be the same.
Good point.
>
> > + list_for_each_prev(tmp, pf_head) {
> > + struct binder_ptr_fixup *tmppf =
> > + list_entry(tmp, struct binder_ptr_fixup, node);
> > +
> > + if (tmppf->offset < pf->offset) {
> > + list_add(&pf->node, tmp);
> > + return 0;
> > + }
> > + }
> > + /*
> > + * if we get here, then the new offset is the lowest so
> > + * insert at the head
> > + */
> > + list_add(&pf->node, pf_head);
> > + return 0;
> > + }
> > + list_add_tail(&pf->node, pf_head);
> > + return 0;
> > +}
> > +
>
> regards,
> dan carpenter
Dan, Thanks for the detailed review on all 3 patches.
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx.
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
