| 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
| ||
|
Date: Fri, 2 Feb 2018 16:36:20 -0700
From: Andreas Dilger <adilger@...ger.ca>
To: Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc: linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] ext4: Add helper functions to access inode numbers
On Feb 2, 2018, at 2:41 AM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
>
> 64-bit inodes counter uses extra fields to store hight part.
> Let's incapsulate inode number reading and writing to extend
> counter in next commits.
>
> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@...il.com>
> ---
> fs/ext4/dir.c | 4 +--
> fs/ext4/ext4.h | 44 +++++++++++++++++++++++---------
> fs/ext4/ialloc.c | 12 ++++-----
> fs/ext4/namei.c | 78 +++++++++++++++++++++++++++++++++++++++-----------------
> fs/ext4/resize.c | 8 +++---
> fs/ext4/super.c | 45 ++++++++++++++++----------------
> 6 files changed, 121 insertions(+), 70 deletions(-)
>
>
> +#define EXT4_SB_VALUES(name) \
> +static inline unsigned long ext4_get_##name(struct super_block *sb) \
> +{ \
> + struct ext4_super_block *es = EXT4_SB(sb)->s_es; \
> + unsigned long value = le32_to_cpu(es->s_##name); \
> + return value; \
> +} \
(style) my preference is to have the linefeed escape '\' aligned at
the end of the line, so they don't interfere with the code, but I
see that is inconsistently used
> +static inline void ext4_set_##name(struct super_block *sb,\
> + unsigned long val)\
(style) align continued line after '(' on previous line
> +{ \
> + struct ext4_super_block *es = EXT4_SB(sb)->s_es; \
> + es->s_##name = cpu_to_le32(val); \
> +}
> +
> +EXT4_SB_VALUES(inodes_count)
> +EXT4_SB_VALUES(free_inodes_count)
> +EXT4_SB_VALUES(last_orphan)
> +EXT4_SB_VALUES(first_error_ino)
> +EXT4_SB_VALUES(last_error_ino)
One minor issue with macros like this is that it is not possible to use
tags or grep to find "ext4_{get,set}_inodes_count()" and other generated
function names. It is useful to at least have those function names in
the comments here, something like:
/*
ext4_get_inodes_count(), ext4_set_inodes_count();
*/
EXT4_SB_VALUES(inodes_count);
so that there is at least some chance of finding them. I always have the
same problem with the ext4_has_feature_*() macros as well, and I'd rather
avoid it here.
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index b6681aebe5cf..21f86c48708b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1543,6 +1543,23 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir,
> return bh;
> }
>
> +static int get_ino(struct inode *dir,
> + struct ext4_dir_entry_2 *de, __u32 *ino)
(style) align after '(' on previous line
> +{
> + struct super_block *sb = dir->i_sb;
> +
> + *ino = le32_to_cpu(de->inode);
> + return 0;
> +}
> +
> +static void set_ino(struct inode *dir,
> + struct ext4_dir_entry_2 *de, unsigned long i_ino)
> +{
> + struct super_block *sb = dir->i_sb;
> +
> + de->inode = cpu_to_le32(i_ino);
> +}
"get_ino" and "set_ino" are pretty generic function names. Also, it is
better to put the common components at the start of the name so they can
sort together. Better to use "dirent_ino_{get,set}()" or maybe
"ext4_dirent_ino_{get,set}()"?
> @@ -2772,8 +2792,8 @@ bool ext4_empty_dir(struct inode *inode)
>
> de = (struct ext4_dir_entry_2 *) bh->b_data;
> de1 = ext4_next_entry(de, sb->s_blocksize);
> - if (le32_to_cpu(de->inode) != inode->i_ino ||
> - le32_to_cpu(de1->inode) == 0 ||
> + if (get_ino(inode, de, &ino) || ino != inode->i_ino ||
> + get_ino(inode, de1, &ino2) || ino2 == 0 ||
> strcmp(".", de->name) || strcmp("..", de1->name)) {
(style) this is confusingly indented (the original was as well). Should
be aligned after the first '(' on the "if" line.
> @@ -2943,14 +2963,14 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode)
>
> ino_next = NEXT_ORPHAN(inode);
> if (prev == &sbi->s_orphan) {
> - jbd_debug(4, "superblock will point to %u\n", ino_next);
> + jbd_debug(4, "superblock will point to %lu\n", ino_next);
(defect) this whole patch chunk should probably be part of the next patch,
since ino_next is not yet changed to __u64, and using cpu_to_le64() to
swab a __u32 value below would lead to data corruption on big-endian CPUs.
> BUFFER_TRACE(sbi->s_sbh, "get_write_access");
> err = ext4_journal_get_write_access(handle, sbi->s_sbh);
> if (err) {
> mutex_unlock(&sbi->s_orphan_lock);
> goto out_brelse;
> }
> - sbi->s_es->s_last_orphan = cpu_to_le32(ino_next);
> + ext4_set_last_orphan(inode->i_sb, cpu_to_le64(ino_next));
Also, since ext4_set_last_orphan() is swabbing "value" internally, this is
going to be broken on big-endian machines.
> mutex_unlock(&sbi->s_orphan_lock);
> err = ext4_handle_dirty_super(handle, inode->i_sb);
> } else {
> @@ -2989,6 +3009,7 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
> struct buffer_head *bh;
> struct ext4_dir_entry_2 *de;
> handle_t *handle = NULL;
> + __u32 ino;
>
> if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
> return -EIO;
> @@ -3012,7 +3033,7 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
> inode = d_inode(dentry);
>
> retval = -EFSCORRUPTED;
> - if (le32_to_cpu(de->inode) != inode->i_ino)
> + if (get_ino(dir, de, &ino) || ino != inode->i_ino)
> goto end_rmdir;
>
> retval = -ENOTEMPTY;
> @@ -3392,13 +3414,15 @@ struct ext4_renament {
> static int ext4_rename_dir_prepare(handle_t *handle, struct ext4_renament *ent)
> {
> int retval;
> + __u32 ino;
>
> ent->dir_bh = ext4_get_first_dir_block(handle, ent->inode,
> &retval, &ent->parent_de,
> &ent->dir_inlined);
> if (!ent->dir_bh)
> return retval;
> - if (le32_to_cpu(ent->parent_de->inode) != ent->dir->i_ino)
> + if (get_ino(ent->dir, ent->parent_de, &ino) ||
> + ino != ent->dir->i_ino)
(style) should align after first '(' on previous line
> @@ -3620,7 +3646,8 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
> * same name. Goodbye sticky bit ;-<
> */
> retval = -ENOENT;
> - if (!old.bh || le32_to_cpu(old.de->inode) != old.inode->i_ino)
> + if (!old.bh || get_ino(old.dir, old.de, &ino) ||
> + ino != old.inode->i_ino)
(style) align after first '(' on previous line
> @@ -3834,7 +3862,8 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> * same name. Goodbye sticky bit ;-<
> */
> retval = -ENOENT;
> - if (!old.bh || le32_to_cpu(old.de->inode) != old.inode->i_ino)
> + if (!old.bh || get_ino(old.dir, old.de, &ino) ||
> + ino != old.inode->i_ino)
...
> @@ -3846,7 +3875,8 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> }
>
> /* RENAME_EXCHANGE case: old *and* new must both exist */
> - if (!new.bh || le32_to_cpu(new.de->inode) != new.inode->i_ino)
> + if (!new.bh || get_ino(new.dir, new.de, &ino) ||
> + ino != new.inode->i_ino)
...
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 035cd3f4785e..d0d5acd1a70d 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -1337,10 +1337,10 @@ static void ext4_update_super(struct super_block *sb,
>
> ext4_blocks_count_set(es, ext4_blocks_count(es) + blocks_count);
> ext4_free_blocks_count_set(es, ext4_free_blocks_count(es) + free_blocks);
> - le32_add_cpu(&es->s_inodes_count, EXT4_INODES_PER_GROUP(sb) *
> - flex_gd->count);
> - le32_add_cpu(&es->s_free_inodes_count, EXT4_INODES_PER_GROUP(sb) *
> - flex_gd->count);
> + ext4_set_inodes_count(sb, ext4_get_inodes_count(sb) +
> + EXT4_INODES_PER_GROUP(sb) * flex_gd->count);
> + ext4_set_free_inodes_count(sb, ext4_get_free_inodes_count(sb) +
> + EXT4_INODES_PER_GROUP(sb) * flex_gd->count);
(style) align after '(' on previous line
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ead9406d9cff..455cad8c29e1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -470,7 +470,7 @@ void __ext4_error_inode(struct inode *inode, const char *function,
> if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
> return;
>
> - es->s_last_error_ino = cpu_to_le32(inode->i_ino);
> + ext4_set_last_error_ino(inode->i_sb, cpu_to_le64(inode->i_ino));
(defect) Should be part of next patch that actually converts to 64-bit inodes.
Also, this would be double-swabbing "inode->i_ino", since that is being done
in the macro now (which also avoids the need to change this code to handle
64-bit inodes in your next patch). Should just be:
ext4_set_last_error_ino(inode->i_sb, inode->i_ino);
> es->s_last_error_block = cpu_to_le64(block);
> if (ext4_error_ratelimit(inode->i_sb)) {
> va_start(args, fmt);
> @@ -506,7 +506,7 @@ void __ext4_error_file(struct file *file, const char *function,
> return;
>
> es = EXT4_SB(inode->i_sb)->s_es;
> - es->s_last_error_ino = cpu_to_le32(inode->i_ino);
> + ext4_set_last_error_ino(inode->i_sb, cpu_to_le64(inode->i_ino));
(defect) ...
> @@ -717,7 +717,7 @@ __acquires(bitlock)
> if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
> return;
>
> - es->s_last_error_ino = cpu_to_le32(ino);
> + ext4_set_last_error_ino(sb, cpu_to_le64(ino));
(defect) ...
> @@ -829,8 +829,8 @@ static void dump_orphan_list(struct super_block *sb, struct ext4_sb_info *sbi)
> {
> struct list_head *l;
>
> - ext4_msg(sb, KERN_ERR, "sb orphan head is %d",
> - le32_to_cpu(sbi->s_es->s_last_orphan));
> + ext4_msg(sb, KERN_ERR, "sb orphan head is %llu",
> + le64_to_cpu(ext4_get_last_orphan(sb)));
(defect) ... should just be "ext4_get_last_orphan(sb)" without swab
> @@ -2483,11 +2483,12 @@ static void ext4_orphan_cleanup(struct super_block *sb,
> */
> if (EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS) {
> jbd_debug(1, "Skipping orphan recovery on fs with errors.\n");
> - es->s_last_orphan = 0;
> + ext4_set_last_orphan(sb, 0);
> break;
> }
>
> - inode = ext4_orphan_get(sb, le32_to_cpu(es->s_last_orphan));
> + inode = ext4_orphan_get(sb,
> + le64_to_cpu(ext4_get_last_orphan(sb)));
(defect) ...
> @@ -2811,9 +2812,9 @@ static void print_daily_error_info(unsigned long arg)
> (int) sizeof(es->s_first_error_func),
> es->s_first_error_func,
> le32_to_cpu(es->s_first_error_line));
> - if (es->s_first_error_ino)
> - printk(KERN_CONT ": inode %u",
> - le32_to_cpu(es->s_first_error_ino));
> + if (ext4_get_first_error_ino(sb))
> + printk(KERN_CONT ": inode %llu",
> + le64_to_cpu(ext4_get_first_error_ino(sb)));
(defect) no need for swab
Also, since the inodes are always "unsigned long" you can just change the
format to "%lu" and no need to change it in your next patch.
> @@ -2825,9 +2826,9 @@ static void print_daily_error_info(unsigned long arg)
> (int) sizeof(es->s_last_error_func),
> es->s_last_error_func,
> le32_to_cpu(es->s_last_error_line));
> - if (es->s_last_error_ino)
> - printk(KERN_CONT ": inode %u",
> - le32_to_cpu(es->s_last_error_ino));
> + if (ext4_get_last_error_ino(sb))
> + printk(KERN_CONT ": inode %llu",
> + le64_to_cpu(ext4_get_last_error_ino(sb)));
(defect) ...
> @@ -4705,9 +4706,9 @@ static int ext4_commit_super(struct super_block *sb, int sync)
> EXT4_C2B(EXT4_SB(sb), percpu_counter_sum_positive(
> &EXT4_SB(sb)->s_freeclusters_counter)));
> if (percpu_counter_initialized(&EXT4_SB(sb)->s_freeinodes_counter))
> - es->s_free_inodes_count =
> - cpu_to_le32(percpu_counter_sum_positive(
> - &EXT4_SB(sb)->s_freeinodes_counter));
> + ext4_set_free_inodes_count(sb,
> + cpu_to_le32(percpu_counter_sum_positive(
> + &EXT4_SB(sb)->s_freeinodes_counter)));
(defect) no need for swab
Cheers, Andreas
Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)
Powered by blists - more mailing lists