[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

[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] 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.
>