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.
Signed-off-by: Wen Yang <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Alexey Dobriyan <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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)
--
1.8.3.1
Wen Yang <[email protected]> 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 <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: "Eric W. Biederman" <[email protected]>
> Cc: Alexey Dobriyan <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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)
On 11/30, Eric W. Biederman wrote:
>
> 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.
Sorry, I don't understand...
Do you mean that "ls /proc/pid/task" can see an empty dir? Afaics this
was possible before d855a4b79f49 too.
Or what?
Oleg.
Oleg Nesterov <[email protected]> writes:
> On 11/30, Eric W. Biederman wrote:
>>
>> 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.
>
> Sorry, I don't understand...
>
> Do you mean that "ls /proc/pid/task" can see an empty dir? Afaics this
> was possible before d855a4b79f49 too.
>
> Or what?
Bah. Brain fart on my part.
I read 7d8952440f40 too fast. I thought it was attempting to make
it so that "ls /proc/tid/task/" would see an empty dir while "ls
/proc/tgid/task/" would see the complete set of threads.
Where tgid is the pid of the thread group leader and tid
is the pid of some thread in the thread group.
But 7d8952440f40 was just attempting to ensure that no thread was
listed more than once in "/proc/xxx/task".
My apologies for the confusion.
Eric