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;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Wen Yang <wenyang@linux.alibaba.com>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        Christian Brauner <christian@brauner.io>,
        Oleg Nesterov <oleg@redhat.com>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <20201128175850.19484-1-wenyang@linux.alibaba.com>
Date:   Mon, 30 Nov 2020 12:41:33 -0600
In-Reply-To: <20201128175850.19484-1-wenyang@linux.alibaba.com> (Wen Yang's
        message of "Sun, 29 Nov 2020 01:58:50 +0800")
Message-ID: <87zh2yit5u.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] proc: add locking checks in proc_inode_is_dead
Precedence: bulk

Wen Yang <wenyang@linux.alibaba.com> writes:

> The proc_inode_is_dead function might race with __unhash_process.
> This will result in a whole bunch of stale proc entries being cached.
> To prevent that, add the required locking.

I assume you are talking about during proc_task_readdir?

It is completely possible for the proc_inode_is_dead to report
the inode is still alive and then for unhash_process to
happen when afterwards.

Have you been able to trigger this race in practice?


Ouch!!!!  Oleg I just looked the introduction of proc_inode_is_dead in
d855a4b79f49 ("proc: don't (ab)use ->group_leader in proc_task_readdir()
paths") introduced a ``regression''.

Breaking the logic introduced in 7d8952440f40 ("[PATCH] procfs: Fix
listing of /proc/NOT_A_TGID/task") to keep those directory listings not
showing up.

Given that it has been 6 years and no one has cared it doesn't look like
an actual regression but it does suggest the proc_inode_is_dead can be
removed entirely as it does not achieve anything in proc_task_readdir.


As for removing the race.  I expect the thing to do is to modify
proc_pid_is_alive to verify the that the pid is still alive.
Oh but look get_task_pid verifies that thread_pid is not NULL
and unhash_process sets thread_pid to NULL.


My brain is too fuzzy right now to tell if it is possible for
get_task_pid to return a pid and then for that pid to pass through
unhash_process, while the code is still in proc_pid_make_inode.

proc_inode_is_dead is definitely not the place to look to close races.

Eric


> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Christian Brauner <christian@brauner.io>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/proc/base.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 1bc9bcd..59720bc 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1994,7 +1994,13 @@ static int pid_revalidate(struct dentry *dentry, unsigned int flags)
>  
>  static inline bool proc_inode_is_dead(struct inode *inode)
>  {
> -	return !proc_pid(inode)->tasks[PIDTYPE_PID].first;
> +	bool has_task;
> +
> +	read_lock(&tasklist_lock);
> +	has_task = pid_has_task(proc_pid(inode), PIDTYPE_PID);
> +	read_unlock(&tasklist_lock);
> +
> +	return !has_task;
>  }
>  
>  int pid_delete_dentry(const struct dentry *dentry)