Message-ID: <CAKFNMomBZBVZZ_ohHDuZpJJWJHz_5FE+6EDxVoiFDvRCfFb_HQ@mail.gmail.com>
Date: Thu, 3 Apr 2025 03:36:12 +0900
From: Ryusuke Konishi <konishi.ryusuke@...il.com>
To: Wentao Liang <vulab@...as.ac.cn>
Cc: linux-nilfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nilfs2: Add pointer check for nilfs_direct_propagate()

I will comment inline.

On Wed, Apr 2, 2025 at 5:59 PM Wentao Liang wrote:
>
> In nilfs_direct_propagate(), the printer get from nilfs_direct_get_ptr()
> need to be checked to ensure it is not an invalid pointer. A proper
> implementation can be found in nilfs_direct_delete(). Add a value check
> and return -ENOENT when it is an invalid pointer.

nilfs_direct_propagate() (and its caller nilfs_bmap_propagate()) are
internal operations called only by the log writer, which writes back
new or modified blocks, so we must ensure that -ENOENT is not
erroneously propagated to system calls.

In nilfs_direct_propagate(), if the pointer value obtained by
nilfs_direct_get_ptr() is NILFS_BMAP_INVALID_PTR, it means that the
metadata (in this case, i_bmap in the nilfs_inode_info struct) that
should point to the data block at the buffer head of the argument is
corrupted and the data block is orphaned, meaning that the file system
has lost consistency.

To handle this case, return -EINVAL instead of -ENOENT.

If you do so, the caller nilfs_bmap_propagate() will treat it as
metadata corruption via nilfs_bmap_convert_error() and the error code
will be converted appropriately.

And please don't cite nilfs_direct_delete() as an example.  It
intentionally returns -ENOENT as an error indicating that the block
you tried to delete did not exist, regardless of whether the
filesystem is corrupted or not.  On the other hand,
nilfs_direct_propagate() assumes that the block associated with the
buffer head of the argument can be looked up.  So, these are based on
different assumptions.

Please revise the changelog description and the returned error code
with the above in mind.

>
> Fixes: 10ff885ba6f5 ("nilfs2: get rid of nilfs_direct uses")

The commit that should be pointed to as the cause is:

Fixes: 36a580eb489f (“nilfs2: direct block mapping”)

> Cc: stable@...r.kernel.org # v2.6+

Please remove this tag. If it becomes a real issue and becomes a
higher priority, I will add it.  (At least with the Fixes tag, it will
be picked up automatically and eventually backported to the stable
trees will be attempted.)
For now I will send it for the next cycle as an extra sanity check,
after making sure it doesn't introduce any new issues.

Again, when you send patches with git send-email, please be careful
not to include stable@...r.kernel.org in the recipient list.
To avoid being noisy, at this stage, it is sufficient to only send the
patch to me (the maintainer), the linux-nilfs ML and LKML.

Thanks,
Ryusuke Konishi

> Signed-off-by: Wentao Liang <vulab@...as.ac.cn>
> ---
>  fs/nilfs2/direct.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/nilfs2/direct.c b/fs/nilfs2/direct.c
> index 893ab36824cc..ff1c9fe72bec 100644
> --- a/fs/nilfs2/direct.c
> +++ b/fs/nilfs2/direct.c
> @@ -273,6 +273,9 @@ static int nilfs_direct_propagate(struct nilfs_bmap *bmap,
>         dat = nilfs_bmap_get_dat(bmap);
>         key = nilfs_bmap_data_get_key(bmap, bh);
>         ptr = nilfs_direct_get_ptr(bmap, key);
> +       if (ptr == NILFS_BMAP_INVALID_PTR)
> +               return -ENOENT;
> +
>         if (!buffer_nilfs_volatile(bh)) {
>                 oldreq.pr_entry_nr = ptr;
>                 newreq.pr_entry_nr = ptr;
> --
> 2.42.0.windows.2
>

Powered by blists - more mailing lists