Date:   Fri, 3 Feb 2023 09:45:39 -0800
From:   Roman Gushchin <roman.gushchin@linux.dev>
To:     Qi Zheng <zhengqi.arch@bytedance.com>
Cc:     akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH] mm: shrinkers: fix deadlock in shrinker debugfs
Message-ID: <Y91IQ1vdaofq40JQ@P9FQF9L96D.corp.robot.car>
References: <20230202105612.64641-1-zhengqi.arch@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230202105612.64641-1-zhengqi.arch@bytedance.com>
Precedence: bulk

On Thu, Feb 02, 2023 at 06:56:12PM +0800, Qi Zheng wrote:
> The debugfs_remove_recursive() is invoked by unregister_shrinker(),
> which is holding the write lock of shrinker_rwsem. It will waits
> for the handler of debugfs file complete. The handler also needs
> to hold the read lock of shrinker_rwsem to do something. So it
> may cause the following deadlock:
> 
>  	CPU0				CPU1
> 
> debugfs_file_get()
> shrinker_debugfs_count_show()/shrinker_debugfs_scan_write()
> 
>      				unregister_shrinker()
> 				--> down_write(&shrinker_rwsem);
> 				    debugfs_remove_recursive()
> 					// wait for (A)
> 				    --> wait_for_completion();
> 
>     // wait for (B)
> --> down_read_killable(&shrinker_rwsem)
> debugfs_file_put() -- (A)
> 
> 				    up_write() -- (B)
> 
> The down_read_killable() can be killed, so that the above deadlock
> can be recovered. But it still requires an extra kill action,
> otherwise it will block all subsequent shrinker-related operations,
> so it's better to fix it.

Oh, indeed, great catch!

With Andrew's fixup:
Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>

Thank you!