[v1] nilfs2: validate file block counts during recovery

[PATCH] nilfs2: validate file block counts during recovery

Posted by Aldo Ariel Panzardo 1 week, 2 days ago

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@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 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

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

Posted by Ryusuke Konishi 6 days, 15 hours ago

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@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  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@gmail.com>

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.

Also, please replace the 'Cc: stable' tag with the following tag, as
with the previous patch:

Cc: stable+noautosel@kernel.org # Non-fatal bug fix; defer backport
until a real issue is reported

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.

Thanks,
Ryusuke Konishi

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

Posted by Viacheslav Dubeyko 6 days, 11 hours ago

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@vger.kernel.org
> > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> > ---
> >  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@gmail.com>
> 
> 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@kernel.org # 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.

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

Posted by Ryusuke Konishi 6 days, 6 hours ago

On Sat, Sep 19, 2026 at 4:33 AM Viacheslav Dubeyko wrote:
>
> On Sat, 2026-09-19 at 00:44 +0900, Ryusuke Konishi wrote:
> > Also, please replace the 'Cc: stable' tag with the following tag, as
> > with the previous patch:
> >
> > Cc: stable+noautosel@kernel.org # 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? :)

Viacheslav,

Since we haven't hit the merge window yet, and linux-next (the
destination where your for-next branch is integrated) is a daily
integration tree intended to be rebased before being pulled into
Linus's tree, updating the commit message on your branch now should be
completely fine.

As documented in Documentation/maintainer/rebasing-and-merging.rst:
"Some trees (linux-next being a significant example) are frequently
rebased by their nature..."

I think it would be better to fix the tag and overwrite-push it now
rather than leaving it as is, though of course it will need to be
frozen once we hit the merge window (or after the official release).

Thanks,
Ryusuke Konishi

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

Posted by Viacheslav Dubeyko 6 days, 4 hours ago

On Sat, 2026-09-19 at 09:46 +0900, Ryusuke Konishi wrote:
> On Sat, Sep 19, 2026 at 4:33 AM Viacheslav Dubeyko wrote:
> > 
> > On Sat, 2026-09-19 at 00:44 +0900, Ryusuke Konishi wrote:
> > > Also, please replace the 'Cc: stable' tag with the following tag,
> > > as
> > > with the previous patch:
> > > 
> > > Cc: stable+noautosel@kernel.org # 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? :)
> 
> Viacheslav,
> 
> Since we haven't hit the merge window yet, and linux-next (the
> destination where your for-next branch is integrated) is a daily
> integration tree intended to be rebased before being pulled into
> Linus's tree, updating the commit message on your branch now should
> be
> completely fine.
> 
> As documented in Documentation/maintainer/rebasing-and-merging.rst:
> "Some trees (linux-next being a significant example) are frequently
> rebased by their nature..."
> 
> I think it would be better to fix the tag and overwrite-push it now
> rather than leaving it as is, though of course it will need to be
> frozen once we hit the merge window (or after the official release).
> 

Sorry, I have too busy day.

The commit message has been updated and pushed.

Thanks,
Slava.

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

Posted by Ryusuke Konishi 6 days, 4 hours ago

On Sat, Sep 19, 2026 at 12:01 PM Viacheslav Dubeyko wrote:
>
> On Sat, 2026-09-19 at 09:46 +0900, Ryusuke Konishi wrote:
> > On Sat, Sep 19, 2026 at 4:33 AM Viacheslav Dubeyko wrote:
> > >
> > > On Sat, 2026-09-19 at 00:44 +0900, Ryusuke Konishi wrote:
> > > > Also, please replace the 'Cc: stable' tag with the following tag,
> > > > as
> > > > with the previous patch:
> > > >
> > > > Cc: stable+noautosel@kernel.org # 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? :)
> >
> > Viacheslav,
> >
> > Since we haven't hit the merge window yet, and linux-next (the
> > destination where your for-next branch is integrated) is a daily
> > integration tree intended to be rebased before being pulled into
> > Linus's tree, updating the commit message on your branch now should
> > be
> > completely fine.
> >
> > As documented in Documentation/maintainer/rebasing-and-merging.rst:
> > "Some trees (linux-next being a significant example) are frequently
> > rebased by their nature..."
> >
> > I think it would be better to fix the tag and overwrite-push it now
> > rather than leaving it as is, though of course it will need to be
> > frozen once we hit the merge window (or after the official release).
> >
>
> Sorry, I have too busy day.
>
> The commit message has been updated and pushed.
>
> Thanks,
> Slava.

No problem.

Thanks for handling this.

Ryusuke Konishi

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

Posted by Aldo Ariel Panzardo 1 week, 2 days ago

Hi Slava,

Right, -EIO is more appropriate for corrupt on-disk metadata. Will
fix in v2.

Aldo

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

Posted by Viacheslav Dubeyko 1 week, 2 days ago

On Tue, 2026-09-15 at 08:13 -0300, 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@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  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;

It sounds like -EIO because we have corrupted state of on-disk
metadata.

Thanks,
Slava.

> +			goto out;
> +		}
>  		nnodeblk = nblocks - ndatablk;
>  
>  		while (ndatablk-- > 0) {

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

Posted by Aldo Ariel Panzardo 1 week, 2 days ago

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@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/nilfs2/recovery.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/nilfs2/recovery.c b/fs/nilfs2/recovery.c
index 4d5a6aa521..28200cef4d 100644
--- a/fs/nilfs2/recovery.c
+++ b/fs/nilfs2/recovery.c
@@ -359,6 +359,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 = -EIO;
+			goto out;
+		}
 		nnodeblk = nblocks - ndatablk;
 
 		while (ndatablk-- > 0) {
-- 
2.43.0

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

Posted by Viacheslav Dubeyko 1 week, 1 day ago

On Tue, 2026-09-15 at 16:57 -0300, 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@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  fs/nilfs2/recovery.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/nilfs2/recovery.c b/fs/nilfs2/recovery.c
> index 4d5a6aa521..28200cef4d 100644
> --- a/fs/nilfs2/recovery.c
> +++ b/fs/nilfs2/recovery.c
> @@ -359,6 +359,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 = -EIO;
> +			goto out;
> +		}
>  		nnodeblk = nblocks - ndatablk;
>  
>  		while (ndatablk-- > 0) {

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.