| Message ID | 20250311163931.1021554-1-maharmstone@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: don't clobber ret in btrfs_validate_super() | expand |
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 > >
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 >> >>
在 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); > > /*
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.
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.
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); /*
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(+)