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

On 08/24, Keke Ming wrote:

On 08/24, Oleg Nesterov wrote:

quoted

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

quoted

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:

quoted

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.