[PATCH] uprobes: Free utask on dup_return_instance() failure

[PATCH] uprobes: Free utask on dup_return_instance() failure

[Keke Ming]

dup_utask() installs the new uprobe_task in t->utask before copying
return_instances. If dup_return_instance() fails, the partially copied
utask is left attached to the child task.

Free the partially copied utask before returning -ENOMEM.

Signed-off-by: Keke Ming <ming.jvle@gmail.com>
---
 kernel/events/uprobes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b25531331902..0c8a664a0fe5 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
 	p = &n_utask->return_instances;
 	for (o = o_utask->return_instances; o; o = o->next) {
 		n = dup_return_instance(o);
-		if (!n)
+		if (!n) {
+			uprobe_free_utask(t);
 			return -ENOMEM;
+		}
 
 		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
 		uprobe = hprobe_expire(&o->hprobe, true);
-- 
2.43.0

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free of `extra_consumers` during `fork()` due to missing pointer nullification in `dup_return_instance()` / `ri_pool_push()`.
--

--- Patch [1]: [PATCH] uprobes: Free utask on dup_return_instance() failure ---
commit 52bd4caeb5b9686b25fe707609034e9397c6100a
Author: Keke Ming <ming.jvle@gmail.com>

uprobes: Free utask on dup_return_instance() failure

dup_utask() installs the new uprobe_task in t->utask before copying
return_instances. If dup_return_instance() fails, the partially copied
utask is left attached to the child task. This patch frees the partially
copied utask before returning -ENOMEM.

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7709ea8824778..7547929ab294d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
>  	p = &n_utask->return_instances;
>  	for (o = o_utask->return_instances; o; o = o->next) {
>  		n = dup_return_instance(o);
> -		if (!n)
> +		if (!n) {
> +			uprobe_free_utask(t);

[Severity: Critical]
This is a pre-existing issue, but can dup_return_instance() leave a dangling
pointer that results in a double free of extra_consumers when
uprobe_free_utask() is called here?

If a multi-consumer uretprobe is hit, extra_consumers is allocated. When this
returns, it is pushed to the reuse pool:

kernel/events/uprobes.c:ri_pool_push() {
    ri->cons_cnt = 0;
    ri->next = utask->ri_pool;
    utask->ri_pool = ri;
}

This resets cons_cnt to 0 but retains the extra_consumers heap pointer.

If the thread then hits a single-consumer uretprobe, alloc_return_instance()
reuses the pooled object. push_consumer() increments cons_cnt to 1 and uses
the inline consumer, leaving the old extra_consumers pointer untouched.

When the task calls fork(), dup_return_instance() does a shallow copy:

kernel/events/uprobes.c:dup_return_instance() {
    ri = kmemdup(old, sizeof(*ri), GFP_KERNEL);
    if (!ri)
        return NULL;

    if (unlikely(old->cons_cnt > 1)) {
        ri->extra_consumers = kmemdup(old->extra_consumers, ...
    ...
}

Since old->cons_cnt == 1, it skips deep-copying or clearing extra_consumers.
Both the parent and child tasks now share the exact same heap pointer. 

When the tasks exit, or if dup_return_instance() fails and triggers the new
cleanup path in this patch, ri_free() will execute kfree(ri->extra_consumers)
in both contexts, freeing the same memory twice.

>  			return -ENOMEM;
> +		}
>  
>  		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
>  		uprobe = hprobe_expire(&o->hprobe, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822054620.452262-1-ming.jvle@gmail.com?part=1

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Masami Hiramatsu]

On Sat, 22 Aug 2026 06:00:14 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Double free of `extra_consumers` during `fork()` due to missing pointer nullification in `dup_return_instance()` / `ri_pool_push()`.
> --
> 
> --- Patch [1]: [PATCH] uprobes: Free utask on dup_return_instance() failure ---
> commit 52bd4caeb5b9686b25fe707609034e9397c6100a
> Author: Keke Ming <ming.jvle@gmail.com>
> 
> uprobes: Free utask on dup_return_instance() failure
> 
> dup_utask() installs the new uprobe_task in t->utask before copying
> return_instances. If dup_return_instance() fails, the partially copied
> utask is left attached to the child task. This patch frees the partially
> copied utask before returning -ENOMEM.
> 
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index 7709ea8824778..7547929ab294d 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >  	p = &n_utask->return_instances;
> >  	for (o = o_utask->return_instances; o; o = o->next) {
> >  		n = dup_return_instance(o);
> > -		if (!n)
> > +		if (!n) {
> > +			uprobe_free_utask(t);
> 
> [Severity: Critical]
> This is a pre-existing issue, but can dup_return_instance() leave a dangling
> pointer that results in a double free of extra_consumers when
> uprobe_free_utask() is called here?

Hmm, this seems another bug here.

> 
> If a multi-consumer uretprobe is hit, extra_consumers is allocated. When this
> returns, it is pushed to the reuse pool:
> 
> kernel/events/uprobes.c:ri_pool_push() {
>     ri->cons_cnt = 0;
>     ri->next = utask->ri_pool;
>     utask->ri_pool = ri;
> }
> 
> This resets cons_cnt to 0 but retains the extra_consumers heap pointer.
> 
> If the thread then hits a single-consumer uretprobe, alloc_return_instance()
> reuses the pooled object. push_consumer() increments cons_cnt to 1 and uses
> the inline consumer, leaving the old extra_consumers pointer untouched.
> 
> When the task calls fork(), dup_return_instance() does a shallow copy:
> 
> kernel/events/uprobes.c:dup_return_instance() {
>     ri = kmemdup(old, sizeof(*ri), GFP_KERNEL);
>     if (!ri)
>         return NULL;
> 
>     if (unlikely(old->cons_cnt > 1)) {
>         ri->extra_consumers = kmemdup(old->extra_consumers, ...
>     ...
> }
> 
> Since old->cons_cnt == 1, it skips deep-copying or clearing extra_consumers.
> Both the parent and child tasks now share the exact same heap pointer. 

Hmm, so either clearing extra_consumers when returning ri to the pool
(with releasing extra_consumers) or clearing new ri->extra_consumers when
dup_return_instance(), will fix this.

Thanks,

> 
> When the tasks exit, or if dup_return_instance() fails and triggers the new
> cleanup path in this patch, ri_free() will execute kfree(ri->extra_consumers)
> in both contexts, freeing the same memory twice.
> 
> >  			return -ENOMEM;
> > +		}
> >  
> >  		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
> >  		uprobe = hprobe_expire(&o->hprobe, true);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260822054620.452262-1-ming.jvle@gmail.com?part=1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Keke Ming]

Thanks, agreed.

> Hmm, this seems another bug here.

I chose method 2 (clearing ri->extra_consumers when dup_return_instance())
in this patch set. Please let me know if you prefer the first approach instead.

[PATCH v2] uprobes: Free utask on dup_return_instance() failure

[Keke Ming]

dup_utask() installs the new uprobe_task in t->utask before copying
return_instances. If dup_return_instance() fails, the partially copied
utask is left attached to the child task.

Free the partially copied utask before returning -ENOMEM.

Also, dup_return_instance() copies the return_instance before fixing
up extra_consumers.

Here, if no deep copy is needed, clear the copied extra_consumers
pointer.

Signed-off-by: Keke Ming <ming.jvle@gmail.com>
---
v2 changes:
- Clear ri->extra_consumers = NULL when old->cons_cnt <= 1 in
  dup_return_instance(), fixing a potential double free during
  cleanup.
- Keep the dup_utask() error cleanup from v1.

 kernel/events/uprobes.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b25531331902..73a6620c5701 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2118,6 +2118,8 @@ static struct return_instance *dup_return_instance(struct return_instance *old)
 			kfree(ri);
 			return NULL;
 		}
+	} else {
+		ri->extra_consumers = NULL;
 	}
 
 	return ri;
@@ -2140,8 +2142,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
 	p = &n_utask->return_instances;
 	for (o = o_utask->return_instances; o; o = o->next) {
 		n = dup_return_instance(o);
-		if (!n)
+		if (!n) {
+			uprobe_free_utask(t);
 			return -ENOMEM;
+		}
 
 		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
 		uprobe = hprobe_expire(&o->hprobe, true);
-- 
2.43.0

Re: [PATCH v2] uprobes: Free utask on dup_return_instance() failure

[sashiko-bot]

> dup_utask() installs the new uprobe_task in t->utask before copying
> return_instances. If dup_return_instance() fails, the partially copied
> utask is left attached to the child task.
> 
> Free the partially copied utask before returning -ENOMEM.
> 
> Also, dup_return_instance() copies the return_instance before fixing
> up extra_consumers.
> 
> Here, if no deep copy is needed, clear the copied extra_consumers
> pointer.
> 
> Signed-off-by: Keke Ming <ming.jvle@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822150906.528208-1-ming.jvle@gmail.com?part=1

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Oleg Nesterov]

(Add Andrii and Jiri)

On 08/22, Keke Ming wrote:
>
> dup_utask() installs the new uprobe_task in t->utask before copying
> return_instances. If dup_return_instance() fails, the partially copied
> utask is left attached to the child task.

Yes,

> Free the partially copied utask before returning -ENOMEM.

But why?

Note that uprobe_copy_process() warns but returns "void", too late
to abort copy_process().

Perhaps we should change uprobe_copy_process() to kill the new child
on failure, it will likely crash anyway.

But I don't think this patch can make the things any better.

Oleg.

> Signed-off-by: Keke Ming <ming.jvle@gmail.com>
> ---
>  kernel/events/uprobes.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b25531331902..0c8a664a0fe5 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
>  	p = &n_utask->return_instances;
>  	for (o = o_utask->return_instances; o; o = o->next) {
>  		n = dup_return_instance(o);
> -		if (!n)
> +		if (!n) {
> +			uprobe_free_utask(t);
>  			return -ENOMEM;
> +		}
>
>  		/* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
>  		uprobe = hprobe_expire(&o->hprobe, true);
> --
> 2.43.0
>

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Keke Ming]

Thanks for the feedback.

> Note that uprobe_copy_process() warns but returns "void", too late
> to abort copy_process().

You are right that freeing the partial utask is not enough. The child
may still be unsafe to run after uprobe_copy_process() fails.

Would it make sense to keep the cleanup and additionally send
SIGKILL to the new child when the  uprobe state copy fails?

Also, should the existing "dup xol area" failure path be handled
the same way? At this point dup_utask() has already succeeded,
so the child has inherited the uprobe task state. However, we fail
to get the xol_area needed for the later setup, so the child's
uprobe state may still be incomplete.

Keke.

On Sun, Aug 23, 2026 at 11:38 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> (Add Andrii and Jiri)
>
> On 08/22, Keke Ming wrote:
> >
> > dup_utask() installs the new uprobe_task in t->utask before copying
> > return_instances. If dup_return_instance() fails, the partially copied
> > utask is left attached to the child task.
>
> Yes,
>
> > Free the partially copied utask before returning -ENOMEM.
>
> But why?
>
> Note that uprobe_copy_process() warns but returns "void", too late
> to abort copy_process().
>
> Perhaps we should change uprobe_copy_process() to kill the new child
> on failure, it will likely crash anyway.
>
> But I don't think this patch can make the things any better.
>
> Oleg.
>
> > Signed-off-by: Keke Ming <ming.jvle@gmail.com>
> > ---
> >  kernel/events/uprobes.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > index b25531331902..0c8a664a0fe5 100644
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct uprobe_task *o_utask)
> >       p = &n_utask->return_instances;
> >       for (o = o_utask->return_instances; o; o = o->next) {
> >               n = dup_return_instance(o);
> > -             if (!n)
> > +             if (!n) {
> > +                     uprobe_free_utask(t);
> >                       return -ENOMEM;
> > +             }
> >
> >               /* if uprobe is non-NULL, we'll have an extra refcount for uprobe */
> >               uprobe = hprobe_expire(&o->hprobe, true);
> > --
> > 2.43.0
> >
>

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Oleg Nesterov]

On 08/24, Keke Ming wrote:
>
> > Note that uprobe_copy_process() warns but returns "void", too late
> > to abort copy_process().
>
> You are right that freeing the partial utask is not enough.

Hmm. It is not that I think "it is not enough", I think this is pointless
whatever we do. Please see below.

But you know what? I am afraid I am totally confused again, this happens
more and more often. So please correct me.

> Would it make sense to keep the cleanup

But again, why do you think it makes any sense to keep the cleanup
you propose?

> and additionally send
> SIGKILL to the new child when the  uprobe state copy fails?

Or SIGILL like the rest of uprobes.c does...

Yes, this is what I meant. But this is only the first step to cleanup
this logic. I'll try to write another email tomorrow.

Lets suppose we change uprobe_copy_process() to kill the child. Then
why do we need to call uprobe_free_utask() in dup_utask() or do anything
else in copy_process() paths?

The child won't return to userspace, it will exit and call uprobe_free_utask()
itself.

> Also, should the existing "dup xol area" failure path be handled
> the same way?

Yes sure. But perhaps needs another discussion.

Oleg.

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Keke Ming]

On 08/24, Oleg Nesterov wrote:

> Hmm. It is not that I think "it is not enough", I think this is pointless
> whatever we do.

Well, I didn't mean to insist on the cleanup, I also think it may
be redundant. If the child is bound to exit, manually calling
uprobe_free_utask(t) in dup_utask() is pointless, as the normal
exit path will handle it.

> Yes, this is what I meant. But this is only the first step to cleanup
> this logic. I'll try to write another email tomorrow.

Of course, I'm all ears.

Here is my current understanding. One thing I am not sure about
is the signal semantics. If the copied uprobe state is incomplete,
the child should not return to user mode with that state.

Option A:

```
if (dup_utask(t, utask)) {
  uprobe_warn(t, "dup ret instances");
  goto kill_child;
}
...
if (!area) {
  // whether we also need to kill it is open to further discussion.
  uprobe_warn(t, "dup xol area");
  goto kill_child;
}
...
kill_child:
  // I was thinking about SIGKILL here because it cannot be ignored
  // Of course, you can correct me
  send_sig(SIGKILL, t, 1);
```

Option B:

> Note that uprobe_copy_process() warns but returns "void", too late
> to abort copy_process().

Looking at copy_process(), uprobe_copy_process() is currently called
after the "No more failure paths" point and after the child has already
been made visible through the task list and pid links.

Would the longer term fix be to move the uprobe state copy earlier,
before the "No more failure paths" point, and then add the needed
bad_fork cleanup for  p->utask?

Keke.

On Mon, Aug 24, 2026 at 3:41 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 08/24, Keke Ming wrote:
> >
> > > Note that uprobe_copy_process() warns but returns "void", too late
> > > to abort copy_process().
> >
> > You are right that freeing the partial utask is not enough.
>
> Hmm. It is not that I think "it is not enough", I think this is pointless
> whatever we do. Please see below.
>
> But you know what? I am afraid I am totally confused again, this happens
> more and more often. So please correct me.
>
> > Would it make sense to keep the cleanup
>
> But again, why do you think it makes any sense to keep the cleanup
> you propose?
>
> > and additionally send
> > SIGKILL to the new child when the  uprobe state copy fails?
>
> Or SIGILL like the rest of uprobes.c does...
>
> Yes, this is what I meant. But this is only the first step to cleanup
> this logic. I'll try to write another email tomorrow.
>
> Lets suppose we change uprobe_copy_process() to kill the child. Then
> why do we need to call uprobe_free_utask() in dup_utask() or do anything
> else in copy_process() paths?
>
> The child won't return to userspace, it will exit and call uprobe_free_utask()
> itself.
>
> > Also, should the existing "dup xol area" failure path be handled
> > the same way?
>
> Yes sure. But perhaps needs another discussion.
>
> Oleg.
>

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Oleg Nesterov]

On 08/24, Keke Ming wrote:
>
> On 08/24, Oleg Nesterov wrote:
>
> > Hmm. It is not that I think "it is not enough", I think this is pointless
> > whatever we do.
>
> Well, I didn't mean to insist on the cleanup, I also think it may
> be redundant. If the child is bound to exit, manually calling
> uprobe_free_utask(t) in dup_utask() is pointless, as the normal
> exit path will handle it.

Yes, but the same is true if the child is not bound to exit... IOW,
I still can't understand your motivation for this patch...

> > But this is only the first step to cleanup
> > this logic. I'll try to write another email tomorrow.
>
> Of course, I'm all ears.

You have already mentioned "Option B" below ;) that is what I meant.

> Here is my current understanding. One thing I am not sure about
> is the signal semantics. If the copied uprobe state is incomplete,
> the child should not return to user mode with that state.

Perhaps... But consider

	func_which_can_be_ret_probed(void)
	{
		if (!fork) {
			printf("CHILD\n");
			exit(0);
		}
	}

> Option A:
...
> ...
> kill_child:
>   // I was thinking about SIGKILL here because it cannot be ignored
>   // Of course, you can correct me
>   send_sig(SIGKILL, t, 1);

force_exit_sig(SIGILL) can't be ignored too. But I am fine either way.

So yes, perhaps this makes sense. Although IMO this all is not that
important; GFP_KERNEL shouldn't fail "in practice" and at least
uprobe_copy_process() warns in this case...

> Option B:
>
> > Note that uprobe_copy_process() warns but returns "void", too late
> > to abort copy_process().
>
> Looking at copy_process(), uprobe_copy_process() is currently called
> after the "No more failure paths" point and after the child has already
> been made visible through the task list and pid links.
>
> Would the longer term fix be to move the uprobe state copy earlier,
> before the "No more failure paths" point, and then add the needed
> bad_fork cleanup for  p->utask?

Agreed, pronably better than Option A. But in therory dup_xol_work()
can still fail after fork().

Oleg.

Re: [PATCH] uprobes: Free utask on dup_return_instance() failure

[Andrii Nakryiko]

On Mon, Aug 24, 2026 at 8:57 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 08/24, Keke Ming wrote:
> >
> > On 08/24, Oleg Nesterov wrote:
> >
> > > Hmm. It is not that I think "it is not enough", I think this is pointless
> > > whatever we do.
> >
> > Well, I didn't mean to insist on the cleanup, I also think it may
> > be redundant. If the child is bound to exit, manually calling
> > uprobe_free_utask(t) in dup_utask() is pointless, as the normal
> > exit path will handle it.
>
> Yes, but the same is true if the child is not bound to exit... IOW,
> I still can't understand your motivation for this patch...
>
> > > But this is only the first step to cleanup
> > > this logic. I'll try to write another email tomorrow.
> >
> > Of course, I'm all ears.
>
> You have already mentioned "Option B" below ;) that is what I meant.
>
> > Here is my current understanding. One thing I am not sure about
> > is the signal semantics. If the copied uprobe state is incomplete,
> > the child should not return to user mode with that state.
>
> Perhaps... But consider
>
>         func_which_can_be_ret_probed(void)
>         {
>                 if (!fork) {
>                         printf("CHILD\n");
>                         exit(0);
>                 }
>         }
>
> > Option A:
> ...
> > ...
> > kill_child:
> >   // I was thinking about SIGKILL here because it cannot be ignored
> >   // Of course, you can correct me
> >   send_sig(SIGKILL, t, 1);
>
> force_exit_sig(SIGILL) can't be ignored too. But I am fine either way.

Current SIGILL sending is already pretty bad behavior that bites us
periodically, instead of doubling down on killing the user space
process because something about installing uprobe goes wrong, let's
think about a bit less destructive way to do this. Mark uprobe for a
particular process as defunct or "detaching it", or something along
those lines. But not just kill innocent processes.

>
> So yes, perhaps this makes sense. Although IMO this all is not that
> important; GFP_KERNEL shouldn't fail "in practice" and at least
> uprobe_copy_process() warns in this case...
>
> > Option B:
> >
> > > Note that uprobe_copy_process() warns but returns "void", too late
> > > to abort copy_process().
> >
> > Looking at copy_process(), uprobe_copy_process() is currently called
> > after the "No more failure paths" point and after the child has already
> > been made visible through the task list and pid links.
> >
> > Would the longer term fix be to move the uprobe state copy earlier,
> > before the "No more failure paths" point, and then add the needed
> > bad_fork cleanup for  p->utask?
>
> Agreed, pronably better than Option A. But in therory dup_xol_work()
> can still fail after fork().
>
> Oleg.
>