Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Mar 2021 09:56:42 +0100
From:   Oleg Nesterov <oleg@redhat.com>
To:     Jim Newsome <jnewsome@torproject.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Christian Brauner <christian@brauner.io>,
        linux-kernel@vger.kernel.org
Subject: Re: patch: do_wait: make PIDTYPE_PID case O(1) instead of O(n)
Message-ID: <20210309085641.GB25222@redhat.com>
References: <b8591e67-1248-6c29-847e-d97e04b58210@torproject.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b8591e67-1248-6c29-847e-d97e04b58210@torproject.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Ah, and you forgot to CC lkml ;) let me resend my email.


Hi Jim,

Please do not use the attachments, just send the patch as plain text.
See Documentation/process/submitting-patches.rst

On 03/08, Jim Newsome wrote:
>
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1462,8 +1462,61 @@ static long do_wait(struct wait_opts *wo)
>  		goto notask;
>
>  	set_current_state(TASK_INTERRUPTIBLE);
> +
>  	read_lock(&tasklist_lock);
>  	tsk = current;
> +
> +	if (wo->wo_type == PIDTYPE_PID) {
> +		// Optimization for PIDTYPE_PID. No need to iterate through child and
> +		// tracee lists to find the target task.

I'd suggest to put this PIDTYPE_PID code into the new function.

> +
> +		struct task_struct *real_parent = NULL;
> +		struct task_struct *target = NULL;
> +		bool do_regular_wait, do_ptrace_wait;
> +
> +		// XXX: Do we need this? Or is the tasklist_lock sufficient?
> +		rcu_read_lock();

No, you don't need rcu lock, tasklist_lock is sufficient

> +		target = pid_task(wo->wo_pid, PIDTYPE_PID);
> +		if (!target) {
> +			rcu_read_unlock();
> +			goto notask;

This is wrong, you forgot to drop tasklist_lock.


> +		real_parent = !target->real_parent ? target->parent :
> +						     target->real_parent;

Hmm, I don't understand the line above... perhaps it connects to the
question below.

> +		if (!real_parent) {
> +			// XXX: Is it a kernel bug to get here? Or would this be
> +			// true of the init process?

Afaics, parent/real_parent can't be NULL if pid_task() succeeds.

> +		do_regular_wait = tsk == real_parent ||
> +				  (!(wo->wo_flags & __WNOTHREAD) &&
> +				   same_thread_group(tsk, real_parent));
> +		do_ptrace_wait = target->ptrace &&
> +				 (tsk == target->parent ||
> +				  (!(wo->wo_flags & __WNOTHREAD) &&
> +				   same_thread_group(tsk, target->parent)));
> +		rcu_read_unlock();
> +
> +		if (do_regular_wait) {
> +			retval =
> +				wait_consider_task(wo, /* ptrace= */ 0, target);
> +			if (retval) {
> +				goto end;
> +			}
> +		}
> +		if (do_ptrace_wait) {
> +			retval =
> +				wait_consider_task(wo, /* ptrace= */ 1, target);
> +			if (retval) {
> +				goto end;
> +			}
> +		}
> +		read_unlock(&tasklist_lock);
> +		goto notask;

This part looks correct at first glance...

Please redo and send V2 ;)

Oleg.