[PATCH 0/3] dm-pcache: built-in support and metadata hardening

[PATCH 0/3] dm-pcache: built-in support and metadata hardening

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This three-patch series tidies dm-pcache’s build glue and tightens the metadata scan.

Patch 1 allow dm-pcache to be linked into vmlinux and avoids clashing with the sunrpc
cache_flush() by using obj-$(CONFIG_DM_PCACHE) and renaming the helper across the tree.

Patch 2 drops a redundant recomputation of the metadata slot pointer while walking headers.

Patch 3 zero-allocates a temporary buffer so callers never see stale metadata,
relies on __free(kvfree) for cleanup, and only copies back once a valid record is found.

Thanks for your review.

Li Chen (3):
  dm-pcache: allow built-in build and rename flush helper
  dm-pcache: reuse meta_addr in pcache_meta_find_latest
  dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

 drivers/md/dm-pcache/Makefile          |  2 +-
 drivers/md/dm-pcache/cache.c           |  2 +-
 drivers/md/dm-pcache/cache.h           |  2 +-
 drivers/md/dm-pcache/cache_req.c       |  6 +++---
 drivers/md/dm-pcache/pcache_internal.h | 15 ++++++++++-----
 5 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.51.0

[PATCH 1/3] dm-pcache: allow built-in build and rename flush helper

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

CONFIG_BCACHE is tristate, so dm-pcache can also be built-in.
Switch the Makefile to use obj-$(CONFIG_DM_PCACHE) so the target can be
linked into vmlinux instead of always being a loadable module.

Also rename cache_flush() to pcache_cache_flush() to avoid a global
symbol clash with sunrpc/cache.c's cache_flush().

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 drivers/md/dm-pcache/Makefile    | 2 +-
 drivers/md/dm-pcache/cache.c     | 2 +-
 drivers/md/dm-pcache/cache.h     | 2 +-
 drivers/md/dm-pcache/cache_req.c | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-pcache/Makefile b/drivers/md/dm-pcache/Makefile
index 86776e4acad24..cedfd38854f63 100644
--- a/drivers/md/dm-pcache/Makefile
+++ b/drivers/md/dm-pcache/Makefile
@@ -1,3 +1,3 @@
 dm-pcache-y := dm_pcache.o cache_dev.o segment.o backing_dev.o cache.o cache_gc.o cache_writeback.o cache_segment.o cache_key.o cache_req.o
 
