Re: ecryptfs

Re: ecryptfs

[Michael Halcrow]

***DO NOT ACCEPT THIS PATCH***

On Mon, Aug 07, 2006 at 11:19:39AM -0500, Michael Halcrow wrote:
> I have a couple more bogus patches I would like feedback on; they're
> bogus at this stage because I am struggling to get them to work
> right. I'll send them as a follow-on to this message.

***DO NOT ACCEPT THIS PATCH***

This code is for commentary only at this stage. Note that it applies
on top of a patch that associates the vfsmount with the dentry rather
than the superblock.

Here is where I am thinking about going with crossing lower mount
points. This patch makes sure that there is a 1-to-1 mapping in inode
numbers between the stacked inodes and the lower inodes. It maintains
the association by modifying the struct inode to include a back
pointer from the lower inode to the stacked inode. Note that Unionfs
has a persistent inode approach that is much more heavy-weight than
this approach, but it does not need to modify anything in the VFS.

I would like the CONFIG_STACK_FS to govern VFS modifications that make
stackable filesystems more functional. My approach is to allow the
kernel to be built without any such modifications, but certain
features will be disabled/degraded in the stacked filesystem.

Unfortunately, with this patch, things blow up on umount; I'll have to
investigate what's going on.

diff --git a/fs/Kconfig b/fs/Kconfig
index afacf48..30c78b9 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -443,6 +443,14 @@ config QUOTA
 	  with the quota tools. Probably the quota support is only useful for
 	  multi user systems. If unsure, say N.
 
+config STACK_FS
+	bool "Stacked filesystem support"
+	default y
+	help
+	  Select Y to enable all features in stacked filesystems (such
+	  as eCryptfs and Unionfs).  Slightly increases the size of
+	  inodes in memory.  If unsure, say Y.
+
 config QFMT_V1
 	tristate "Old quota format support"
 	depends on QUOTA
@@ -984,6 +992,9 @@ config ECRYPT_FS
 	  Encrypted filesystem that operates on the VFS layer.  See
 	  Documentation/ecryptfs.txt to learn more about eCryptfs.
 
+	  It is strongly recommended that you also enable ``Stacked
+	  filesystem support'' (CONFIG_STACK_FS) if you build eCryptfs.
+
 	  To compile this file system support as a module, choose M here: the
 	  module will be called ecryptfs.
 
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 349ce2a..3e8f377 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -376,6 +376,51 @@ ecryptfs_set_dentry_lower_mnt(struct den
 		lower_mnt;
 }
 
