Re: [PATCH 4/4] eCryptfs: ino_t to u64 for filldir

[Michael Halcrow <mhalcrow@us.ibm.com>]

On Wed, Sep 06, 2006 at 10:44:45AM +0100, David Howells wrote:
> Michael Halcrow <mhalcrow@us.ibm.com> wrote:
> > +int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode)
> > +{
> > +	if (ecryptfs_inode_to_private(inode) && 
> 
> Is this part of the condition actually necessary?  Can you not
> guarantee that this will always be true?

This check is not needed, as far as I can tell. I think I am going to
go the conservative route and convert the check to a BUG_ON() for now.

> > +	ecryptfs_set_inode_lower(inode, igrab((struct inode *)lower_inode));
> 
> igrab() might fail.  I would recommend doing it before calling
> iget5_unlocked() and drop the extraneous reference to lower_inode
> afterwards if the eCryptFS inode returned is already set up.
<snip>
> > +static void ecryptfs_read_inode(struct inode *inode) { }
> 
> You shouldn't need that any more.  Just leave the read_inode op
> pointer unset.  The NULL pointer exception handler will let you know
> if anyone tries to access it (which they shouldn't - only *you*
> should call iget*() on your own inodes).
> 
> Looks good otherwise.  Just the igrab() thing is a real problem.

How's this?

---

Modify inode number generation to account for differences in the inode
number data type sizes in lower filesystems.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/ecryptfs_kernel.h |    3 +++
 fs/ecryptfs/inode.c           |   16 ++++++++++++++++
 fs/ecryptfs/main.c            |   29 +++++++++++++----------------
 fs/ecryptfs/super.c           |   18 ++++++++++--------
 4 files changed, 42 insertions(+), 24 deletions(-)

f260d0b3dad0dadcb8a70b9f3e251a5aae1360f0
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 349ce2a..85660da 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -474,5 +474,8 @@ int ecryptfs_truncate(struct dentry *den
 int
 ecryptfs_process_cipher(struct crypto_tfm **tfm, struct crypto_tfm **key_tfm,
 			char *cipher_name, size_t key_size);
+int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode);
+int ecryptfs_inode_set(struct inode *inode, void *lower_inode);
+void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode);
 
 #endif /* #ifndef ECRYPTFS_KERNEL_H */
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index d8659ff..96b05a6 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -1024,6 +1024,22 @@ out:
 	return rc;
 }
 
+int ecryptfs_inode_test(struct inode *inode, void *candidate_lower_inode)
+{
+	BUG_ON(!ecryptfs_inode_to_private(inode));
+	if ((ecryptfs_inode_to_lower(inode)
+	     == (struct inode *)candidate_lower_inode))
+		return 1;
+	else
+		return 0;
+}
+
+int ecryptfs_inode_set(struct inode *inode, void *lower_inode)
+{
+	ecryptfs_init_inode(inode, (struct inode *)lower_inode);
+	return 0;
+}
+
 struct inode_operations ecryptfs_symlink_iops = {
 	.readlink = ecryptfs_readlink,
 	.follow_link = ecryptfs_follow_link,
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index f7ea912..ecf9faf 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -68,39 +68,36 @@ void __ecryptfs_printk(const char *fmt, 
  *
  * Interposes upper and lower dentries.
  *
- * This function will allocate an ecryptfs_inode through the call to
- * iget(sb, lower_inode->i_ino).
- *
  * Returns zero on success; non-zero otherwise
  */
 int ecryptfs_interpose(struct dentry *lower_dentry, struct dentry *dentry,
 		       struct super_block *sb, int flag)
 {
 	struct inode *lower_inode;
-	int rc = 0;
 	struct inode *inode;
+	int rc = 0;
 
 	lower_inode = lower_dentry->d_inode;
 	if (lower_inode->i_sb != ecryptfs_superblock_to_lower(sb)) {
 		rc = -EXDEV;
 		goto out;
 	}
-	inode = iget(sb, lower_inode->i_ino);
-	if (!inode) {
-		rc = -EACCES;
+	if (!igrab(lower_inode)) {
+		rc = -ESTALE;
 		goto out;
 	}
-	/* This check is required here because if we failed to allocated the
-	 * required space for an inode_info_cache struct, then the only way
-	 * we know we failed, is by the pointer being NULL */
-	if (!ecryptfs_inode_to_private(inode)) {
-		ecryptfs_printk(KERN_ERR, "Out of memory. Failure to "
-				"allocate memory in ecryptfs_read_inode.\n");
-		rc = -ENOMEM;
+	inode = iget5_locked(sb, (unsigned long)lower_inode,
+			     ecryptfs_inode_test, ecryptfs_inode_set,
+			     lower_inode);
+	if (!inode) {
+		rc = -EACCES;
+		iput(lower_inode);
 		goto out;
 	}
-	if (!ecryptfs_inode_to_lower(inode))
-		ecryptfs_set_inode_lower(inode, igrab(lower_inode));
+	if (inode->i_state & I_NEW)
+		unlock_new_inode(inode);
+	else
+		iput(lower_inode);
 	if (S_ISLNK(lower_inode->i_mode))
 		inode->i_op = &ecryptfs_symlink_iops;
 	else if (S_ISDIR(lower_inode->i_mode))
diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
index f4f06ea..9edadac 100644
--- a/fs/ecryptfs/super.c
+++ b/fs/ecryptfs/super.c
@@ -78,19 +78,22 @@ static void ecryptfs_destroy_inode(struc
 }
 
 /**
- * ecryptfs_read_inode
+ * ecryptfs_init_inode
  * @inode: The ecryptfs inode
  *
  * Set up the ecryptfs inode.
  */
-static void ecryptfs_read_inode(struct inode *inode)
+void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
 {
-	/* This is where we setup the self-reference in the vfs_inode's
-	 * i_private. That way we don't have to walk the list again. */
+	/* This is where we setup the self-reference in the
+	 * vfs_inode's i_private. That way we don't have to walk the
+	 * list again. */
 	ecryptfs_set_inode_private(inode,
-				   list_entry(inode, struct ecryptfs_inode_info,
-					      vfs_inode));
-	ecryptfs_set_inode_lower(inode, NULL);
+				   container_of(inode,
+						struct ecryptfs_inode_info,
+						vfs_inode));
+	ecryptfs_set_inode_lower(inode, lower_inode);
+	inode->i_ino = lower_inode->i_ino;
 	inode->i_version++;
 	inode->i_op = &ecryptfs_main_iops;
 	inode->i_fop = &ecryptfs_main_fops;
@@ -192,7 +195,6 @@ out:
 struct super_operations ecryptfs_sops = {
 	.alloc_inode = ecryptfs_alloc_inode,
 	.destroy_inode = ecryptfs_destroy_inode,
-	.read_inode = ecryptfs_read_inode,
 	.drop_inode = generic_delete_inode,
 	.put_super = ecryptfs_put_super,
 	.statfs = ecryptfs_statfs,
-- 
1.3.3

Attachment: signature.asc
Description: Digital signature