Commit b226804532a8 ("hfs: Replace BUG_ON with error handling for CNID count checks") was an improvement in that it avoids kernel crash, but was not an improvement in several aspects. First aspect is that it increased possibility of committing corrupted counter values to mdb, for that commit did not eliminate possibility of temporarily overflowing counter values despite these counter values might be concurrently read by hfs_mdb_commit() because nothing serializes hfs_new_inode() and hfs_mdb_commit(). hfs_new_inode() { hfs_mdb_commit() { (...snipped...) (...snipped...) // 4294967295 -> 4294967296 file_count = atomic64_inc_return(&HFS_SB(sb)->file_count); // Detects overflow if (file_count > U32_MAX) { // 0 will be committed instead of 4294967295 mdb->drFilCnt = cpu_to_be32((u32)atomic64_read(&HFS_SB(sb)->file_count)); // 4294967296 -> 4294967295 atomic64_dec(&HFS_SB(sb)->file_count); goto out_discard; } (...snipped...) (...snipped...) } } Second aspect is that is_hfs_cnid_counts_valid() cannot check for negative counter values due to s64 and u32 comparison. if (unlikely(atomic64_read(&sbi->file_count) > U32_MAX)) { pr_warn("file count exceeds limit\n"); corrupted = true; } Third aspect is that is_hfs_cnid_counts_valid() needlessly rejects creation/deletion of files/directories, for overflow of these counter values is not fatal error condition. These counter values are only for informational use (which is not guaranteed to be in sync with actual number of files/directories). The only fatal error condition that must reject is that all available 32bits inode numbers are already in use when creating a file or directory. Other conditions can be checked and corrected when fsck.hfs is run. Fourth aspect is that is_hfs_cnid_counts_valid() calls printk() without ratelimit. A filesystem could stay in mounted state for days/weeks/months, and e.g. sync() request could be called for e.g. 100 times per second. The consequence will be stall problem due to printk() flooding and/or out of disk space due to unimportant kernel messages. Fix some of these aspects, by don't allow temporarily overflowing counter values for files/directories and remove printk() for file/directory counters. This patch does not touch next_id counter, for there are different topics to consider. Technically speaking, we can shrink these counter values from atomic64_t to atomic_t, but this patch does not change because Viacheslav Dubeyko does not want to treat -1 (negative value) as U32_MAX (positive value). Signed-off-by: Tetsuo Handa --- fs/hfs/inode.c | 30 ++++++++++-------------------- fs/hfs/mdb.c | 8 -------- 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c index 878535db64d6..e10f96d1711e 100644 --- a/fs/hfs/inode.c +++ b/fs/hfs/inode.c @@ -185,8 +185,6 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t struct super_block *sb = dir->i_sb; struct inode *inode = new_inode(sb); s64 next_id; - s64 file_count; - s64 folder_count; int err = -ENOMEM; if (!inode) @@ -216,13 +214,8 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t HFS_I(inode)->tz_secondswest = sys_tz.tz_minuteswest * 60; if (S_ISDIR(mode)) { inode->i_size = 2; - folder_count = atomic64_inc_return(&HFS_SB(sb)->folder_count); - if (folder_count> U32_MAX) { - atomic64_dec(&HFS_SB(sb)->folder_count); - pr_err("cannot create new inode: folder count exceeds limit\n"); - goto out_discard; - } - if (dir->i_ino == HFS_ROOT_CNID) + atomic64_add_unless(&HFS_SB(sb)->folder_count, 1, U32_MAX); + if (dir->i_ino == HFS_ROOT_CNID && HFS_SB(sb)->root_dirs != U16_MAX) HFS_SB(sb)->root_dirs++; inode->i_op = &hfs_dir_inode_operations; inode->i_fop = &hfs_dir_operations; @@ -230,13 +223,8 @@ struct inode *hfs_new_inode(struct inode *dir, const struct qstr *name, umode_t inode->i_mode &= ~HFS_SB(inode->i_sb)->s_dir_umask; } else if (S_ISREG(mode)) { HFS_I(inode)->clump_blocks = HFS_SB(sb)->clumpablks; - file_count = atomic64_inc_return(&HFS_SB(sb)->file_count); - if (file_count > U32_MAX) { - atomic64_dec(&HFS_SB(sb)->file_count); - pr_err("cannot create new inode: file count exceeds limit\n"); - goto out_discard; - } - if (dir->i_ino == HFS_ROOT_CNID) + atomic64_add_unless(&HFS_SB(sb)->file_count, 1, U32_MAX); + if (dir->i_ino == HFS_ROOT_CNID && HFS_SB(sb)->root_files != U16_MAX) HFS_SB(sb)->root_files++; inode->i_op = &hfs_file_inode_operations; inode->i_fop = &hfs_file_operations; @@ -272,16 +260,18 @@ void hfs_delete_inode(struct inode *inode) hfs_dbg("ino %lu\n", inode->i_ino); if (S_ISDIR(inode->i_mode)) { - atomic64_dec(&HFS_SB(sb)->folder_count); - if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID)) + atomic64_add_unless(&HFS_SB(sb)->folder_count, -1, 0); + if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID) && + HFS_SB(sb)->root_dirs) HFS_SB(sb)->root_dirs--; set_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags); hfs_mark_mdb_dirty(sb); return; } - atomic64_dec(&HFS_SB(sb)->file_count); - if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID)) + atomic64_add_unless(&HFS_SB(sb)->file_count, -1, 0); + if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID) && + HFS_SB(sb)->root_files) HFS_SB(sb)->root_files--; if (S_ISREG(inode->i_mode)) { if (!inode->i_nlink) { diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c index a97cea35ca2e..e79e36b7ed84 100644 --- a/fs/hfs/mdb.c +++ b/fs/hfs/mdb.c @@ -73,14 +73,6 @@ bool is_hfs_cnid_counts_valid(struct super_block *sb) pr_warn("next CNID exceeds limit\n"); corrupted = true; } - if (unlikely(atomic64_read(&sbi->file_count) > U32_MAX)) { - pr_warn("file count exceeds limit\n"); - corrupted = true; - } - if (unlikely(atomic64_read(&sbi->folder_count) > U32_MAX)) { - pr_warn("folder count exceeds limit\n"); - corrupted = true; - } return !corrupted; } -- 2.53.0