lkml.org 
[lkml]   [2022]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4] proc: Fix a dentry lock race between release_task and lookup
From
Date
在 2022/7/12 22:16, Brian Foster 写道:
> On Wed, Jun 01, 2022 at 02:23:32PM +0800, Zhihao Cheng wrote:
>> Commit 7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc")
>> moved proc_flush_task() behind __exit_signal(). Then, process systemd
>> can take long period high cpu usage during releasing task in following
>> concurrent processes:
>>
>> systemd ps
>> kernel_waitid stat(/proc/tgid)
>> do_wait filename_lookup
>> wait_consider_task lookup_fast
>> release_task
>> __exit_signal
>> __unhash_process
>> detach_pid
>> __change_pid // remove task->pid_links
>> d_revalidate -> pid_revalidate // 0
>> d_invalidate(/proc/tgid)
>> shrink_dcache_parent(/proc/tgid)
>> d_walk(/proc/tgid)
>> spin_lock_nested(/proc/tgid/fd)
>> // iterating opened fd
>> proc_flush_pid |
>> d_invalidate (/proc/tgid/fd) |
>> shrink_dcache_parent(/proc/tgid/fd) |
>> shrink_dentry_list(subdirs) ↓
>> shrink_lock_dentry(/proc/tgid/fd) --> race on dentry lock
>>
>
> Curious... can this same sort of thing happen with /proc/<tgid>/task if
> that dir similarly has a lot of dentries?
>

Yes. It could happend too. There will be many dentries under
/proc/<tgid>/task when there are many tasks under same thread group.

We must put /proc/<tgid>/task into pid->inodes, because we have to
handle single thread exiting situation: Any one of threads should
invalidate its /proc/<tgid>/task/<pid> dentry before begin released. You
may refer to the function proc_flush_task_mnt() before commit
7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc").

> ...
>> Fixes: 7bc3e6e55acf06 ("proc: Use a list of inodes to flush from proc")
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216054
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>> v1->v2: Add new helper proc_pid_make_base_inode that performs the extra
>> work of adding to the pid->list.
>> v2->v3: Add performance regression in commit message.
>> v3->v4: Make proc_pid_make_base_inode() static
>> fs/proc/base.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index c1031843cc6a..d884933950fd 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
> ...
>> @@ -1931,6 +1926,27 @@ struct inode *proc_pid_make_inode(struct super_block * sb,
>> return NULL;
>> }
>>
>> +static struct inode *proc_pid_make_base_inode(struct super_block *sb,
>> + struct task_struct *task, umode_t mode)
>> +{
>> + struct inode *inode;
>> + struct proc_inode *ei;
>> + struct pid *pid;
>> +
>> + inode = proc_pid_make_inode(sb, task, mode);
>> + if (!inode)
>> + return NULL;
>> +
>> + /* Let proc_flush_pid find this directory inode */
>> + ei = PROC_I(inode);
>> + pid = ei->pid;
>> + spin_lock(&pid->lock);
>> + hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
>> + spin_unlock(&pid->lock);
>> +
>> + return inode;
>> +}
>> +
>
> Somewhat related to the question above.. it would be nice if this
> wrapper had a line or two comment above it that explained when it should
> or shouldn't be used over the underlying function (for example, why or
> why not include /proc/<tgid>/task?). Otherwise the patch overall seems
> reasonable to me..
>

Thanks for advice, I will add some notes in v5.
> Brian
>
>> int pid_getattr(struct user_namespace *mnt_userns, const struct path *path,
>> struct kstat *stat, u32 request_mask, unsigned int query_flags)
>> {
>> @@ -3350,7 +3366,8 @@ static struct dentry *proc_pid_instantiate(struct dentry * dentry,
>> {
>> struct inode *inode;
>>
>> - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
>> + inode = proc_pid_make_base_inode(dentry->d_sb, task,
>> + S_IFDIR | S_IRUGO | S_IXUGO);
>> if (!inode)
>> return ERR_PTR(-ENOENT);
>>
>> @@ -3649,7 +3666,8 @@ static struct dentry *proc_task_instantiate(struct dentry *dentry,
>> struct task_struct *task, const void *ptr)
>> {
>> struct inode *inode;
>> - inode = proc_pid_make_inode(dentry->d_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
>> + inode = proc_pid_make_base_inode(dentry->d_sb, task,
>> + S_IFDIR | S_IRUGO | S_IXUGO);
>> if (!inode)
>> return ERR_PTR(-ENOENT);
>>
>> --
>> 2.31.1
>>
>
> .
>

\
 
 \ /
  Last update: 2022-07-13 09:26    [W:0.148 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site