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

[Dongsheng Yang <dongsheng.yang@linux.dev>]

在 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​
>