| Message ID | 20210903090538.GA7283@kili |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ext2: do not sleep in ext2_error() | expand |
On Fri, Sep 03, 2021 at 12:05:38PM +0300, Dan Carpenter wrote: > No one expects error logging functions to sleep so sometimes they are > called with spinlocks held. In this case the problematic call tree is: > > ext2_statfs() <- disables preempt > -> ext2_count_free_inodes() > -> ext2_get_group_desc() > -> ext2_error() > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > This is just from static analysis. NOT TESTED! > > Probably a safer fix would be to just call pr_err() instead of > ext2_error() in ext2_get_group_desc(). I can send that fix instead if > people want. Looking at both of the ext2_error() calls in ext2_get_group_desc(), those are really more in the way of assertions rather than warning of an on-disk corruption issue. The second "group descriptor not loaded" should never happen, and the "block_group >= groups_count" should have been caught via an invalid block number or check by the caller (or an outright code bug in say ext2_statfs(). So I suspect both of those would be more usefule as a WARN() rather than a call to ext2_error(), since stack trace would actually provide more useful data to root causing the issue. Jan, what do you think? - Ted P.S. The same analysis applies for ext4_get_group_desc(), BTW. We don't take a lock in ext4_statfs() so trying to take a lock while sleeping is not an issue. For both ext2 and ext4, the caller is not supposed to holding spin locks when it calls ext[24]_error(). In cases where it is absolutely not avoidable, special measures are required --- see for example __ext4_grp_locked_error().
On Fri, Sep 03, 2021 at 08:48:38AM -0400, Theodore Ts'o wrote: > On Fri, Sep 03, 2021 at 12:05:38PM +0300, Dan Carpenter wrote: > > No one expects error logging functions to sleep so sometimes they are > > called with spinlocks held. In this case the problematic call tree is: > > > > ext2_statfs() <- disables preempt > > -> ext2_count_free_inodes() > > -> ext2_get_group_desc() > > -> ext2_error() > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > This is just from static analysis. NOT TESTED! > > > > Probably a safer fix would be to just call pr_err() instead of > > ext2_error() in ext2_get_group_desc(). I can send that fix instead if > > people want. > > Looking at both of the ext2_error() calls in ext2_get_group_desc(), > those are really more in the way of assertions rather than warning of > an on-disk corruption issue. The second "group descriptor not loaded" > should never happen, and the "block_group >= groups_count" should have > been caught via an invalid block number or check by the caller (or an > outright code bug in say ext2_statfs(). > > So I suspect both of those would be more usefule as a WARN() rather > than a call to ext2_error(), since stack trace would actually provide > more useful data to root causing the issue. Jan, what do you think? > > - Ted Thanks Ted, I'll resend with the WARN() change. regards, dan carpenter
On Fri 03-09-21 08:48:38, Theodore Ts'o wrote: > On Fri, Sep 03, 2021 at 12:05:38PM +0300, Dan Carpenter wrote: > > No one expects error logging functions to sleep so sometimes they are > > called with spinlocks held. In this case the problematic call tree is: > > > > ext2_statfs() <- disables preempt > > -> ext2_count_free_inodes() > > -> ext2_get_group_desc() > > -> ext2_error() > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > This is just from static analysis. NOT TESTED! > > > > Probably a safer fix would be to just call pr_err() instead of > > ext2_error() in ext2_get_group_desc(). I can send that fix instead if > > people want. > > Looking at both of the ext2_error() calls in ext2_get_group_desc(), > those are really more in the way of assertions rather than warning of > an on-disk corruption issue. The second "group descriptor not loaded" > should never happen, and the "block_group >= groups_count" should have > been caught via an invalid block number or check by the caller (or an > outright code bug in say ext2_statfs(). > > So I suspect both of those would be more usefule as a WARN() rather > than a call to ext2_error(), since stack trace would actually provide > more useful data to root causing the issue. Jan, what do you think? Yes, I agree. Definitely better than not flushing error on other ext2_error() calls. BTW, Dan, I don't see a patch with WARN() in my inbox. Did it get lost somewhere? Honza
diff --git a/fs/ext2/super.c b/fs/ext2/super.c index d8d580b609ba..ba345ab860f0 100644 --- a/fs/ext2/super.c +++ b/fs/ext2/super.c @@ -59,7 +59,7 @@ void ext2_error(struct super_block *sb, const char *function, sbi->s_mount_state |= EXT2_ERROR_FS; es->s_state |= cpu_to_le16(EXT2_ERROR_FS); spin_unlock(&sbi->s_lock); - ext2_sync_super(sb, es, 1); + ext2_sync_super(sb, es, 0); } va_start(args, fmt);
No one expects error logging functions to sleep so sometimes they are called with spinlocks held. In this case the problematic call tree is: ext2_statfs() <- disables preempt -> ext2_count_free_inodes() -> ext2_get_group_desc() -> ext2_error() Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- This is just from static analysis. NOT TESTED! Probably a safer fix would be to just call pr_err() instead of ext2_error() in ext2_get_group_desc(). I can send that fix instead if people want. fs/ext2/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)