[PATCH] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[PATCH] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[Zhen Ni]

Fix smatch warn:
fs/adfs/dir_fplus.c:146 adfs_fplus_read() warn: missing error code 'ret'

Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
---
 fs/adfs/dir_fplus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 4a15924014da..4334279409b2 100644
--- a/fs/adfs/dir_fplus.c
+++ b/fs/adfs/dir_fplus.c
@@ -143,6 +143,7 @@ static int adfs_fplus_read(struct super_block *sb, u32 indaddr,
 
 	if (adfs_fplus_checkbyte(dir) != t->bigdircheckbyte) {
 		adfs_error(sb, "dir %06x checkbyte mismatch\n", indaddr);
+		ret = -EIO;
 		goto out;
 	}
 
-- 
2.20.1

Re: [PATCH] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[Markus Elfring]

> Fix smatch warn:
> fs/adfs/dir_fplus.c:146 adfs_fplus_read() warn: missing error code 'ret'

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.16#n145

Regards,
Markus

Re: [PATCH] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[Al Viro]

On Fri, Aug 01, 2025 at 05:38:06PM +0800, Zhen Ni wrote:
> Fix smatch warn:
> fs/adfs/dir_fplus.c:146 adfs_fplus_read() warn: missing error code 'ret'

Gyah...  You don't fix $TOOL warnings, you fix underlying bugs - or, in case
of $TOOL generating a false positive, making $TOOL to STFU.  At the very
least you need to tell which one it is.

In this case it was not a false positive, so you need an explanation of
what's wrong with the code in question ("it makes smatch to complain"
is *NOT* it).

What happens here is a failure exit where we do complain into dmesg,
but actually report success to the caller, since the eventual return
value (in 'ret') is 0 left behind by successful adfs_fplus_validate_tail()
in the previous test.

_IF_ that behaviour had been an intentional mitigation strategy,
it needs to be explicitly described as such; however, looking at the
history of that function it seems more likely that this is an analogue
of the bug in previous failure exit fixed in 587065dcac64 ("fs/adfs:
bigdir: Fix an error code in adfs_fplus_read()"), so it's probably
better to return an error, same as we do on the other failure exits.

As it is, commit message fails to explain anything other than "smatch
has been involved in catching this one".  If you want to mention that,
sure, but it's on the same level as "so-and-so told me this place
looks fishy" - by all means, credit the contribution.  Reported-by/
caught-by/actually quoting smatch warning somewhere in the message -
up to you, but that shouldn't be the entirety of commit message ;-)

Re: [PATCH] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[Russell King (Oracle)]

On Fri, Aug 01, 2025 at 08:22:00PM +0100, Al Viro wrote:
> As it is, commit message fails to explain anything other than "smatch
> has been involved in catching this one".  If you want to mention that,
> sure, but it's on the same level as "so-and-so told me this place
> looks fishy" - by all means, credit the contribution.  Reported-by/
> caught-by/actually quoting smatch warning somewhere in the message -
> up to you, but that shouldn't be the entirety of commit message ;-)

Agree with everything you've said.

FWIW, the patch is definitely correct, but the commit text should
also have:

Fixes: d79288b4f61b ("fs/adfs: bigdir: calculate and validate directory checkbyte")

as that introduced the bug.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v2] fs/adfs: bigdir: Restore EIO errno return when checkbyte mismatch

[Zhen Ni]

The error path in adfs_fplus_read() prints an error message via
adfs_error() but incorrectly returns success (0) to the caller.
This occurs because the 'ret' variable remains set to 0 from the earlier
successful call to adfs_fplus_validate_tail().

Fix by setting 'ret = -EIO' before jumping to the error exit.

This issue was detected by smatch static analysis:
warning: fs/adfs/dir_fplus.c:146: adfs_fplus_read() warn: missing error
code 'ret'.

Fixes: d79288b4f61b ("fs/adfs: bigdir: calculate and validate directory checkbyte")
Cc: stable@vger.kernel.org
Signed-off-by: Zhen Ni <zhen.ni@easystack.cn>
---
Changes in v2:
- Add tags of 'Fixes' and 'Cc' in commit message
- Added error description and the corresponding fix in commit message
---
 fs/adfs/dir_fplus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/adfs/dir_fplus.c b/fs/adfs/dir_fplus.c
index 4a15924014da..4334279409b2 100644
--- a/fs/adfs/dir_fplus.c
+++ b/fs/adfs/dir_fplus.c
@@ -143,6 +143,7 @@ static int adfs_fplus_read(struct super_block *sb, u32 indaddr,
 
 	if (adfs_fplus_checkbyte(dir) != t->bigdircheckbyte) {
 		adfs_error(sb, "dir %06x checkbyte mismatch\n", indaddr);
+		ret = -EIO;
 		goto out;
 	}
 
-- 
2.20.1