-obj-m += dm-pcache.o
+obj-$(CONFIG_DM_PCACHE) += dm-pcache.o
diff --git a/drivers/md/dm-pcache/cache.c b/drivers/md/dm-pcache/cache.c
index d8e92367d9470..d516d49042272 100644
--- a/drivers/md/dm-pcache/cache.c
+++ b/drivers/md/dm-pcache/cache.c
@@ -411,7 +411,7 @@ void pcache_cache_stop(struct dm_pcache *pcache)
 {
 	struct pcache_cache *cache = &pcache->cache;
 
-	cache_flush(cache);
+	pcache_cache_flush(cache);
 
 	cancel_delayed_work_sync(&cache->gc_work);
 	flush_work(&cache->clean_work);
diff --git a/drivers/md/dm-pcache/cache.h b/drivers/md/dm-pcache/cache.h
index 1136d86958c8c..27613b56be54c 100644
--- a/drivers/md/dm-pcache/cache.h
+++ b/drivers/md/dm-pcache/cache.h
@@ -339,7 +339,7 @@ void cache_seg_put(struct pcache_cache_segment *cache_seg);
 void cache_seg_set_next_seg(struct pcache_cache_segment *cache_seg, u32 seg_id);
 
 /* cache request*/
-int cache_flush(struct pcache_cache *cache);
+int pcache_cache_flush(struct pcache_cache *cache);
 void miss_read_end_work_fn(struct work_struct *work);
 int pcache_cache_handle_req(struct pcache_cache *cache, struct pcache_request *pcache_req);
 
diff --git a/drivers/md/dm-pcache/cache_req.c b/drivers/md/dm-pcache/cache_req.c
index 27f94c1fa968c..7854a30e07b7f 100644
--- a/drivers/md/dm-pcache/cache_req.c
+++ b/drivers/md/dm-pcache/cache_req.c
@@ -790,7 +790,7 @@ static int cache_write(struct pcache_cache *cache, struct pcache_request *pcache
 }
 
 /**
- * cache_flush - Flush all ksets to persist any pending cache data
+ * pcache_cache_flush - Flush all ksets to persist any pending cache data
  * @cache: Pointer to the cache structure
  *
  * This function iterates through all ksets associated with the provided `cache`
@@ -802,7 +802,7 @@ static int cache_write(struct pcache_cache *cache, struct pcache_request *pcache
  * the respective error code, preventing the flush operation from proceeding to
  * subsequent ksets.
  */
-int cache_flush(struct pcache_cache *cache)
+int pcache_cache_flush(struct pcache_cache *cache)
 {
 	struct pcache_cache_kset *kset;
 	int ret;
@@ -827,7 +827,7 @@ int pcache_cache_handle_req(struct pcache_cache *cache, struct pcache_request *p
 	struct bio *bio = pcache_req->bio;
 
 	if (unlikely(bio->bi_opf & REQ_PREFLUSH))
-		return cache_flush(cache);
+		return pcache_cache_flush(cache);
 
 	if (bio_data_dir(bio) == READ)
 		return cache_read(cache, pcache_req);
-- 
2.51.0

[PATCH 2/3] dm-pcache: reuse meta_addr in pcache_meta_find_latest

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

pcache_meta_find_latest() already computes the metadata address as
meta_addr. Reuse that instead of recomputing.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 drivers/md/dm-pcache/pcache_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
index d427e534727ce..b7a3319d2bd3e 100644
--- a/drivers/md/dm-pcache/pcache_internal.h
+++ b/drivers/md/dm-pcache/pcache_internal.h
@@ -99,7 +99,7 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
 		/* Update latest if a more recent sequence is found */
 		if (!latest || pcache_meta_seq_after(meta->seq, seq_latest)) {
 			seq_latest = meta->seq;
-			latest = (void *)header + (i * meta_max_size);
+			latest = meta_addr;
 		}
 	}
 
-- 
2.51.0

[PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Before this change pcache_meta_find_latest() was copying each
slot directly into meta_ret while scanning. If no valid slot
was found and the function returned NULL, meta_ret still held
whatever was last copied (possibly CRC-bad). Later users
(e.g. cache_segs_init) could mistakenly trust that data.

Allocate a temporary buffer instead and only populate meta_ret after a
valid/latest header is found. If no valid header exists we return NULL
without touching meta_ret.

Also add __free(kvfree) so the temporary buffer is always freed, and
include the needed headers.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 drivers/md/dm-pcache/pcache_internal.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
index b7a3319d2bd3e..ac28f9dd2986f 100644
--- a/drivers/md/dm-pcache/pcache_internal.h
+++ b/drivers/md/dm-pcache/pcache_internal.h
@@ -4,6 +4,8 @@
 
 #include <linux/delay.h>
 #include <linux/crc32c.h>
+#include <linux/slab.h>
+#include <linux/cleanup.h>
 
 #define pcache_err(fmt, ...)							\
 	pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
@@ -79,14 +81,17 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
 					u32 meta_size, u32 meta_max_size,
 					void *meta_ret)
 {
-	struct pcache_meta_header *meta, *latest = NULL;
+	struct pcache_meta_header *latest = NULL;
+	struct pcache_meta_header *meta __free(kvfree);
 	u32 i, seq_latest = 0;
-	void *meta_addr;
 
-	meta = meta_ret;
+	meta = kvzalloc(meta_size, GFP_KERNEL);
+	if (!meta)
+		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < PCACHE_META_INDEX_MAX; i++) {
-		meta_addr = (void *)header + (i * meta_max_size);
+		void *meta_addr = (void *)header + (i * meta_max_size);
+
 		if (copy_mc_to_kernel(meta, meta_addr, meta_size)) {
 			pcache_err("hardware memory error when copy meta");
 			return ERR_PTR(-EIO);
-- 
2.51.0

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Dongsheng Yang]

Hi Li,

     It seems you sent the same patch again, shoud it be V2 instead?

Thanx

在 11/5/2025 4:46 PM, Li Chen 写道:
> From: Li Chen <chenl311@chinatelecom.cn>
>
> Before this change pcache_meta_find_latest() was copying each
> slot directly into meta_ret while scanning. If no valid slot
> was found and the function returned NULL, meta_ret still held
> whatever was last copied (possibly CRC-bad). Later users
> (e.g. cache_segs_init) could mistakenly trust that data.
>
> Allocate a temporary buffer instead and only populate meta_ret after a
> valid/latest header is found. If no valid header exists we return NULL
> without touching meta_ret.
>
> Also add __free(kvfree) so the temporary buffer is always freed, and
> include the needed headers.
>
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
>   drivers/md/dm-pcache/pcache_internal.h | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
> index b7a3319d2bd3e..ac28f9dd2986f 100644
> --- a/drivers/md/dm-pcache/pcache_internal.h
> +++ b/drivers/md/dm-pcache/pcache_internal.h
> @@ -4,6 +4,8 @@
>   
>   #include <linux/delay.h>
>   #include <linux/crc32c.h>
> +#include <linux/slab.h>
> +#include <linux/cleanup.h>
>   
>   #define pcache_err(fmt, ...)							\
>   	pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
> @@ -79,14 +81,17 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
>   					u32 meta_size, u32 meta_max_size,
>   					void *meta_ret)
>   {
> -	struct pcache_meta_header *meta, *latest = NULL;
> +	struct pcache_meta_header *latest = NULL;
> +	struct pcache_meta_header *meta __free(kvfree);
>   	u32 i, seq_latest = 0;
> -	void *meta_addr;
>   
> -	meta = meta_ret;
> +	meta = kvzalloc(meta_size, GFP_KERNEL);
> +	if (!meta)
> +		return ERR_PTR(-ENOMEM);
>   
>   	for (i = 0; i < PCACHE_META_INDEX_MAX; i++) {
> -		meta_addr = (void *)header + (i * meta_max_size);
> +		void *meta_addr = (void *)header + (i * meta_max_size);
> +
>   		if (copy_mc_to_kernel(meta, meta_addr, meta_size)) {
>   			pcache_err("hardware memory error when copy meta");
>   			return ERR_PTR(-EIO);

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

Hi Dongsheng, 

 ---- On Mon, 10 Nov 2025 19:18:38 +0800  Dongsheng Yang <dongsheng.yang@linux.dev> wrote --- 
 > Hi Li,
 > 
 >      It seems you sent the same patch again, shoud it be V2 instead?

Apologies for the duplicate patchset. I mistakenly included pcache in that GCC plugin series.
I will send the pcache v2 patchset tomorrow.

Regards,

Li​

[RFC PATCH 0/2] Add cleanup_plugin for detecting problematic cleanup patterns

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Hello,

This patch series introduces a new GCC plugin called cleanup_plugin that
warns developers about problematic patterns when using variables with
__attribute__((cleanup(...))). The plugin addresses concerns documented
in include/linux/cleanup.h regarding resource leaks and interdependency
issues.

The cleanup attribute helpers (__free, DEFINE_FREE, etc.) are designed
to automatically clean up resources when variables go out of scope,
following LIFO (last in first out) ordering. However, certain patterns
can lead to subtle bugs:

1. Uninitialized cleanup variables: Variables declared with cleanup
   attributes but not initialized can cause issues when cleanup functions
   are called on undefined values.

2. NULL-initialized cleanup variables: The "__free(...) = NULL" pattern
   at function top can cause interdependency problems, especially when
   combined with guards or multiple cleanup variables, as the cleanup
   may run in unexpected contexts.

The plugin detects both of these problematic patterns and provides clear
warnings to developers, helping prevent  incorrect cleanup ordering.
Importantly, the plugin's warnings are not converted
to errors by -Werror, allowing builds to continue while still alerting
developers to potential issues.

The plugin is enabled by default as it provides valuable compile-time
feedback without impacting build performance.

Li Chen (2):
  gcc-plugins: add cleanup_plugin for uninitialized cleanup detection
  gcc-plugins: cleanup_plugin: detect NULL init

 scripts/Makefile.gcc-plugins         |   1 +
 scripts/gcc-plugins/Kconfig          |   6 +
 scripts/gcc-plugins/cleanup_plugin.c | 204 +++++++++++++++++++++++++++
 3 files changed, 211 insertions(+)
 create mode 100644 scripts/gcc-plugins/cleanup_plugin.c

-- 
2.51.0

Re: [RFC PATCH 0/2] Add cleanup_plugin for detecting problematic cleanup patterns

[Li Chen]

+Peter, Dan, and Bjorn

(My apologies for the oversight)

 ---- On Wed, 05 Nov 2025 16:46:55 +0800  Li Chen <me@linux.beauty> wrote --- 
 > From: Li Chen <chenl311@chinatelecom.cn>
 > 
 > Hello,
 > 
 > This patch series introduces a new GCC plugin called cleanup_plugin that
 > warns developers about problematic patterns when using variables with
 > __attribute__((cleanup(...))). The plugin addresses concerns documented
 > in include/linux/cleanup.h regarding resource leaks and interdependency
 > issues.
 > 
 > The cleanup attribute helpers (__free, DEFINE_FREE, etc.) are designed
 > to automatically clean up resources when variables go out of scope,
 > following LIFO (last in first out) ordering. However, certain patterns
 > can lead to subtle bugs:
 > 
 > 1. Uninitialized cleanup variables: Variables declared with cleanup
 >    attributes but not initialized can cause issues when cleanup functions
 >    are called on undefined values.
 > 
 > 2. NULL-initialized cleanup variables: The "__free(...) = NULL" pattern
 >    at function top can cause interdependency problems, especially when
 >    combined with guards or multiple cleanup variables, as the cleanup
 >    may run in unexpected contexts.
 > 
 > The plugin detects both of these problematic patterns and provides clear
 > warnings to developers, helping prevent  incorrect cleanup ordering.
 > Importantly, the plugin's warnings are not converted
 > to errors by -Werror, allowing builds to continue while still alerting
 > developers to potential issues.
 > 
 > The plugin is enabled by default as it provides valuable compile-time
 > feedback without impacting build performance.
 > 
 > Li Chen (2):
 >   gcc-plugins: add cleanup_plugin for uninitialized cleanup detection
 >   gcc-plugins: cleanup_plugin: detect NULL init
 > 
 >  scripts/Makefile.gcc-plugins         |   1 +
 >  scripts/gcc-plugins/Kconfig          |   6 +
 >  scripts/gcc-plugins/cleanup_plugin.c | 204 +++++++++++++++++++++++++++
 >  3 files changed, 211 insertions(+)
 >  create mode 100644 scripts/gcc-plugins/cleanup_plugin.c
 > 
 > -- 
 > 2.51.0
 > 
 > 
Regards,

Li​

Re: [RFC PATCH 0/2] Add cleanup_plugin for detecting problematic cleanup patterns

[Peter Zijlstra]

On Wed, Nov 05, 2025 at 05:04:02PM +0800, Li Chen wrote:
> +Peter, Dan, and Bjorn
> 
> (My apologies for the oversight)
> 
>  ---- On Wed, 05 Nov 2025 16:46:55 +0800  Li Chen <me@linux.beauty> wrote --- 
>  > From: Li Chen <chenl311@chinatelecom.cn>
>  > 
>  > Hello,
>  > 
>  > This patch series introduces a new GCC plugin called cleanup_plugin that
>  > warns developers about problematic patterns when using variables with
>  > __attribute__((cleanup(...))). The plugin addresses concerns documented
>  > in include/linux/cleanup.h regarding resource leaks and interdependency
>  > issues.
>  > 
>  > The cleanup attribute helpers (__free, DEFINE_FREE, etc.) are designed
>  > to automatically clean up resources when variables go out of scope,
>  > following LIFO (last in first out) ordering. However, certain patterns
>  > can lead to subtle bugs:
>  > 
>  > 1. Uninitialized cleanup variables: Variables declared with cleanup
>  >    attributes but not initialized can cause issues when cleanup functions
>  >    are called on undefined values.
>  > 
>  > 2. NULL-initialized cleanup variables: The "__free(...) = NULL" pattern
>  >    at function top can cause interdependency problems, especially when
>  >    combined with guards or multiple cleanup variables, as the cleanup
>  >    may run in unexpected contexts.
>  > 
>  > The plugin detects both of these problematic patterns and provides clear
>  > warnings to developers, helping prevent  incorrect cleanup ordering.
>  > Importantly, the plugin's warnings are not converted
>  > to errors by -Werror, allowing builds to continue while still alerting
>  > developers to potential issues.
>  > 
>  > The plugin is enabled by default as it provides valuable compile-time
>  > feedback without impacting build performance.

IIRC GCC also allow dumb stuff like gotos into the scope of a cleanup
variable, where clang will fail the compile. Does this plugin also fix
this?

Re: [RFC PATCH 0/2] Add cleanup_plugin for detecting problematic cleanup patterns

[Li Chen]

Hi Peter,

 ---- On Wed, 05 Nov 2025 17:49:04 +0800  Peter Zijlstra <peterz@infradead.org> wrote --- 
 > On Wed, Nov 05, 2025 at 05:04:02PM +0800, Li Chen wrote:
 > > +Peter, Dan, and Bjorn
 > > 
 > > (My apologies for the oversight)
 > > 
 > >  ---- On Wed, 05 Nov 2025 16:46:55 +0800  Li Chen <me@linux.beauty> wrote --- 
 > >  > From: Li Chen <chenl311@chinatelecom.cn>
 > >  > 
 > >  > Hello,
 > >  > 
 > >  > This patch series introduces a new GCC plugin called cleanup_plugin that
 > >  > warns developers about problematic patterns when using variables with
 > >  > __attribute__((cleanup(...))). The plugin addresses concerns documented
 > >  > in include/linux/cleanup.h regarding resource leaks and interdependency
 > >  > issues.
 > >  > 
 > >  > The cleanup attribute helpers (__free, DEFINE_FREE, etc.) are designed
 > >  > to automatically clean up resources when variables go out of scope,
 > >  > following LIFO (last in first out) ordering. However, certain patterns
 > >  > can lead to subtle bugs:
 > >  > 
 > >  > 1. Uninitialized cleanup variables: Variables declared with cleanup
 > >  >    attributes but not initialized can cause issues when cleanup functions
 > >  >    are called on undefined values.
 > >  > 
 > >  > 2. NULL-initialized cleanup variables: The "__free(...) = NULL" pattern
 > >  >    at function top can cause interdependency problems, especially when
 > >  >    combined with guards or multiple cleanup variables, as the cleanup
 > >  >    may run in unexpected contexts.
 > >  > 
 > >  > The plugin detects both of these problematic patterns and provides clear
 > >  > warnings to developers, helping prevent  incorrect cleanup ordering.
 > >  > Importantly, the plugin's warnings are not converted
 > >  > to errors by -Werror, allowing builds to continue while still alerting
 > >  > developers to potential issues.
 > >  > 
 > >  > The plugin is enabled by default as it provides valuable compile-time
 > >  > feedback without impacting build performance.
 > 
 > IIRC GCC also allow dumb stuff like gotos into the scope of a cleanup
 > variable, where clang will fail the compile. Does this plugin also fix
 > this?
 > 

I'm sorry, but I don't fully understand what you mean by "gotos into the 
scope of a cleanup variable". Could you please provide a sample to illustrate this issue?
And I would try to fix it here if I can.

Regards,

Li​

[RFC PATCH 1/2] gcc-plugins: add cleanup_plugin for uninitialized cleanup detection

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Add a GCC plugin to warn about uninitialized automatic variables with
cleanup attributes. This helps catch this potential interdependency problem
as documented in include/linux/cleanup.h.

The plugin detects uninitialized cleanup variables and warns developers
without blocking builds (warnings are not converted to errors by -Werror).

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 scripts/Makefile.gcc-plugins         |   1 +
 scripts/gcc-plugins/Kconfig          |   6 ++
 scripts/gcc-plugins/cleanup_plugin.c | 106 +++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)
 create mode 100644 scripts/gcc-plugins/cleanup_plugin.c

diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index b0e1423b09c21..b948261c142e6 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
+gcc-plugin-$(CONFIG_GCC_PLUGIN_CLEANUP_ATTRIBUTE_WARN) += cleanup_plugin.so
 gcc-plugin-$(CONFIG_GCC_PLUGIN_LATENT_ENTROPY)	+= latent_entropy_plugin.so
 gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_LATENT_ENTROPY)		\
 		+= -DLATENT_ENTROPY_PLUGIN
diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index 6b34ba19358d8..906d50eb5efa6 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -36,4 +36,10 @@ config GCC_PLUGIN_LATENT_ENTROPY
 	   * https://grsecurity.net/
 	   * https://pax.grsecurity.net/
 
+config GCC_PLUGIN_CLEANUP_ATTRIBUTE_WARN
+	def_bool y
+	help
+	  Warn when local automatic variables annotated with
+	  __attribute__((cleanup(...))) are declared without an initializer.
+
 endif
diff --git a/scripts/gcc-plugins/cleanup_plugin.c b/scripts/gcc-plugins/cleanup_plugin.c
new file mode 100644
index 0000000000000..d28f8969186de
--- /dev/null
+++ b/scripts/gcc-plugins/cleanup_plugin.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Warn about uninitialized automatic variables that use the
+ * __attribute__((cleanup(...))) attribute.
+ */
+
+#include "gcc-common.h"
+
+__visible int plugin_is_GPL_compatible;
+
+static struct plugin_info cleanup_plugin_info = {
+	.version = PLUGIN_VERSION,
+	.help = "Warn when cleanup attribute variables lack initializers\n",
+};
+
+static bool has_cleanup_attribute(tree var)
+{
+	tree attrs;
+
+	attrs = DECL_ATTRIBUTES(var);
+	if (!attrs)
+		return false;
+
+	return lookup_attribute("cleanup", attrs) != NULL_TREE;
+}
+
+static bool is_candidate_decl(tree var)
+{
+	if (TREE_CODE(var) != VAR_DECL)
+		return false;
+
+	if (DECL_ARTIFICIAL(var))
+		return false;
+
+	if (TREE_STATIC(var) || DECL_EXTERNAL(var))
+		return false;
+
+	if (!has_cleanup_attribute(var))
+		return false;
+
+	return true;
+}
+
+static bool has_declaration_initializer(tree var)
+{
+	if (DECL_INITIAL(var))
+		return true;
+
+#ifdef DECL_INITIALIZED_P
+	if (DECL_INITIALIZED_P(var))
+		return true;
+#endif
+
+	return false;
+}
+
+static void warn_if_uninitialized(tree var)
+{
+	location_t loc;
+	bool saved_warning_as_error;
+
+	if (has_declaration_initializer(var))
+		return;
+
+	loc = DECL_SOURCE_LOCATION(var);
+	if (loc == UNKNOWN_LOCATION)
+		return;
+
+	/* Temporarily disable treating warnings as errors for this specific warning */
+	saved_warning_as_error = global_dc->warning_as_error_requested_p();
+	global_dc->set_warning_as_error_requested(false);
+	warning_at(
+		loc, 0,
+		"%qD declared with cleanup attribute is not initialized at declaration",
+		var);
+	/* Restore the original setting */
+	global_dc->set_warning_as_error_requested(saved_warning_as_error);
+}
+
+static void cleanup_finish_decl(void *gcc_data, void *user_data)
+{
+	tree var = (tree)gcc_data;
+
+	(void)user_data;
+
+	if (!is_candidate_decl(var))
+		return;
+
+	warn_if_uninitialized(var);
+}
+
+__visible int plugin_init(struct plugin_name_args *plugin_info,
+			  struct plugin_gcc_version *version)
+{
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	register_callback(plugin_info->base_name, PLUGIN_INFO, NULL,
+			  &cleanup_plugin_info);
+	register_callback(plugin_info->base_name, PLUGIN_FINISH_DECL,
+			  cleanup_finish_decl, NULL);
+
+	return 0;
+}
-- 
2.51.0

[RFC PATCH 2/2] gcc-plugins: cleanup_plugin: detect NULL init

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

The cleanup_plugin should warn about both uninitialized cleanup variables
and those initialized to NULL, as documented in include/linux/cleanup.h.
The "__free(...) = NULL" pattern at function top poses interdependency
problems.

Also, plugin warnings should not be converted to errors by -Werror, as
they are informational warnings meant to guide developers, not block
builds.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 scripts/gcc-plugins/Kconfig          |   2 +-
 scripts/gcc-plugins/cleanup_plugin.c | 122 ++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 13 deletions(-)

diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
index 906d50eb5efa6..ae6f79c617149 100644
--- a/scripts/gcc-plugins/Kconfig
+++ b/scripts/gcc-plugins/Kconfig
@@ -40,6 +40,6 @@ config GCC_PLUGIN_CLEANUP_ATTRIBUTE_WARN
 	def_bool y
 	help
 	  Warn when local automatic variables annotated with
-	  __attribute__((cleanup(...))) are declared without an initializer.
+	  __attribute__((cleanup(...))) are declared without an constructor.
 
 endif
diff --git a/scripts/gcc-plugins/cleanup_plugin.c b/scripts/gcc-plugins/cleanup_plugin.c
index d28f8969186de..c0744bbb7ef15 100644
--- a/scripts/gcc-plugins/cleanup_plugin.c
+++ b/scripts/gcc-plugins/cleanup_plugin.c
@@ -1,7 +1,69 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Warn about uninitialized automatic variables that use the
- * __attribute__((cleanup(...))) attribute.
+ * Copyright 2025 by Li Chen <me@linux.beauty>
+ *
+ * This gcc plugin warns about problematic patterns when using variables
+ * with __attribute__((cleanup(...))). The cleanup attribute helpers
+ * (__free, DEFINE_FREE, etc.) are designed to automatically clean up
+ * resources when variables go out of scope, following LIFO ordering.
+ * However, certain patterns can lead to interdependency issues.
+ *
+ * The plugin detects two problematic patterns:
+ *
+ * 1. Uninitialized cleanup variables:
+ *    Variables declared with cleanup attributes but not initialized can
+ *    cause issues when cleanup functions are called on undefined values.
+ *
+ *    Example:
+ *    void func(void)
+ *    {
+ *        struct resource *res __free(cleanup);  // Warning: not initialized
+ *        res = acquire_resource();
+ *        // ...
+ *    }
+ *
+ *    Should be:
+ *    void func(void)
+ *    {
+ *        struct resource *res __free(cleanup) = acquire_resource();
+ *        // ...
+ *    }
+ *
+ * 2. NULL-initialized cleanup variables:
+ *    The "__free(...) = NULL" pattern at function top can cause
+ *    interdependency problems, especially when combined with guards or
+ *    multiple cleanup variables, as documented in include/linux/cleanup.h.
+ *
+ *    Example:
+ *    void func(void)
+ *    {
+ *        struct resource *res __free(cleanup) = NULL;  // Warning: NULL init
+ *        guard(mutex)(&lock);
+ *        res = acquire_resource();
+ *        // cleanup may run without lock held!
+ *    }
+ *
+ *    Should be:
+ *    void func(void)
+ *    {
+ *        guard(mutex)(&lock);
+ *        struct resource *res __free(cleanup) = acquire_resource();
+ *        // ...
+ *    }
+ *
+ * The plugin provides clear warnings to help developers identify these
+ * patterns during compilation. Importantly, these warnings are not
+ * converted to errors by -Werror, allowing builds to continue while
+ * still alerting developers to potential issues.
+ *
+ * Options:
+ * - None currently supported
+ *
+ * Attribute: __attribute__((cleanup(...)))
+ *  The cleanup gcc attribute can be used on automatic variables to
+ *  specify a function to be called when the variable goes out of scope.
+ *  This plugin validates that such variables are properly initialized
+ *  at declaration time to avoid interdependency issues.
  */
 
 #include "gcc-common.h"
@@ -41,38 +103,74 @@ static bool is_candidate_decl(tree var)
 	return true;
 }
 
-static bool has_declaration_initializer(tree var)
+static bool is_null_initializer(tree initial)
 {
-	if (DECL_INITIAL(var))
+	if (!initial)
+		return false;
+
+	/* Check if the initializer is NULL pointer constant */
+	if (initial == null_pointer_node)
 		return true;
 
-#ifdef DECL_INITIALIZED_P
-	if (DECL_INITIALIZED_P(var))
+	/* Check if it's an integer constant zero (which can be NULL) */
+	if (TREE_CODE(initial) == INTEGER_CST && integer_zerop(initial))
 		return true;
-#endif
 
 	return false;
 }
 
+static bool has_valid_declaration_initializer(tree var)
+{
+	tree initial = DECL_INITIAL(var);
+
+	/* No initializer at all */
+	if (!initial) {
+#ifdef DECL_INITIALIZED_P
+		if (DECL_INITIALIZED_P(var))
+			return true;
+#endif
+		return false;
+	}
+
+	/* NULL initialization is considered invalid for cleanup variables */
+	if (is_null_initializer(initial))
+		return false;
+
+	/* Any other non-NULL initializer is valid */
+	return true;
+}
+
 static void warn_if_uninitialized(tree var)
 {
 	location_t loc;
 	bool saved_warning_as_error;
+	tree initial = DECL_INITIAL(var);
+	bool is_null_init = false;
 
-	if (has_declaration_initializer(var))
+	if (has_valid_declaration_initializer(var))
 		return;
 
 	loc = DECL_SOURCE_LOCATION(var);
 	if (loc == UNKNOWN_LOCATION)
 		return;
 
+	/* Check if it's a NULL initialization */
+	is_null_init = initial && is_null_initializer(initial);
+
 	/* Temporarily disable treating warnings as errors for this specific warning */
 	saved_warning_as_error = global_dc->warning_as_error_requested_p();
 	global_dc->set_warning_as_error_requested(false);
-	warning_at(
-		loc, 0,
-		"%qD declared with cleanup attribute is not initialized at declaration",
-		var);
+	if (is_null_init) {
+		warning_at(
+			loc, 0,
+			"%qD declared with cleanup attribute is initialized to NULL at declaration",
+			var);
+	} else {
+		warning_at(
+			loc, 0,
+			"%qD declared with cleanup attribute is not initialized at declaration",
+			var);
+	}
 	/* Restore the original setting */
 	global_dc->set_warning_as_error_requested(saved_warning_as_error);
 }
-- 
2.51.0

Re: [PATCH 0/3] dm-pcache: built-in support and metadata hardening

[Li Chen]

My apologies, please disregard the dm-pcache portion of this series. This series will focus solely on gcc-plugin.

 ---- On Wed, 05 Nov 2025 16:46:51 +0800  Li Chen <me@linux.beauty> wrote --- 
 > From: Li Chen <chenl311@chinatelecom.cn>
 > 
 > This three-patch series tidies dm-pcache’s build glue and tightens the metadata scan.
 > 
 > Patch 1 allow dm-pcache to be linked into vmlinux and avoids clashing with the sunrpc
 > cache_flush() by using obj-$(CONFIG_DM_PCACHE) and renaming the helper across the tree.
 > 
 > Patch 2 drops a redundant recomputation of the metadata slot pointer while walking headers.
 > 
 > Patch 3 zero-allocates a temporary buffer so callers never see stale metadata,
 > relies on __free(kvfree) for cleanup, and only copies back once a valid record is found.
 > 
 > Thanks for your review.
 > 
 > Li Chen (3):
 >   dm-pcache: allow built-in build and rename flush helper
 >   dm-pcache: reuse meta_addr in pcache_meta_find_latest
 >   dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()
 > 
 >  drivers/md/dm-pcache/Makefile          |  2 +-
 >  drivers/md/dm-pcache/cache.c           |  2 +-
 >  drivers/md/dm-pcache/cache.h           |  2 +-
 >  drivers/md/dm-pcache/cache_req.c       |  6 +++---
 >  drivers/md/dm-pcache/pcache_internal.h | 15 ++++++++++-----
 >  5 files changed, 16 insertions(+), 11 deletions(-)
 > 
 > -- 
 > 2.51.0
 > 
 > 
Regards,

Li​

[PATCH 0/3] dm-pcache: built-in support and metadata hardening

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

This three-patch series tidies dm-pcache’s build glue and tightens the metadata scan.

Patch 1 allow dm-pcache to be linked into vmlinux and avoids clashing with the sunrpc
cache_flush() by using obj-$(CONFIG_DM_PCACHE) and renaming the helper across the tree.

Patch 2 drops a redundant recomputation of the metadata slot pointer while walking headers.

Patch 3 zero-allocates a temporary buffer so callers never see stale metadata,
relies on __free(kvfree) for cleanup, and only copies back once a valid record is found.

Thanks for your review.

Li Chen (3):
  dm-pcache: allow built-in build and rename flush helper
  dm-pcache: reuse meta_addr in pcache_meta_find_latest
  dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

 drivers/md/dm-pcache/Makefile          |  2 +-
 drivers/md/dm-pcache/cache.c           |  2 +-
 drivers/md/dm-pcache/cache.h           |  2 +-
 drivers/md/dm-pcache/cache_req.c       |  6 +++---
 drivers/md/dm-pcache/pcache_internal.h | 15 ++++++++++-----
 5 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.51.0

[PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

From: Li Chen <chenl311@chinatelecom.cn>

Before this change pcache_meta_find_latest() was copying each
slot directly into meta_ret while scanning. If no valid slot
was found and the function returned NULL, meta_ret still held
whatever was last copied (possibly CRC-bad). Later users
(e.g. cache_segs_init) could mistakenly trust that data.

Allocate a temporary buffer instead and only populate meta_ret after a
valid/latest header is found. If no valid header exists we return NULL
without touching meta_ret.

Also add __free(kvfree) so the temporary buffer is always freed, and
include the needed headers.

Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
---
 drivers/md/dm-pcache/pcache_internal.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
index b7a3319d2bd3e..ac28f9dd2986f 100644
--- a/drivers/md/dm-pcache/pcache_internal.h
+++ b/drivers/md/dm-pcache/pcache_internal.h
@@ -4,6 +4,8 @@
 
 #include <linux/delay.h>
 #include <linux/crc32c.h>
+#include <linux/slab.h>
+#include <linux/cleanup.h>
 
 #define pcache_err(fmt, ...)							\
 	pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
@@ -79,14 +81,17 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
 					u32 meta_size, u32 meta_max_size,
 					void *meta_ret)
 {
-	struct pcache_meta_header *meta, *latest = NULL;
+	struct pcache_meta_header *latest = NULL;
+	struct pcache_meta_header *meta __free(kvfree);
 	u32 i, seq_latest = 0;
-	void *meta_addr;
 
-	meta = meta_ret;
+	meta = kvzalloc(meta_size, GFP_KERNEL);
+	if (!meta)
+		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < PCACHE_META_INDEX_MAX; i++) {
-		meta_addr = (void *)header + (i * meta_max_size);
+		void *meta_addr = (void *)header + (i * meta_max_size);
+
 		if (copy_mc_to_kernel(meta, meta_addr, meta_size)) {
 			pcache_err("hardware memory error when copy meta");
 			return ERR_PTR(-EIO);
-- 
2.51.0

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

Hi Zheng,

 ---- On Fri, 31 Oct 2025 10:01:23 +0800  Zheng Gu <cengku@gmail.com> wrote --- 
 >> On Thu, Oct 30, 2025 at 8:36 PM Li Chen <me@linux.beauty> wrote:From: Li Chen <chenl311@chinatelecom.cn>
 >> 
 >> Before this change pcache_meta_find_latest() was copying each
 >> slot directly into meta_ret while scanning. If no valid slot
 >> was found and the function returned NULL, meta_ret still held
 >> whatever was last copied (possibly CRC-bad). Later users
 >> (e.g. cache_segs_init) could mistakenly trust that data.
 > 
 > This functions is * __must_check*, users must check the return value first before touching the meta_ret, so it should not be a problem here.

Right now, the callers only check the return value with IS_ERR(). If the
function returns NULL instead of an error pointer, a caller like
cache_info_init() will assume that no valid cache_info was found because all cache_info are
corrupted. Instead, it will try to init a new one, and then return 0 (success), 
https://github.com/torvalds/linux/blob/master/drivers/md/dm-pcache/cache.c#L61

Later, cache_tail_init() will access cache->cache_info.flags. But in this
path all cache_info may have already been corrupted, and the CRCs are mismatched
(https://github.com/torvalds/linux/blob/ba36dd5ee6fd4643ebbf6ee6eefcecf0b07e35c7/drivers/md/dm-pcache/pcache_internal.h#L97),
so flags may contain garbage.

This commit fixes this issue by allocating a temp buffer with kvmalloc, so meta_ret would never
contain corrupted values.

Regards,

Li​

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Dongsheng Yang]

在 11/1/2025 9:10 PM, Li Chen 写道:
> Hi Zheng,
>
>   ---- On Fri, 31 Oct 2025 10:01:23 +0800  Zheng Gu <cengku@gmail.com> wrote ---
>   >> On Thu, Oct 30, 2025 at 8:36 PM Li Chen <me@linux.beauty> wrote:From: Li Chen <chenl311@chinatelecom.cn>
>   >>
>   >> Before this change pcache_meta_find_latest() was copying each
>   >> slot directly into meta_ret while scanning. If no valid slot
>   >> was found and the function returned NULL, meta_ret still held
>   >> whatever was last copied (possibly CRC-bad). Later users
>   >> (e.g. cache_segs_init) could mistakenly trust that data.
>   >
>   > This functions is * __must_check*, users must check the return value first before touching the meta_ret, so it should not be a problem here.
>
> Right now, the callers only check the return value with IS_ERR(). If the
> function returns NULL instead of an error pointer, a caller like
> cache_info_init() will assume that no valid cache_info was found because all cache_info are
> corrupted. Instead, it will try to init a new one, and then return 0 (success),
> https://github.com/torvalds/linux/blob/master/drivers/md/dm-pcache/cache.c#L61
>
> Later, cache_tail_init() will access cache->cache_info.flags. But in this
> path all cache_info may have already been corrupted, and the CRCs are mismatched
> (https://github.com/torvalds/linux/blob/ba36dd5ee6fd4643ebbf6ee6eefcecf0b07e35c7/drivers/md/dm-pcache/pcache_internal.h#L97),
> so flags may contain garbage.
>
> This commit fixes this issue by allocating a temp buffer with kvmalloc, so meta_ret would never
> contain corrupted values.

Hi

     Thanx for your fix. So the better change should be reseting 
cache_info in cache_info_init_default() firstly by memset() with 0.

Allocating a temp buffer in pcache_meta_find_latest() is really not a 
good idea.

Thanx

>
> Regards,
>
> Li​
>
>

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

Hi Dongsheng,


 ---- On Tue, 04 Nov 2025 14:46:33 +0800  Dongsheng Yang <dongsheng.yang@linux.dev> wrote --- 
 > 
 > 在 11/1/2025 9:10 PM, Li Chen 写道:
 > > Hi Zheng,
 > >
 > >   ---- On Fri, 31 Oct 2025 10:01:23 +0800  Zheng Gu <cengku@gmail.com> wrote ---
 > >   >> On Thu, Oct 30, 2025 at 8:36 PM Li Chen <me@linux.beauty> wrote:From: Li Chen <chenl311@chinatelecom.cn>
 > >   >>
 > >   >> Before this change pcache_meta_find_latest() was copying each
 > >   >> slot directly into meta_ret while scanning. If no valid slot
 > >   >> was found and the function returned NULL, meta_ret still held
 > >   >> whatever was last copied (possibly CRC-bad). Later users
 > >   >> (e.g. cache_segs_init) could mistakenly trust that data.
 > >   >
 > >   > This functions is * __must_check*, users must check the return value first before touching the meta_ret, so it should not be a problem here.
 > >
 > > Right now, the callers only check the return value with IS_ERR(). If the
 > > function returns NULL instead of an error pointer, a caller like
 > > cache_info_init() will assume that no valid cache_info was found because all cache_info are
 > > corrupted. Instead, it will try to init a new one, and then return 0 (success),
 > > https://github.com/torvalds/linux/blob/master/drivers/md/dm-pcache/cache.c#L61
 > >
 > > Later, cache_tail_init() will access cache->cache_info.flags. But in this
 > > path all cache_info may have already been corrupted, and the CRCs are mismatched
 > > (https://github.com/torvalds/linux/blob/ba36dd5ee6fd4643ebbf6ee6eefcecf0b07e35c7/drivers/md/dm-pcache/pcache_internal.h#L97),
 > > so flags may contain garbage.
 > >
 > > This commit fixes this issue by allocating a temp buffer with kvmalloc, so meta_ret would never
 > > contain corrupted values.
 > 
 > Hi
 > 
 >      Thanx for your fix. So the better change should be reseting 
 > cache_info in cache_info_init_default() firstly by memset() with 0.
 > 
 > Allocating a temp buffer in pcache_meta_find_latest() is really not a 
 > good idea.

I considered using memset before sending the patch, but a temporary buffer seems more elegant. 
Since the variable is relatively large, I avoided stack allocation. If you prefer memset, should it be implemented 
within pcache_meta_find_latest or all its callers?

Regards,

Li​

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Dongsheng Yang]

在 11/4/2025 9:36 PM, Li Chen 写道:
> Hi Dongsheng,
>
>
>   ---- On Tue, 04 Nov 2025 14:46:33 +0800  Dongsheng Yang <dongsheng.yang@linux.dev> wrote ---
>   >
>   > 在 11/1/2025 9:10 PM, Li Chen 写道:
>   > > Hi Zheng,
>   > >
>   > >   ---- On Fri, 31 Oct 2025 10:01:23 +0800  Zheng Gu <cengku@gmail.com> wrote ---
>   > >   >> On Thu, Oct 30, 2025 at 8:36 PM Li Chen <me@linux.beauty> wrote:From: Li Chen <chenl311@chinatelecom.cn>
>   > >   >>
>   > >   >> Before this change pcache_meta_find_latest() was copying each
>   > >   >> slot directly into meta_ret while scanning. If no valid slot
>   > >   >> was found and the function returned NULL, meta_ret still held
>   > >   >> whatever was last copied (possibly CRC-bad). Later users
>   > >   >> (e.g. cache_segs_init) could mistakenly trust that data.
>   > >   >
>   > >   > This functions is * __must_check*, users must check the return value first before touching the meta_ret, so it should not be a problem here.
>   > >
>   > > Right now, the callers only check the return value with IS_ERR(). If the
>   > > function returns NULL instead of an error pointer, a caller like
>   > > cache_info_init() will assume that no valid cache_info was found because all cache_info are
>   > > corrupted. Instead, it will try to init a new one, and then return 0 (success),
>   > > https://github.com/torvalds/linux/blob/master/drivers/md/dm-pcache/cache.c#L61
>   > >
>   > > Later, cache_tail_init() will access cache->cache_info.flags. But in this
>   > > path all cache_info may have already been corrupted, and the CRCs are mismatched
>   > > (https://github.com/torvalds/linux/blob/ba36dd5ee6fd4643ebbf6ee6eefcecf0b07e35c7/drivers/md/dm-pcache/pcache_internal.h#L97),
>   > > so flags may contain garbage.
>   > >
>   > > This commit fixes this issue by allocating a temp buffer with kvmalloc, so meta_ret would never
>   > > contain corrupted values.
>   >
>   > Hi
>   >
>   >      Thanx for your fix. So the better change should be reseting
>   > cache_info in cache_info_init_default() firstly by memset() with 0.
>   >
>   > Allocating a temp buffer in pcache_meta_find_latest() is really not a
>   > good idea.
>
> I considered using memset before sending the patch, but a temporary buffer seems more elegant.
> Since the variable is relatively large, I avoided stack allocation. If you prefer memset, should it be implemented
> within pcache_meta_find_latest or all its callers?


callers should do this thing, it's about default value initialization, 
the callers understand what to do, but pcache_meta_find_latest() does not.

So the usage looks like below:

         meta = pcache_meta_find_latest();

If meta is error, return error.

If meta is not NULL, meta is valid, just use it.

If meta is NULL, that means there is no valid meta onmedia, just init it 
with default value (including cache_info.flags you mentioned, the 
default of this flags should be 0).


BTW, when you memset cache_info with 0 in cache_info_init_default();, 
you can remove this line: cache_info->header.seq = 0;

Thanx

>
> Regards,
>
> Li​
>

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Jonathan Cameron]

On Thu, 30 Oct 2025 20:33:21 +0800
Li Chen <me@linux.beauty> wrote:

> From: Li Chen <chenl311@chinatelecom.cn>
> 
> Before this change pcache_meta_find_latest() was copying each
> slot directly into meta_ret while scanning. If no valid slot
> was found and the function returned NULL, meta_ret still held
> whatever was last copied (possibly CRC-bad). Later users
> (e.g. cache_segs_init) could mistakenly trust that data.
> 
> Allocate a temporary buffer instead and only populate meta_ret after a
> valid/latest header is found. If no valid header exists we return NULL
> without touching meta_ret.
> 
> Also add __free(kvfree) so the temporary buffer is always freed, and
> include the needed headers.
> 
> Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
> ---
>  drivers/md/dm-pcache/pcache_internal.h | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
> index b7a3319d2bd3e..ac28f9dd2986f 100644
> --- a/drivers/md/dm-pcache/pcache_internal.h
> +++ b/drivers/md/dm-pcache/pcache_internal.h
> @@ -4,6 +4,8 @@
>  
>  #include <linux/delay.h>
>  #include <linux/crc32c.h>
> +#include <linux/slab.h>
> +#include <linux/cleanup.h>
>  
>  #define pcache_err(fmt, ...)							\
>  	pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
> @@ -79,14 +81,17 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
>  					u32 meta_size, u32 meta_max_size,
>  					void *meta_ret)
>  {
> -	struct pcache_meta_header *meta, *latest = NULL;
> +	struct pcache_meta_header *latest = NULL;
> +	struct pcache_meta_header *meta __free(kvfree);
>  	u32 i, seq_latest = 0;
> -	void *meta_addr;
>  
> -	meta = meta_ret;
> +	meta = kvzalloc(meta_size, GFP_KERNEL);
See the guidance notes in cleanup.h  THis hsould be

	struct pcache_meta_header *meta __free(kvfree) =
		kvzalloc(meta_size, GFP_KERNEL);

That is the constructor and destructor must be together. Inline variable
declarations are fine for this one type of use.

> +	if (!meta)
> +		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < PCACHE_META_INDEX_MAX; i++) {
> -		meta_addr = (void *)header + (i * meta_max_size);
> +		void *meta_addr = (void *)header + (i * meta_max_size);
> +
>  		if (copy_mc_to_kernel(meta, meta_addr, meta_size)) {
>  			pcache_err("hardware memory error when copy meta");
>  			return ERR_PTR(-EIO);

Re: [PATCH 3/3] dm-pcache: avoid leaking invalid metadata in pcache_meta_find_latest()

[Li Chen]

Hi Jonathan,

 ---- On Mon, 03 Nov 2025 19:38:03 +0800  Jonathan Cameron <jonathan.cameron@huawei.com> wrote --- 
 > On Thu, 30 Oct 2025 20:33:21 +0800
 > Li Chen <me@linux.beauty> wrote:
 > 
 > > From: Li Chen <chenl311@chinatelecom.cn>
 > > 
 > > Before this change pcache_meta_find_latest() was copying each
 > > slot directly into meta_ret while scanning. If no valid slot
 > > was found and the function returned NULL, meta_ret still held
 > > whatever was last copied (possibly CRC-bad). Later users
 > > (e.g. cache_segs_init) could mistakenly trust that data.
 > > 
 > > Allocate a temporary buffer instead and only populate meta_ret after a
 > > valid/latest header is found. If no valid header exists we return NULL
 > > without touching meta_ret.
 > > 
 > > Also add __free(kvfree) so the temporary buffer is always freed, and
 > > include the needed headers.
 > > 
 > > Signed-off-by: Li Chen <chenl311@chinatelecom.cn>
 > > ---
 > >  drivers/md/dm-pcache/pcache_internal.h | 13 +++++++++----
 > >  1 file changed, 9 insertions(+), 4 deletions(-)
 > > 
 > > diff --git a/drivers/md/dm-pcache/pcache_internal.h b/drivers/md/dm-pcache/pcache_internal.h
 > > index b7a3319d2bd3e..ac28f9dd2986f 100644
 > > --- a/drivers/md/dm-pcache/pcache_internal.h
 > > +++ b/drivers/md/dm-pcache/pcache_internal.h
 > > @@ -4,6 +4,8 @@
 > >  
 > >  #include <linux/delay.h>
 > >  #include <linux/crc32c.h>
 > > +#include <linux/slab.h>
 > > +#include <linux/cleanup.h>
 > >  
 > >  #define pcache_err(fmt, ...)                            \
 > >      pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
 > > @@ -79,14 +81,17 @@ static inline void __must_check *pcache_meta_find_latest(struct pcache_meta_head
 > >                      u32 meta_size, u32 meta_max_size,
 > >                      void *meta_ret)
 > >  {
 > > -    struct pcache_meta_header *meta, *latest = NULL;
 > > +    struct pcache_meta_header *latest = NULL;
 > > +    struct pcache_meta_header *meta __free(kvfree);
 > >      u32 i, seq_latest = 0;
 > > -    void *meta_addr;
 > >  
 > > -    meta = meta_ret;
 > > +    meta = kvzalloc(meta_size, GFP_KERNEL);
 > See the guidance notes in cleanup.h  THis hsould be
 > 
 >     struct pcache_meta_header *meta __free(kvfree) =
 >         kvzalloc(meta_size, GFP_KERNEL);
 > 
 > That is the constructor and destructor must be together. Inline variable
 > declarations are fine for this one type of use.

I appreciate your suggestion very much.

Regards,

Li​