+#ifdef CONFIG_STACK_FS
+static inline int
+ecryptfs_get_inode(struct inode **inode, struct inode *lower_inode,
+		   struct super_block *sb)
+{
+	*inode = lower_inode->i_stacked_inode;
+	if (!*inode) {
+		*inode = iget(sb, iunique(sb, 0));
+		if (!*inode)
+			return -EACCES;
+		lower_inode->i_stacked_inode = *inode;
+	}
+	return 0;
+}
+#else
+static inline int
+ecryptfs_get_inode(struct inode **inode, struct inode *lower_inode,
+		   struct super_block *sb)
+{
+	/* Without CONFIG_STACK_FS enabled, we have no way to safely
+	 * cross mount points in the lower filesystem. */
+	if (lower_inode->i_sb != ecryptfs_superblock_to_lower(sb)) {
+		printk(KERN_ERR "Cannot cross mount point in lower filesystem; "
+		       "recompile your kernel with CONFIG_STACK_FS to enable "
+		       "this feature.\n");
+		return -EXDEV;
+	}
+	*inode = iget(sb, lower_inode->i_ino);
+	if (!*inode)
+		return -EACCES;
+	return 0;
+}
+#endif /* CONFIG_STACK_FS */
+
+#ifdef CONFIG_STACK_FS
+static inline void ecryptfs_clear_inode_stacking(struct inode *inode)
+{
+	struct inode *lower_inode = ecryptfs_inode_to_lower(inode);
+
+	lower_inode->i_stacked_inode = NULL;
+}
+#else
+static inline void ecryptfs_clear_inode_stacking(struct inode *inode) { }
+#endif /* CONFIG_STACK_FS */
+
 #define ecryptfs_printk(type, fmt, arg...) \
         __ecryptfs_printk(type "%s: " fmt, __FUNCTION__, ## arg);
 void __ecryptfs_printk(const char *fmt, ...);
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index d2e1cb0..119c0a5 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -77,22 +77,16 @@ int ecryptfs_interpose(struct dentry *lo
 		       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;
+	rc = ecryptfs_get_inode(&inode, lower_inode, sb);
+	if (rc) {
+		printk(KERN_ERR "Error getting inode\n");
 		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 */
+	/* Did we succeed in allocating ecryptfs_inode_info? */
 	if (!ecryptfs_inode_to_private(inode)) {
 		ecryptfs_printk(KERN_ERR, "Out of memory. Failure to "
 				"allocate memory in ecryptfs_read_inode.\n");
diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
index 120f060..d96aa65 100644
--- a/fs/ecryptfs/super.c
+++ b/fs/ecryptfs/super.c
@@ -138,6 +138,7 @@ static int ecryptfs_statfs(struct dentry
  */
 static void ecryptfs_clear_inode(struct inode *inode)
 {
+	ecryptfs_clear_inode_stacking(inode);
 	iput(ecryptfs_inode_to_lower(inode));
 }
 
diff --git a/fs/inode.c b/fs/inode.c
index 89ce5ba..3d3efc2 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -163,7 +163,10 @@ #endif
 				bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
 			mapping->backing_dev_info = bdi;
 		}
-		inode->i_private = 0;
+		inode->i_private = NULL;
+#ifdef CONFIG_STACK_FS
+		inode->i_stacked_inode = NULL;
+#endif
 		inode->i_mapping = mapping;
 	}
 	return inode;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6cd7004..306a028 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -578,6 +578,9 @@ #endif
 	atomic_t		i_writecount;
 	void			*i_security;
 	void			*i_private; /* fs or device private pointer */
+#ifdef CONFIG_STACK_FS
+	struct inode		*i_stacked_inode; /* stackable fs support */
+#endif
 #ifdef __NEED_I_SIZE_ORDERED
 	seqcount_t		i_size_seqcount;
 #endif

Re: ecryptfs

[Josef Sipek]

On Mon, Aug 07, 2006 at 11:31:15AM -0500, Michael Halcrow wrote:
...
> Here is where I am thinking about going with crossing lower mount
> points. This patch makes sure that there is a 1-to-1 mapping in inode
> numbers between the stacked inodes and the lower inodes. It maintains
> the association by modifying the struct inode to include a back
> pointer from the lower inode to the stacked inode.

Do you maintain the inode numbers across mounts (of ecryptfs)? The patch
doesn't look like it does.

> Note that Unionfs
> has a persistent inode approach that is much more heavy-weight than
> this approach, but it does not need to modify anything in the VFS.

I'd just like to point out, that we'd (Unionfs folks) love to see a solution
at the VFS level.

> I would like the CONFIG_STACK_FS to govern VFS modifications that make
> stackable filesystems more functional. My approach is to allow the kernel
> to be built without any such modifications, but certain features will be
> disabled/degraded in the stacked filesystem.
 
Makes sense.

> +		inode->i_private = NULL;
> +#ifdef CONFIG_STACK_FS
> +		inode->i_stacked_inode = NULL;
> +#endif
>  		inode->i_mapping = mapping;

Hrm. Looking at this made me think...This patch introduces pointers up the
stack in the VFS. Would it make sense to introduce the down pointers as
well, and make ecryptfs, et. al., depend on STACK_FS? This would clean up
some of the use of private data. Of course these pointers (the up & down)
make struct inode grow 8 bytes (on 32-bit systems).

Josef "Jeff" Sipek.

-- 
Intellectuals solve problems; geniuses prevent them
		- Albert Einstein

Re: ecryptfs

[Michael Halcrow]

On Mon, Aug 07, 2006 at 01:21:04PM -0400, Josef Sipek wrote:
> On Mon, Aug 07, 2006 at 11:31:15AM -0500, Michael Halcrow wrote:
> ...
> > Here is where I am thinking about going with crossing lower mount
> > points. This patch makes sure that there is a 1-to-1 mapping in
> > inode numbers between the stacked inodes and the lower inodes. It
> > maintains the association by modifying the struct inode to include
> > a back pointer from the lower inode to the stacked inode.
> 
> Do you maintain the inode numbers across mounts (of ecryptfs)? The
> patch doesn't look like it does.

Nope; this patch just aims to make sure that stacked and lower inodes
maintain a 1-to-1 relationship.

> > +		inode->i_private = NULL;
> > +#ifdef CONFIG_STACK_FS
> > +		inode->i_stacked_inode = NULL;
> > +#endif
> >  		inode->i_mapping = mapping;
> 
> Hrm. Looking at this made me think...This patch introduces pointers
> up the stack in the VFS. Would it make sense to introduce the down
> pointers as well, and make ecryptfs, et. al., depend on STACK_FS?
> This would clean up some of the use of private data. Of course these
> pointers (the up & down) make struct inode grow 8 bytes (on 32-bit
> systems).

I would say that using the existing private data pointer is a better
option than consuming more space in each and every inode struct.

Mike

Re: ecryptfs

[Shaya Potter]

On Mon, 2006-08-07 at 17:47 -0500, Michael Halcrow wrote:
> On Mon, Aug 07, 2006 at 01:21:04PM -0400, Josef Sipek wrote:
> > On Mon, Aug 07, 2006 at 11:31:15AM -0500, Michael Halcrow wrote:
> > ...
> > > Here is where I am thinking about going with crossing lower mount
> > > points. This patch makes sure that there is a 1-to-1 mapping in
> > > inode numbers between the stacked inodes and the lower inodes. It
> > > maintains the association by modifying the struct inode to include
> > > a back pointer from the lower inode to the stacked inode.
> > 
> > Do you maintain the inode numbers across mounts (of ecryptfs)? The
> > patch doesn't look like it does.
> 
> Nope; this patch just aims to make sure that stacked and lower inodes
> maintain a 1-to-1 relationship.

so that won't let your cross mount points.... i.e. the 2 underlying
mountpoints can have the same inode numbers, couldn't they?

Re: ecryptfs

[Michael Halcrow]

On Mon, Aug 07, 2006 at 08:13:10PM -0400, Shaya Potter wrote:
> On Mon, 2006-08-07 at 17:47 -0500, Michael Halcrow wrote:
> > On Mon, Aug 07, 2006 at 01:21:04PM -0400, Josef Sipek wrote:
> > > On Mon, Aug 07, 2006 at 11:31:15AM -0500, Michael Halcrow wrote:
> > > ...
> > > > Here is where I am thinking about going with crossing lower mount
> > > > points. This patch makes sure that there is a 1-to-1 mapping in
> > > > inode numbers between the stacked inodes and the lower inodes. It
> > > > maintains the association by modifying the struct inode to include
> > > > a back pointer from the lower inode to the stacked inode.
> > > 
> > > Do you maintain the inode numbers across mounts (of ecryptfs)? The
> > > patch doesn't look like it does.
> > 
> > Nope; this patch just aims to make sure that stacked and lower inodes
> > maintain a 1-to-1 relationship.
> 
> so that won't let your cross mount points.... i.e. the 2 underlying
> mountpoints can have the same inode numbers, couldn't they?

Two underlying mountpoints can have the sane inode number. This patch
will assign a unique inode number to each stacked inode while linking
to the stacked inode from the lower inode to keep a 1-to-1 mapping of
stacked and lower inodes. Something like this is necessary for
crossing mount points in the lower filesystem while handling hard
links right. There are other issues with regard to namespaces and what
not that still need to be addressed.

Mike

Re: ecryptfs

[Shaya Potter]

Michael Halcrow wrote:
> On Mon, Aug 07, 2006 at 08:13:10PM -0400, Shaya Potter wrote:
>> On Mon, 2006-08-07 at 17:47 -0500, Michael Halcrow wrote:
>>> On Mon, Aug 07, 2006 at 01:21:04PM -0400, Josef Sipek wrote:
>>>> On Mon, Aug 07, 2006 at 11:31:15AM -0500, Michael Halcrow wrote:
>>>> ...
>>>>> Here is where I am thinking about going with crossing lower mount
>>>>> points. This patch makes sure that there is a 1-to-1 mapping in
>>>>> inode numbers between the stacked inodes and the lower inodes. It
>>>>> maintains the association by modifying the struct inode to include
>>>>> a back pointer from the lower inode to the stacked inode.
>>>> Do you maintain the inode numbers across mounts (of ecryptfs)? The
>>>> patch doesn't look like it does.
>>> Nope; this patch just aims to make sure that stacked and lower inodes
>>> maintain a 1-to-1 relationship.
>> so that won't let your cross mount points.... i.e. the 2 underlying
>> mountpoints can have the same inode numbers, couldn't they?
> 
> Two underlying mountpoints can have the sane inode number. This patch
> will assign a unique inode number to each stacked inode while linking
> to the stacked inode from the lower inode to keep a 1-to-1 mapping of
> stacked and lower inodes. Something like this is necessary for
> crossing mount points in the lower filesystem while handling hard
> links right. There are other issues with regard to namespaces and what
> not that still need to be addressed.

ok, I understand, your mention of unionfs's persistent inodes confused me.