On Thu, Aug 01, 2024 at 02:00:18PM +0200, Oleg Nesterov wrote:
> On 08/01, Jiri Olsa wrote:
> >
> > > @@ -474,10 +477,10 @@ static void testmod_unregister_uprobe(void)
> > > {
> > > mutex_lock(&testmod_uprobe_mutex);
> > >
> > > - if (uprobe.offset) {
> > > - uprobe_unregister(d_real_inode(uprobe.path.dentry),
> > > - uprobe.offset, &uprobe.consumer);
> > > + if (uprobe.uprobe) {
> > > + uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
> > > uprobe.offset = 0;
> > > + uprobe.uprobe = NULL;
> >
> > ugh, I think we leak &uprobe.path.. I can send follow up fix if needed
>
> Yeah, with or without this change. And with this change we do not need
> uprobe.offset.
>
> Please see the patch below, this is what I've added to 5/5.
>
> Do you see any problems?
looks good, thanks!
>
> Note the additional path_put() in testmod_unregister_uprobe(). Does it need
> a separate patch or can it come with 5/5 ?
I think it'd be better to have it separately, the test is already
released.. so people might want to backport just the fix
thanks,
jirka
>
> Oleg.
>
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -432,7 +432,7 @@ uprobe_ret_handler(struct uprobe_consumer *self, unsigned
> long func,
>
> struct testmod_uprobe {
> struct path path;
> - loff_t offset;
> + struct uprobe *uprobe;
> struct uprobe_consumer consumer;
> };
>
> @@ -446,25 +446,25 @@ static int testmod_register_uprobe(loff_t offset)
> {
> int err = -EBUSY;
>
> - if (uprobe.offset)
> + if (uprobe.uprobe)
> return -EBUSY;
>
> mutex_lock(&testmod_uprobe_mutex);
>
> - if (uprobe.offset)
> + if (uprobe.uprobe)
> goto out;
>
> err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
> if (err)
> goto out;
>
> - err = uprobe_register(d_real_inode(uprobe.path.dentry),
> - offset, 0, &uprobe.consumer);
> - if (err)
> + uprobe.uprobe = uprobe_register(d_real_inode(uprobe.path.dentry),
> + offset, 0, &uprobe.consumer);
> + if (IS_ERR(uprobe.uprobe)) {
> + err = PTR_ERR(uprobe.uprobe);
> path_put(&uprobe.path);
> - else
> - uprobe.offset = offset;
> -
> + uprobe.uprobe = NULL;
> + }
> out:
> mutex_unlock(&testmod_uprobe_mutex);
> return err;
> @@ -474,10 +474,10 @@ static void testmod_unregister_uprobe(void)
> {
> mutex_lock(&testmod_uprobe_mutex);
>
> - if (uprobe.offset) {
> - uprobe_unregister(d_real_inode(uprobe.path.dentry),
> - uprobe.offset, &uprobe.consumer);
> - uprobe.offset = 0;
> + if (uprobe.uprobe) {
> + uprobe_unregister(uprobe.uprobe, &uprobe.consumer);
> + path_put(&uprobe.path);
> + uprobe.uprobe = NULL;
> }
>
> mutex_unlock(&testmod_uprobe_mutex);
>
