On Mon, Jun 24, 2024 at 05:21:38PM -0700, Andrii Nakryiko wrote:
SNIP
> + for (i = 0; i < cnt; i++) {
> + uc = get_uprobe_consumer(i, ctx);
> +
> + /* Each consumer must have at least one set consumer */
> + if (!uc || (!uc->handler && !uc->ret_handler))
> + return -EINVAL;
> + /* Racy, just to catch the obvious mistakes */
> + if (uc->offset > i_size_read(inode))
> + return -EINVAL;
> + if (uc->uprobe)
> + return -EINVAL;
> + /*
> + * This ensures that copy_from_page(), copy_to_page() and
> + * __update_ref_ctr() can't cross page boundary.
> + */
> + if (!IS_ALIGNED(uc->offset, UPROBE_SWBP_INSN_SIZE))
> + return -EINVAL;
> + if (!IS_ALIGNED(uc->ref_ctr_offset, sizeof(short)))
> + return -EINVAL;
> + }
>
> - down_write(&uprobe->register_rwsem);
> - consumer_add(uprobe, uc);
> - ret = register_for_each_vma(uprobe, uc);
> - if (ret)
> - __uprobe_unregister(uprobe, uc);
> - up_write(&uprobe->register_rwsem);
> + for (i = 0; i < cnt; i++) {
> + uc = get_uprobe_consumer(i, ctx);
>
> - if (ret)
> - put_uprobe(uprobe);
> + uprobe = alloc_uprobe(inode, uc->offset, uc->ref_ctr_offset);
> + if (IS_ERR(uprobe)) {
> + ret = PTR_ERR(uprobe);
> + goto cleanup_uprobes;
> + }
> +
> + uc->uprobe = uprobe;
> + }
> +
> + for (i = 0; i < cnt; i++) {
> + uc = get_uprobe_consumer(i, ctx);
> + uprobe = uc->uprobe;
> +
> + down_write(&uprobe->register_rwsem);
> + consumer_add(uprobe, uc);
> + ret = register_for_each_vma(uprobe, uc);
> + if (ret)
> + __uprobe_unregister(uprobe, uc);
> + up_write(&uprobe->register_rwsem);
> +
> + if (ret) {
> + put_uprobe(uprobe);
> + goto cleanup_unreg;
> + }
> + }
> +
> + return 0;
>
> +cleanup_unreg:
> + /* unregister all uprobes we managed to register until failure */
> + for (i--; i >= 0; i--) {
> + uc = get_uprobe_consumer(i, ctx);
> +
> + down_write(&uprobe->register_rwsem);
> + __uprobe_unregister(uc->uprobe, uc);
> + up_write(&uprobe->register_rwsem);
> + }
> +cleanup_uprobes:
when we jump here from 'goto cleanup_uprobes' not all of the
consumers might have uc->uprobe set up
perhaps we can set cnt = i - 1 before the goto, or just check uc->uprobe below
> + /* put all the successfully allocated/reused uprobes */
> + for (i = cnt - 1; i >= 0; i--) {
curious, any reason why we go from the top here?
thanks,
jirka
> + uc = get_uprobe_consumer(i, ctx);
> +
> + put_uprobe(uc->uprobe);
> + uc->uprobe = NULL;
> + }
> return ret;
> }
>
> int uprobe_register(struct inode *inode, struct uprobe_consumer *uc)
> {
> - return __uprobe_register(inode, uc->offset, uc->ref_ctr_offset, uc);
> + return uprobe_register_batch(inode, 1, uprobe_consumer_identity, uc);
> }
> EXPORT_SYMBOL_GPL(uprobe_register);
>
> --
> 2.43.0
>
