We should set the error code when ext4_commit_super check argument
failed.
Signed-off-by: Fengnan Chang <[email protected]>
---
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
Is there any problem with this patch? I did not see a reply, please let me
know if there is a problem. Thanks
-----?ʼ?ԭ??-----
??????: Fengnan Chang <[email protected]>
????ʱ??: 2021??3??29?? 11:58
?ռ???: [email protected]; [email protected]; [email protected]
????: Fengnan Chang <[email protected]>
????: [PATCH] ext4: fix error code in ext4_commit_super
We should set the error code when ext4_commit_super check argument failed.
Signed-off-by: Fengnan Chang <[email protected]>
---
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
On Apr 1, 2021, at 9:32 PM, <[email protected]> <[email protected]> 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 <[email protected]>
> 发送时间: 2021年3月29日 11:58
> 收件人: [email protected]; [email protected]; [email protected]
>
> 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 <[email protected]>
> ---
> 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
Thanks for your advice, We did found this problem during code review.
I will revise it according to your opinion.
-----邮件原件-----
发件人: Andreas Dilger <[email protected]>
发送时间: 2021年4月2日 15:58
收件人: [email protected]
抄送: Theodore Y. Ts'o <[email protected]>; Ext4 Developers List
<[email protected]>
主题: Re: [PATCH] ext4: fix error code in ext4_commit_super
On Apr 1, 2021, at 9:32 PM, <[email protected]> <[email protected]>
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 <[email protected]>
> 发送时间: 2021年3月29日 11:58
> 收件人: [email protected]; [email protected];
> [email protected]
>
> 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 <[email protected]>
> ---
> 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