Messages in this thread Patch in this message | | | From | (Eric W. Biederman) | Date | Mon, 30 Nov 2020 12:41:33 -0600 | Subject | Re: [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)
| |