[PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[Mark Harmstone]

Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in
btrfs_validate_super(), which clobbers the value of ret set earlier.
This has the effect of negating the validity checks done earlier, making
it so btrfs could potentially try to mount invalid filesystems.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
Cc: Qu Wenruo <wqu@suse.com>
Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")
---
 fs/btrfs/disk-io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0afd3c0f2fab..4421c946a53c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2562,6 +2562,9 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
+	if (ret)
+		return ret;
+
 	ret = validate_sys_chunk_array(fs_info, sb);
 
 	/*
-- 
2.45.3

Re: [PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[Filipe Manana]

On Tue, Mar 11, 2025 at 4:39 PM Mark Harmstone <maharmstone@fb.com> wrote:
>
> Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in
> btrfs_validate_super(), which clobbers the value of ret set earlier.
> This has the effect of negating the validity checks done earlier, making
> it so btrfs could potentially try to mount invalid filesystems.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Cc: Qu Wenruo <wqu@suse.com>
> Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")
> ---
>  fs/btrfs/disk-io.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0afd3c0f2fab..4421c946a53c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2562,6 +2562,9 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
>                 ret = -EINVAL;
>         }
>
> +       if (ret)
> +               return ret;
> +
>         ret = validate_sys_chunk_array(fs_info, sb);

While this fixes the problem, the function is structured in a way that
is easy to get into this sort of issue.
Rather than set 'ret' to -EINVAL every time some check fails and then
continue, I'd rather have it return -EINVAL immediately.

Anyway, this fixes a bug, so:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

>
>         /*
> --
> 2.45.3
>
>

Re: [PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[Mark Harmstone]

On 11/3/25 16:54, Filipe Manana wrote:
> > 
> On Tue, Mar 11, 2025 at 4:39 PM Mark Harmstone <maharmstone@fb.com> wrote:
>>
>> Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in
>> btrfs_validate_super(), which clobbers the value of ret set earlier.
>> This has the effect of negating the validity checks done earlier, making
>> it so btrfs could potentially try to mount invalid filesystems.
>>
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>> Cc: Qu Wenruo <wqu@suse.com>
>> Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")
>> ---
>>   fs/btrfs/disk-io.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 0afd3c0f2fab..4421c946a53c 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2562,6 +2562,9 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
>>                  ret = -EINVAL;
>>          }
>>
>> +       if (ret)
>> +               return ret;
>> +
>>          ret = validate_sys_chunk_array(fs_info, sb);
> 
> While this fixes the problem, the function is structured in a way that
> is easy to get into this sort of issue.
> Rather than set 'ret' to -EINVAL every time some check fails and then
> continue, I'd rather have it return -EINVAL immediately.
> 
> Anyway, this fixes a bug, so:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks. Yes, I did think that, but if there's multiple reasons why your 
FS won't mount you probably want to know all of them.

> 
> Thanks.
> 
>>
>>          /*
>> --
>> 2.45.3
>>
>>

Re: [PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[Qu Wenruo]

在 2025/3/12 03:09, Mark Harmstone 写道:
> Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in
> btrfs_validate_super(), which clobbers the value of ret set earlier.
> This has the effect of negating the validity checks done earlier, making
> it so btrfs could potentially try to mount invalid filesystems.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Cc: Qu Wenruo <wqu@suse.com>
> Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks for catching this.

Although I agree with Filipe that it's better to return the error 
immediately in the future.

Yes, currently it will report all the errors, but if the first check 
against magic number fails, the super block can just be garbage and all 
the remaining checks will be triggered.

Other than a single "no valid fs found" message, we got all kinds of 
errors, which is not that easy for end users to understand.

Thanks,
Qu

> ---
>   fs/btrfs/disk-io.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0afd3c0f2fab..4421c946a53c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2562,6 +2562,9 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
>   		ret = -EINVAL;
>   	}
>   
> +	if (ret)
> +		return ret;
> +
>   	ret = validate_sys_chunk_array(fs_info, sb);
>   
>   	/*

Re: [PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[David Sterba]

On Wed, Mar 12, 2025 at 07:40:09AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2025/3/12 03:09, Mark Harmstone 写道:
> > Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in
> > btrfs_validate_super(), which clobbers the value of ret set earlier.
> > This has the effect of negating the validity checks done earlier, making
> > it so btrfs could potentially try to mount invalid filesystems.
> > 
> > Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> > Cc: Qu Wenruo <wqu@suse.com>
> > Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Thanks for catching this.
> 
> Although I agree with Filipe that it's better to return the error 
> immediately in the future.
> 
> Yes, currently it will report all the errors, but if the first check 
> against magic number fails, the super block can just be garbage and all 
> the remaining checks will be triggered.

We can return immediately when the magic does not match, it's more
likely that the sb would be garbage so the following checks will fail
and be just noise.

However if the magic is correct then I find it better to print which
errors were found, they get all logged to the syslog and it can help
analyzing the problem.

Re: [PATCH] btrfs: don't clobber ret in btrfs_validate_super()

[David Sterba]

On Tue, Mar 11, 2025 at 04:39:25PM +0000, Mark Harmstone wrote:
> Commit 2a9bb78cfd36 introduces a call to validate_sys_chunk_array() in

Please use full commit references in changelogs, like 

2a9bb78cfd36 ("btrfs: validate system chunk array at
btrfs_validate_super()")

there's a commonly used git alias:

[alias]
	one = show -s --pretty='format:%h (\"%s\")

> btrfs_validate_super(), which clobbers the value of ret set earlier.
> This has the effect of negating the validity checks done earlier, making
> it so btrfs could potentially try to mount invalid filesystems.
> 
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
> Cc: Qu Wenruo <wqu@suse.com>
> Fixes: 2a9bb78cfd36 ("btrfs: validate system chunk array at btrfs_validate_super()")
> ---
>  fs/btrfs/disk-io.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 0afd3c0f2fab..4421c946a53c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2562,6 +2562,9 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
>  		ret = -EINVAL;
>  	}
>  
> +	if (ret)
> +		return ret;
> +
>  	ret = validate_sys_chunk_array(fs_info, sb);

This is ok as a fix for the return value cloberring.

Tips for followup cleanups:

Right after the call to validate_sys_chunk_array() there are two checks
that duplicate the sys_array validation (maximum and minimum length). So
they can be removed from btrfs_validate_super(). Then there are two more
checks of generation of chunk tree and "cache_generation". Both can be
moved in front of the sys_array validation as they're simple like the
other checks.

As discussed in the thread, the magic check can return immediately as
its mismatch will likely trigger several other checks and would not
bring much help.