On Wed, Jun 26, 2024 at 4:27 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> 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
yep, you are right, I missed this part during multiple rounds of
refactorings. I think the `if (uc->uprobe)` check is the cleanest
approach here.
>
>
> > + /* put all the successfully allocated/reused uprobes */
> > + for (i = cnt - 1; i >= 0; i--) {
>
> curious, any reason why we go from the top here?
No particular reason. This started as (i = i - 1; i >= 0; i--), but
then as I kept splitting steps I needed to do this over all uprobes.
Anyways, I can do a clean `i = 0; i < cnt; i++` with `if (uc->uprobe)`
check.
>
> 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
> >
