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

From: Michael Halcrow
Date: Wed Sep 06 2006 - 19:17:56 EST

On Wed, Sep 06, 2006 at 10:44:45AM +0100, David Howells wrote:
> Michael Halcrow <mhalcrow@xxxxxxxxxx> 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@xxxxxxxxxx>

---

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