linux-kernel - Re: [Ecryptfs-devel] [PATCH] ecryptfs: some inode attrs, and a question

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: <1232215916.7015.78.camel@norville.austin.ibm.com>
Date:	Sat, 17 Jan 2009 18:11:56 +0000
From:	Dave Kleikamp <shaggy@...ux.vnet.ibm.com>
To:	hooanon05@...oo.co.jp
Cc:	Tyler Hicks <tyhicks@...ux.vnet.ibm.com>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	linux-kernel@...r.kernel.org, ecryptfs-devel@...ts.launchpad.net
Subject: Re: [Ecryptfs-devel] [PATCH] ecryptfs: some inode attrs, and a
 question

On Sun, 2009-01-18 at 02:42 +0900, hooanon05@...oo.co.jp wrote:
> Dave Kleikamp:
> > Does this function make sense (un-compiled, un-tested)?
> > 
> > void ecryptfs_update_inode_from_lower(struct dentry *dentry)
> > {
> > 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> > 
> > 	if (d_unhashed(lower_dentry))
> > 		d_drop(dentry);
> > 	else {
> > 		struct inode *inode = dentry->d_inode;
> > 		struct inode *lower_inode = lower_dentry->d_inode;
> > 
> > 		inode->i_nlink = lower_inode->i_nlink;
> > 		inode->i_ctime = lower_inode->i_ctime;
> > 		/* Should anything else go here ? */
> > 	}
> > }
> 
> Yes, testing by d_unhashed() is best, I think.
> But all inode attributes are maintained by ecryptfs_interpose(), so you
> don't need to handle them.

maybe ecryptfs_interpose() should be checking d_unhashed().

> And I found another bug in ecryptfs_link().
> Here is a patch including that.
> 
> 
> J. R. Okajima
> 
> ----------------------------------------------------------------------
> refine ecryptfs_link()
> 
> ecryptfs_link() doesn't need to,,,
> - copy i_nlink and i_size for the target inode since they are already
>   handled by ecryptfs_interpose(). (I don't know why i_size was
>   necessary)

For a regular file, the size of the upper inode is not the same as the
size of the lower inode.  The lower inode includes the header blocks
which are not visible in the upper inode.  So ecryptfs_interpose() will
overwrite the correct upper inode size.

> - maintain dir attributes based upon the lower dir, instead of
>   lower_new_dentry. (another bug)
> - several d_drop()s. do it only when the lower_new_dentry is d_drop()ed
>   by the lower fs or vfs_link() failed.
> - testing lower_new_dentry->d_inode after vfs_link() succeeded.
> 
> Signed-off-by: J. R. Okajima <hooanon05@...oo.co.jp>
> 
> diff -u -p -r1.1 inode.c
> --- inode.c	19 Dec 2008 03:05:27 -0000	1.1
> +++ inode.c	17 Jan 2009 17:26:53 -0000
> @@ -405,10 +407,8 @@ static int ecryptfs_link(struct dentry *
>  	struct dentry *lower_old_dentry;
>  	struct dentry *lower_new_dentry;
>  	struct dentry *lower_dir_dentry;
> -	u64 file_size_save;
>  	int rc;
> 
> -	file_size_save = i_size_read(old_dentry->d_inode);
>  	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
>  	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
>  	dget(lower_old_dentry);
> @@ -416,23 +416,23 @@ static int ecryptfs_link(struct dentry *
>  	lower_dir_dentry = lock_parent(lower_new_dentry);
>  	rc = vfs_link(lower_old_dentry, lower_dir_dentry->d_inode,
>  		      lower_new_dentry);
> -	if (rc || !lower_new_dentry->d_inode)
> -		goto out_lock;
> -	rc = ecryptfs_interpose(lower_new_dentry, new_dentry, dir->i_sb, 0);
> -	if (rc)
> -		goto out_lock;
> -	fsstack_copy_attr_times(dir, lower_new_dentry->d_inode);
> -	fsstack_copy_inode_size(dir, lower_new_dentry->d_inode);
> -	old_dentry->d_inode->i_nlink =
> -		ecryptfs_inode_to_lower(old_dentry->d_inode)->i_nlink;
> -	i_size_write(new_dentry->d_inode, file_size_save);
> -out_lock:
> +	if (!rc) {
> +		fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
> +		fsstack_copy_inode_size(dir, lower_dir_dentry->d_inode);
> +		if (!d_unhashed(lower_new_dentry)
> +		    /* && lower_new_dentry->d_inode */) {
> +			rc = ecryptfs_interpose(lower_new_dentry, new_dentry,
> +						dir->i_sb, 0);
> +			if (!rc)
> +				goto out_lock; /* success */
> +		}
> +	}
> + out_drop:
> +	d_drop(new_dentry);
> + out_lock:
>  	unlock_dir(lower_dir_dentry);
>  	dput(lower_new_dentry);
>  	dput(lower_old_dentry);
> -	d_drop(lower_old_dentry);
> -	d_drop(new_dentry);
> -	d_drop(old_dentry);
>  	return rc;
>  }
> 
-- 
David Kleikamp
IBM Linux Technology Center

--
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/