linux-kernel - Re: [PATCH stable 5.10.y] proc: fix a dentry lock race between release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2023 13:11:57 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Suraj Jitindar Singh <surajjs@...zon.com>
Cc:     stable@...r.kernel.org, linux-kernel@...r.kernel.org,
        adobriyan@...il.com, sjitindarsingh@...il.com,
        Zhihao Cheng <chengzhihao1@...wei.com>,
        Zhang Yi <yi.zhang@...wei.com>,
        Brian Foster <bfoster@...hat.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Eric Biederman <ebiederm@...ssion.com>,
        Matthew Wilcox <willy@...radead.org>,
        Baoquan He <bhe@...hat.com>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Yu Kuai <yukuai3@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH stable 5.10.y] proc: fix a dentry lock race between
 release_task and lookup

On Tue, Sep 19, 2023 at 04:33:35PM -0700, Suraj Jitindar Singh wrote:
> From: Zhihao Cheng <chengzhihao1@...wei.com>
> 
> 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@huawei.com
> 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@...wei.com>
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> Suggested-by: Brian Foster <bfoster@...hat.com>
> Reviewed-by: Brian Foster <bfoster@...hat.com>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Alexey Dobriyan <adobriyan@...il.com>
> Cc: Eric Biederman <ebiederm@...ssion.com>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Baoquan He <bhe@...hat.com>
> Cc: Kalesh Singh <kaleshsingh@...gle.com>
> Cc: Yu Kuai <yukuai3@...wei.com>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> [ bp: Context adjustments ]
> Signed-off-by: Suraj Jitindar Singh <surajjs@...zon.com>

both now queued up, thanks.

greg k-h