Re: [PATCH stable 5.10.y] proc: fix a dentry lock race between release_task and lookup

[Greg KH]

On Tue, Sep 19, 2023 at 04:33:35PM -0700, Suraj Jitindar Singh wrote:
> From: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> 
> commit d919a1e79bac890421537cf02ae773007bf55e6b upstream.
> 
> 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
> 
> Function d_invalidate() will remove dentry from hash firstly, but why does
> proc_flush_pid() process dentry '/proc/tgid/fd' before dentry
> '/proc/tgid'?  That's because proc_pid_make_inode() adds proc inode in
> reverse order by invoking hlist_add_head_rcu().  But proc should not add
> any inodes under '/proc/tgid' except '/proc/tgid/task/pid', fix it by
> adding inode into 'pid->inodes' only if the inode is /proc/tgid or
> /proc/tgid/task/pid.
> 
> Performance regression:
> Create 200 tasks, each task open one file for 50,000 times. Kill all
> tasks when opened files exceed 10,000,000 (cat /proc/sys/fs/file-nr).
> 
> Before fix:
> $ time killall -wq aa
>   real    4m40.946s   # During this period, we can see 'ps' and 'systemd'
> 			taking high cpu usage.
> 
> After fix:
> $ time killall -wq aa
>   real    1m20.732s   # During this period, we can see 'systemd' taking
> 			high cpu usage.
> 
> Link: https://lkml.kernel.org/r/20220713130029.4133533-1-chengzhihao1@xxxxxxxxxx
> 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@xxxxxxxxxx>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> Suggested-by: Brian Foster <bfoster@xxxxxxxxxx>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> Cc: Eric Biederman <ebiederm@xxxxxxxxxxxx>
> Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Cc: Baoquan He <bhe@xxxxxxxxxx>
> Cc: Kalesh Singh <kaleshsingh@xxxxxxxxxx>
> Cc: Yu Kuai <yukuai3@xxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> [ bp: Context adjustments ]
> Signed-off-by: Suraj Jitindar Singh <surajjs@xxxxxxxxxx>

both now queued up, thanks.

greg k-h