linux-ext4 - Re: [PATCH] ext4: fix error code in ext4_commit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <C68A6772-93DE-49E4-8C33-0AF480B4DC3F@dilger.ca>
Date:   Fri, 2 Apr 2021 01:58:29 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     changfengnan@...o.com
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: fix error code in ext4_commit_super

On Apr 1, 2021, at 9:32 PM, <changfengnan@...o.com> <changfengnan@...o.com> wrote:
> 
> Is there any problem with this patch? I did not see a reply, please let me
> know if there is a problem. Thanks

It's only been a few days since the patch was first posted, so nobody has
had a chance to review it yet.  The patch looks "obviously correct" at
first glance, but often changing code is not as obvious as first expected.
Also, is "-EINVAL" the best code here?  For "block_device_ejected()" it
might be more clear to return "-ENODEV" so that it prints "No such device"
instead of "Invalid argument" in userspace.

> 发件人: Fengnan Chang <changfengnan@...o.com>
> 发送时间: 2021年3月29日 11:58
> 收件人: tytso@....edu; adilger.kernel@...ger.ca; linux-ext4@...r.kernel.org
> 
> We should set the error code when ext4_commit_super check argument failed.

It would be useful if this also described how this problem was hit, or
what caused this issue to be seen/fixed.  "Found while reviewing code"
is OK, if that is the case, or "crashed during mount when ejecting a
floppy disk", or whatever.  That makes it more clear how important the
bug fix is to be landed and backported.

It would also be useful to include a "Fixes:" label to show which patch
caused the problem, and help decide which stable kernels need this patch.
From running "git blame fs/ext4/super.c" appears that commit 2d01ddc86606
("ext4: save error info to sb through journal if available") introduced
the problem, but it seems like that patch only copied it from older code.
However, further digging shows commit c4be0c1dc4cdc ("filesystem freeze:
add error handling of write_super_lockfs/unlockfs") added this particular
code (error = 0; return error).  Before that time, no error was returned
from this function at all, so the commit message should include:

Fixes: c4be0c1dc4cdc ("filesystem freeze: add error handling of write_super_lockfs/unlockfs")

Cheers, Andreas

> Signed-off-by: Fengnan Chang <changfengnan@...o.com>
> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c index
> 03373471131c..5440b8ff86a8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5501,7 +5501,7 @@ static int ext4_commit_super(struct super_block *sb,
> int sync)
> 	int error = 0;
> 
> 	if (!sbh || block_device_ejected(sb))
> -		return error;
> +		return -EINVAL;
> 
> 	/*
> 	 * If the file system is mounted read-only, don't update the
> --
> 2.29.0
> 
> 
> 

Cheers, Andreas

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)