by David Howells

[permalink] [raw]

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

Michael Halcrow <[email protected]> wrote:

> Is it safe to assume that a pointer will always be equal to or smaller
> than an unsigned long for all architectures?

I presume you're talking about the sizes of the types. Inside the Linux
kernel:

sizeof(void *) == sizeof(unsigned long)

must always hold true. There are too many things that depend on it to do
otherwise.

> Casting a pointer to an unsigned long, in general, makes me a bit
> uncomfortable.

Pointers are just numbers that are used in a particular way. Go and look in
linux/err.h:-)

> Dave Chinner told me that XFS uses 32-bit inode numbers on 32-bit
> machines, so I imagine that this patch really is only helpful for NFS.

At the moment, maybe, but they could always change it. And what about Reiser4?

> +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?

> + 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.

You're also casting lower_inode twice. Whilst there's nothing actually wrong
with that, it might look better if you assigned it to its own variable at the
top of the function and only do the cast once.

> +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.

David

2006-09-06 23:20:53

by Michael Halcrow

[permalink] [raw]

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

On Wed, Sep 06, 2006 at 10:44:45AM +0100, David Howells wrote:
> Michael Halcrow <[email protected]> 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 <[email protected]>

---

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

Attachments:

(No filename) (6.04 kB)
signature.asc (481.00 B)
Digital signature Download all attachments

2006-09-07 10:28:24

by David Howells

[permalink] [raw]

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

Michael Halcrow <[email protected]> wrote:

> How's this?

Looks good.

Acked-By: David Howells <[email protected]>