linux-kernel - Re: [PATCH 11/11] ext4: add cross rename support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131126095125.GB1384@quack.suse.cz>
Date:	Tue, 26 Nov 2013 10:51:25 +0100
From:	Jan Kara <jack@...e.cz>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	viro@...IV.linux.org.uk, torvalds@...ux-foundation.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	hch@...radead.org, akpm@...ux-foundation.org, dhowells@...hat.com,
	zab@...hat.com, jack@...e.cz, luto@...capital.net, mszeredi@...e.cz
Subject: Re: [PATCH 11/11] ext4: add cross rename support

On Wed 20-11-13 14:01:52, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@...e.cz>
> 
> Implement RENAME_EXCHANGE flag in renameat2 syscall.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@...e.cz>
> ---
>  fs/ext4/namei.c | 97 ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 69 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index d258b354b937..5307e482f403 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
...
> -	old.dir->i_ctime = old.dir->i_mtime = ext4_current_time(old.dir);
> -	ext4_update_dx_flag(old.dir);
> +	/* S_ISDIR(old.inode->i_mode */
>  	if (old.dir_bh) {
>  		retval = ext4_rename_dir_finish(handle, &old, new.dir->i_ino);
>  		if (retval)
>  			goto end_rename;
>  
> -		ext4_dec_count(handle, old.dir);
> -		if (new.inode) {
> -			/* checked empty_dir above, can't have another parent,
> -			 * ext4_dec_count() won't work for many-linked dirs */
> -			clear_nlink(new.inode);
> -		} else {
> +		if (!(flags & RENAME_EXCHANGE) || !S_ISDIR(new.inode->i_mode))
> +			ext4_dec_count(handle, old.dir);
> +
> +		if (!new.inode || !S_ISDIR(new.inode->i_mode)) {
>  			ext4_inc_count(handle, new.dir);
>  			ext4_update_dx_flag(new.dir);
>  			ext4_mark_inode_dirty(handle, new.dir);
>  		}
>  	}
> +	/* (flags & RENAME_EXCHANGE) && S_ISDIR(new.inode->i_mode */
> +	if (new.dir_bh) {
> +		retval = ext4_rename_dir_finish(handle, &new, old.dir->i_ino);
> +		if (retval)
> +			goto end_rename;
> +
> +		if (!S_ISDIR(old.inode->i_mode)) {
> +			ext4_dec_count(handle, new.dir);
> +			ext4_inc_count(handle, old.dir);
> +			ext4_mark_inode_dirty(handle, new.dir);
> +		}
> +	}
>  	ext4_mark_inode_dirty(handle, old.dir);
> -	if (new.inode) {
> +	if (!(flags & RENAME_EXCHANGE) && new.inode) {
> +		ext4_dec_count(handle, new.inode);
> +		new.inode->i_ctime = ext4_current_time(new.inode);
> +		if (S_ISDIR(old.inode->i_mode)) {
> +			/* checked empty_dir above, can't have another parent,
> +			 * ext4_dec_count() won't work for many-linked dirs */
> +			clear_nlink(new.inode);
> +		}
  This hunk looks strange. Why do you check S_ISDIR(old.inode->i_mode)? I'd
presume we need to clear nlink if new.inode is a directory...

>  		ext4_mark_inode_dirty(handle, new.inode);
>  		if (!new.inode->i_nlink)
> 			ext4_orphan_add(handle, new.inode);
  Generally, I'm a bit unhappy about the number of various RENAME_EXCHANGE
checks and the asymmetry between new & old which now shouldn't needed (that
much). Especially the link count handling looks more complex than it should
be.

I'd hope that it should be possible to "delete new.inode iff
!RENAME_EXCHANGE" and then the rest shouldn't need to care about
RENAME_EXCHANGE at all and treat old & new completely symmetrically... Now I
realize this isn't that easy because we want to do all error checks first
before doing any changes on disk but still I hope some improvement can be
made (maybe just zero out new.inode in our 'new' ext4_renament to allow for
code to be symmetric and delete it on disk only when ext4_rename() is
finishing).

If the above won't be workable, we might at least make the link count
handling more obvious by computing "old_link_cnt_update,
new_link_cnt_update" - how link counts of parent dirs should be updated
(-1, 0, +1) and then do the checks and updates based on this in one place.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/