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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux - Powered by OpenVZ