| Message ID | 20230223073222.81702-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] f2fs: fix to check quota inums | expand |
On 02/23, Yangtao Li wrote: > We should check quota file ino before enable quota, and not only 0. > > BTW fix following check error: > > WARNING: Do not crash the kernel unless it is absolutely > unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) > instead of BUG() or variants. > > Fixes: ea6767337f86 ("f2fs: support quota sys files") > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/f2fs/super.c | 54 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 36 insertions(+), 18 deletions(-) > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index aa55dc12aff2..c7e0639892e2 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -2652,22 +2652,40 @@ int f2fs_enable_quota_files(struct f2fs_sb_info *sbi, bool rdonly) > return enabled; > } > > +static inline bool f2fs_check_quota_inum(int type, unsigned long qf_inum) > +{ > + switch (type) { > + case USRQUOTA: > + return qf_inum == 4; > + case GRPQUOTA: > + return qf_inum == 5; > + case PRJQUOTA: > + return qf_inum == 6; I don't want to make this dependency of the inode numbers. > + default: > + return false; > + } > +} > + > static int f2fs_quota_enable(struct super_block *sb, int type, int format_id, > unsigned int flags) > { > + struct f2fs_sb_info *sbi = F2FS_SB(sb); > struct inode *qf_inode; > unsigned long qf_inum; > int err; > > - BUG_ON(!f2fs_sb_has_quota_ino(F2FS_SB(sb))); > + f2fs_bug_on(sbi, !f2fs_sb_has_quota_ino(sbi)); > > qf_inum = f2fs_qf_ino(sb, type); > - if (!qf_inum) > - return -EPERM; > + if (!f2fs_check_quota_inum(type, qf_inum)) { > + f2fs_err(sbi, "Bad quota inum: %lu, type: %d", > + qf_inum, type); > + return -EFSCORRUPTED; > + } > > qf_inode = f2fs_iget(sb, qf_inum); > if (IS_ERR(qf_inode)) { > - f2fs_err(F2FS_SB(sb), "Bad quota inode %u:%lu", type, qf_inum); > + f2fs_err(sbi, "Bad quota inode %u:%lu", type, qf_inum); > return PTR_ERR(qf_inode); > } > > @@ -2682,7 +2700,7 @@ static int f2fs_enable_quotas(struct super_block *sb) > { > struct f2fs_sb_info *sbi = F2FS_SB(sb); > int type, err = 0; > - unsigned long qf_inum; > + char count = MAXQUOTAS; > bool quota_mopt[MAXQUOTAS] = { > test_opt(sbi, USRQUOTA), > test_opt(sbi, GRPQUOTA), > @@ -2696,21 +2714,21 @@ static int f2fs_enable_quotas(struct super_block *sb) > > sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE; > > - for (type = 0; type < MAXQUOTAS; type++) { > - qf_inum = f2fs_qf_ino(sb, type); > - if (qf_inum) { > - err = f2fs_quota_enable(sb, type, QFMT_VFS_V1, > + if (!f2fs_sb_has_project_quota(sbi)) > + count--; > + > + for (type = 0; type < count; type++) { > + err = f2fs_quota_enable(sb, type, QFMT_VFS_V1, > DQUOT_USAGE_ENABLED | > (quota_mopt[type] ? DQUOT_LIMITS_ENABLED : 0)); > - if (err) { > - f2fs_err(sbi, "Failed to enable quota tracking (type=%d, err=%d). Please run fsck to fix.", > - type, err); > - for (type--; type >= 0; type--) > - dquot_quota_off(sb, type); > - set_sbi_flag(F2FS_SB(sb), > - SBI_QUOTA_NEED_REPAIR); > - return err; > - } > + if (err) { > + f2fs_err(sbi, "Failed to enable quota tracking (type=%d, err=%d). Please run fsck to fix.", > + type, err); > + for (type--; type >= 0; type--) > + dquot_quota_off(sb, type); > + set_sbi_flag(F2FS_SB(sb), > + SBI_QUOTA_NEED_REPAIR); > + return err; > } > } > return 0; > -- > 2.25.1
On 2023/3/7 4:59, Jaegeuk Kim wrote: > On 02/23, Yangtao Li wrote: >> +static inline bool f2fs_check_quota_inum(int type, unsigned long qf_inum) >> +{ >> + switch (type) { >> + case USRQUOTA: >> + return qf_inum == 4; >> + case GRPQUOTA: >> + return qf_inum == 5; >> + case PRJQUOTA: >> + return qf_inum == 6; > > I don't want to make this dependency of the inode numbers. Actually, mkfs.f2fs allocates qf_inum dynamically, above sanity check may fail mount()... Thanks,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index aa55dc12aff2..c7e0639892e2 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -2652,22 +2652,40 @@ int f2fs_enable_quota_files(struct f2fs_sb_info *sbi, bool rdonly) return enabled; } +static inline bool f2fs_check_quota_inum(int type, unsigned long qf_inum) +{ + switch (type) { + case USRQUOTA: + return qf_inum == 4; + case GRPQUOTA: + return qf_inum == 5; + case PRJQUOTA: + return qf_inum == 6; + default: + return false; + } +} + static int f2fs_quota_enable(struct super_block *sb, int type, int format_id, unsigned int flags) { + struct f2fs_sb_info *sbi = F2FS_SB(sb); struct inode *qf_inode; unsigned long qf_inum; int err; - BUG_ON(!f2fs_sb_has_quota_ino(F2FS_SB(sb))); + f2fs_bug_on(sbi, !f2fs_sb_has_quota_ino(sbi)); qf_inum = f2fs_qf_ino(sb, type); - if (!qf_inum) - return -EPERM; + if (!f2fs_check_quota_inum(type, qf_inum)) { + f2fs_err(sbi, "Bad quota inum: %lu, type: %d", + qf_inum, type); + return -EFSCORRUPTED; + } qf_inode = f2fs_iget(sb, qf_inum); if (IS_ERR(qf_inode)) { - f2fs_err(F2FS_SB(sb), "Bad quota inode %u:%lu", type, qf_inum); + f2fs_err(sbi, "Bad quota inode %u:%lu", type, qf_inum); return PTR_ERR(qf_inode); } @@ -2682,7 +2700,7 @@ static int f2fs_enable_quotas(struct super_block *sb) { struct f2fs_sb_info *sbi = F2FS_SB(sb); int type, err = 0; - unsigned long qf_inum; + char count = MAXQUOTAS; bool quota_mopt[MAXQUOTAS] = { test_opt(sbi, USRQUOTA), test_opt(sbi, GRPQUOTA), @@ -2696,21 +2714,21 @@ static int f2fs_enable_quotas(struct super_block *sb) sb_dqopt(sb)->flags |= DQUOT_QUOTA_SYS_FILE; - for (type = 0; type < MAXQUOTAS; type++) { - qf_inum = f2fs_qf_ino(sb, type); - if (qf_inum) { - err = f2fs_quota_enable(sb, type, QFMT_VFS_V1, + if (!f2fs_sb_has_project_quota(sbi)) + count--; + + for (type = 0; type < count; type++) { + err = f2fs_quota_enable(sb, type, QFMT_VFS_V1, DQUOT_USAGE_ENABLED | (quota_mopt[type] ? DQUOT_LIMITS_ENABLED : 0)); - if (err) { - f2fs_err(sbi, "Failed to enable quota tracking (type=%d, err=%d). Please run fsck to fix.", - type, err); - for (type--; type >= 0; type--) - dquot_quota_off(sb, type); - set_sbi_flag(F2FS_SB(sb), - SBI_QUOTA_NEED_REPAIR); - return err; - } + if (err) { + f2fs_err(sbi, "Failed to enable quota tracking (type=%d, err=%d). Please run fsck to fix.", + type, err); + for (type--; type >= 0; type--) + dquot_quota_off(sb, type); + set_sbi_flag(F2FS_SB(sb), + SBI_QUOTA_NEED_REPAIR); + return err; } } return 0;
We should check quota file ino before enable quota, and not only 0. BTW fix following check error: WARNING: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants. Fixes: ea6767337f86 ("f2fs: support quota sys files") Signed-off-by: Yangtao Li <frank.li@vivo.com> --- fs/f2fs/super.c | 54 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 18 deletions(-)