lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] proc: add locking checks in proc_inode_is_dead
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)

\
 
 \ /
  Last update: 2020-11-30 19:45    [W:0.056 / U:1.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site