linux-kernel - Re: [PATCH v2] vmscan: Do not free nr_deferred for memcg aware shrinkers

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Jul 2022 13:43:13 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     tujinjiang@...edance.com
Cc:     shy828301@...il.com, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] vmscan: Do not free nr_deferred for memcg aware
 shrinkers

On Thu 28-07-22 17:13:41, tujinjiang@...edance.com wrote:
> From: Jinjiang Tu <tujinjiang@...edance.com>
> 
> When a memcg aware shrinker is registered by register_shrinker(),
> shrinker->nr_deferred will not be initialized. But when the shrinker
> is unregistered by unregister_shrinker(), shrinker->nr_deferred
> will be freed.
> 
> Luckily, the memcg aware shrinkers in the current kernel are pre-zeroed.
> But a new memcg aware shrinker may be added in the future, and we should
> not assume the shrinker is pre-zeroed.
> 
> Another unregister API free_prealloced_shrinker() does not assume the
> shrinker is pre-zered and free shrinker->nr_deferred only if it is
> not memcg aware. So unregister_shrinker() should do like
> free_prealloced_shrinker().
> 
> Signed-off-by: Jinjiang Tu <tujinjiang@...edance.com>
> ---
>  mm/vmscan.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f7d9a683e3a7..f8a9a5349b6e 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -675,8 +675,11 @@ void unregister_shrinker(struct shrinker *shrinker)
>  	down_write(&shrinker_rwsem);
>  	list_del(&shrinker->list);
>  	shrinker->flags &= ~SHRINKER_REGISTERED;
> -	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> +	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
>  		unregister_memcg_shrinker(shrinker);
> +		up_write(&shrinker_rwsem);
> +		return;
> +	}
>  	up_write(&shrinker_rwsem);
>  
>  	kfree(shrinker->nr_deferred);

Can we get rid of the code duplication?
--- 
diff --git a/mm/vmscan.c b/mm/vmscan.c
index f7d9a683e3a7..308279414fe8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -632,12 +632,10 @@ int prealloc_shrinker(struct shrinker *shrinker)
 	return 0;
 }
 
-void free_prealloced_shrinker(struct shrinker *shrinker)
+static void __free_shrinker(struct shrinker *shrinker)
 {
 	if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
-		down_write(&shrinker_rwsem);
 		unregister_memcg_shrinker(shrinker);
-		up_write(&shrinker_rwsem);
 		return;
 	}
 
@@ -645,6 +643,13 @@ void free_prealloced_shrinker(struct shrinker *shrinker)
 	shrinker->nr_deferred = NULL;
 }
 
+void free_prealloced_shrinker(struct shrinker *shrinker)
+{
+	down_write(&shrinker_rwsem);
+	__free_shrinker(shrinker);
+	up_write(&shrinker_rwsem);
+}
+
 void register_shrinker_prepared(struct shrinker *shrinker)
 {
 	down_write(&shrinker_rwsem);
@@ -675,12 +680,9 @@ void unregister_shrinker(struct shrinker *shrinker)
 	down_write(&shrinker_rwsem);
 	list_del(&shrinker->list);
 	shrinker->flags &= ~SHRINKER_REGISTERED;
-	if (shrinker->flags & SHRINKER_MEMCG_AWARE)
-		unregister_memcg_shrinker(shrinker);
-	up_write(&shrinker_rwsem);
 
-	kfree(shrinker->nr_deferred);
-	shrinker->nr_deferred = NULL;
+	__free_shrinker(shrinker);
+	up_write(&shrinker_rwsem);
 }
 EXPORT_SYMBOL(unregister_shrinker);
 
-- 
Michal Hocko
SUSE Labs