Re: [PATCH] nilfs2: validate file block counts during recovery

[Viacheslav Dubeyko]

On Sat, 2026-09-19 at 00:44 +0900, Ryusuke Konishi wrote:
> On Tue, Sep 15, 2026 at 8:14 PM Aldo Ariel Panzardo wrote:
> > 
> > nilfs_scan_dsync_log() trusts the block counts in each on-disk file
> > information entry. If fi_ndatablk is greater than fi_nblocks, the
> > data
> > block loop can consume excessive summary entries and the later
> > subtraction
> > used to derive the number of node blocks underflows.
> > 
> > Reject inconsistent file information entries before consuming their
> > block
> > information.
> > 
> > Fixes: 0f3e1c7f23f8 ("nilfs2: recovery functions")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
> > ---
> >  fs/nilfs2/recovery.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/fs/nilfs2/recovery.c b/fs/nilfs2/recovery.c
> > index 4d5a6aa521..6e4e0cf4cc 100644
> > --- a/fs/nilfs2/recovery.c
> > +++ b/fs/nilfs2/recovery.c
> > @@ -322,6 +322,7 @@ static void nilfs_skip_summary_info(struct
> > the_nilfs *nilfs,
> >   *
> >   * Return: 0 on success, or one of the following negative error
> > codes on
> >   * failure:
> > + * * %-EINVAL  - Invalid block counts in a file information entry.
> >   * * %-EIO     - I/O error.
> >   * * %-ENOMEM  - Insufficient memory available.
> >   */
> > @@ -359,6 +360,10 @@ static int nilfs_scan_dsync_log(struct
> > the_nilfs *nilfs, sector_t start_blocknr,
> >                 ino = le64_to_cpu(finfo->fi_ino);
> >                 nblocks = le32_to_cpu(finfo->fi_nblocks);
> >                 ndatablk = le32_to_cpu(finfo->fi_ndatablk);
> > +               if (ndatablk > nblocks) {
> > +                       err = -EINVAL;
> > +                       goto out;
> > +               }
> >                 nnodeblk = nblocks - ndatablk;
> > 
> >                 while (ndatablk-- > 0) {
> > --
> > 2.43.0
> 
> Acked-by: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>
> 
> Hi Viacheslav,
> 
> Please apply this v1 patch instead of the v2 patch.
> 
> This function is called as part of mount operations.  When format
> errors are detected during superblock reading or log scanning at
> mount
> time, returning -EINVAL rather than -EIO is in line with the mount
> system call behavior, so the v1 implementation is the correct one.

I missed that. Thanks for the explanation.

> 
> Also, please replace the 'Cc: stable' tag with the following tag, as
> with the previous patch:
> 
> Cc: stable+noautosel@xxxxxxxxxx # Non-fatal bug fix; defer backport
> until a real issue is reported

Damn! I missed this request. I already pushed the commit. Are you OK
with that? :)

> 
> I actually ran a test where a pseudo underflow of the nnodeblk
> variable occurred, but no issues that compromise system stability
> happened.
> 
> Even if nnodeblk becomes a large value, long-duration block device
> scanning does not occur; it simply skips the position and abandons
> roll-forward recovery midway.  Since the mount normally succeeds, if
> anything, this silent failure is the issue.
> 
> However, this is based on the assumption that the file system image
> was intentionally tampered with (anything goes) with root privileges,
> including log checksums.  Therefore, as long as it does not break the
> system, I do not believe it meets the stable kernel rules for
> backporting.
> 

Applied.

Thanks,
Slava.