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

[Oleg Nesterov <oleg@redhat.com>]

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.