Here are several fixes for linux-2.6.27/fs/ecryptfs.
- The ecryptfs inode holds a reference to the lower inode, but doesn't
increment the reference counter. When a user sets inotify to the
ecryptfs inode, it may live without the corresponding dentry. In this
case the referecen to the lower inode may be broken.
This patch maintains the reference of the lower inode.
- follow the VFS unlink sequence in ecryptfs_unlink() which is
inrementing and decrementing the inode->i_count and the reference
counter for the dentry.
- maintain the link count and ctime in ecryptfs_rmdir() because a user
may issue fstat(2) later.
- remove the unnecessary d_drop()s in ecryptfs_link().
And I have experienced a strange behaviour. When ecryptfs gets -ENOSPC
from the lower fs, it converts and returns EINVAL to the userspace. Is
this an intended behaviour?
J. R. Okajima
Index: linux-2.6.27/fs/ecryptfs/inode.c
===================================================================
retrieving revision 1.1
retrieving revision 1.2
diff -u -p -r1.1 -r1.2
--- linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 03:05:27 -0000 1.1
+++ linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 19:52:26 -0000 1.2
@@ -430,9 +430,6 @@ 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;
}
@@ -444,7 +441,10 @@ static int ecryptfs_unlink(struct inode
struct dentry *lower_dir_dentry;
lower_dir_dentry = lock_parent(lower_dentry);
+ dget(lower_dentry);
+ atomic_inc_return(&lower_dentry->d_inode->i_count);
rc = vfs_unlink(lower_dir_inode, lower_dentry);
+ dput(lower_dentry);
if (rc) {
printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
goto out_unlock;
@@ -455,6 +455,7 @@ static int ecryptfs_unlink(struct inode
dentry->d_inode->i_ctime = dir->i_ctime;
d_drop(dentry);
out_unlock:
+ iput(lower_dentry->d_inode);
unlock_dir(lower_dir_dentry);
return rc;
}
@@ -538,8 +539,12 @@ static int ecryptfs_rmdir(struct inode *
fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
dir->i_nlink = lower_dir_dentry->d_inode->i_nlink;
unlock_dir(lower_dir_dentry);
- if (!rc)
+ if (!rc) {
+ struct inode *inode = dentry->d_inode;
+ inode->i_nlink = ecryptfs_inode_to_lower(inode)->i_nlink;
+ inode->i_ctime = dir->i_ctime;
d_drop(dentry);
+ }
dput(dentry);
return rc;
}
Index: linux-2.6.27/fs/ecryptfs/super.c
===================================================================
retrieving revision 1.1
retrieving revision 1.2
diff -u -p -r1.1 -r1.2
--- linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 04:37:43 -0000 1.1
+++ linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 19:52:26 -0000 1.2
@@ -89,6 +89,7 @@ static void ecryptfs_destroy_inode(struc
}
}
mutex_unlock(&inode_info->lower_file_mutex);
+ iput(inode_info->wii_inode);
ecryptfs_destroy_crypt_stat(&inode_info->crypt_stat);
kmem_cache_free(ecryptfs_inode_info_cache, inode_info);
}
@@ -101,6 +102,7 @@ static void ecryptfs_destroy_inode(struc
*/
void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
{
+ atomic_inc_return(&lower_inode->i_count);
ecryptfs_set_inode_lower(inode, lower_inode);
inode->i_ino = lower_inode->i_ino;
inode->i_version++;
cc'ing ecryptfs-devel. mhalcrow has moved on to the dark side^W^W^W
another company.
Shaggy
On Tue, 2009-01-13 at 15:20 +0900, [email protected] wrote:
> Here are several fixes for linux-2.6.27/fs/ecryptfs.
>
> - The ecryptfs inode holds a reference to the lower inode, but doesn't
> increment the reference counter. When a user sets inotify to the
> ecryptfs inode, it may live without the corresponding dentry. In this
> case the referecen to the lower inode may be broken.
> This patch maintains the reference of the lower inode.
>
> - follow the VFS unlink sequence in ecryptfs_unlink() which is
> inrementing and decrementing the inode->i_count and the reference
> counter for the dentry.
>
> - maintain the link count and ctime in ecryptfs_rmdir() because a user
> may issue fstat(2) later.
>
> - remove the unnecessary d_drop()s in ecryptfs_link().
>
> And I have experienced a strange behaviour. When ecryptfs gets -ENOSPC
> from the lower fs, it converts and returns EINVAL to the userspace. Is
> this an intended behaviour?
>
>
> J. R. Okajima
>
> Index: linux-2.6.27/fs/ecryptfs/inode.c
> ===================================================================
> retrieving revision 1.1
> retrieving revision 1.2
> diff -u -p -r1.1 -r1.2
> --- linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 03:05:27 -0000 1.1
> +++ linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 19:52:26 -0000 1.2
> @@ -430,9 +430,6 @@ 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;
> }
>
> @@ -444,7 +441,10 @@ static int ecryptfs_unlink(struct inode
> struct dentry *lower_dir_dentry;
>
> lower_dir_dentry = lock_parent(lower_dentry);
> + dget(lower_dentry);
> + atomic_inc_return(&lower_dentry->d_inode->i_count);
> rc = vfs_unlink(lower_dir_inode, lower_dentry);
> + dput(lower_dentry);
> if (rc) {
> printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
> goto out_unlock;
> @@ -455,6 +455,7 @@ static int ecryptfs_unlink(struct inode
> dentry->d_inode->i_ctime = dir->i_ctime;
> d_drop(dentry);
> out_unlock:
> + iput(lower_dentry->d_inode);
> unlock_dir(lower_dir_dentry);
> return rc;
> }
> @@ -538,8 +539,12 @@ static int ecryptfs_rmdir(struct inode *
> fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
> dir->i_nlink = lower_dir_dentry->d_inode->i_nlink;
> unlock_dir(lower_dir_dentry);
> - if (!rc)
> + if (!rc) {
> + struct inode *inode = dentry->d_inode;
> + inode->i_nlink = ecryptfs_inode_to_lower(inode)->i_nlink;
> + inode->i_ctime = dir->i_ctime;
> d_drop(dentry);
> + }
> dput(dentry);
> return rc;
> }
> Index: linux-2.6.27/fs/ecryptfs/super.c
> ===================================================================
> retrieving revision 1.1
> retrieving revision 1.2
> diff -u -p -r1.1 -r1.2
> --- linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 04:37:43 -0000 1.1
> +++ linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 19:52:26 -0000 1.2
> @@ -89,6 +89,7 @@ static void ecryptfs_destroy_inode(struc
> }
> }
> mutex_unlock(&inode_info->lower_file_mutex);
> + iput(inode_info->wii_inode);
> ecryptfs_destroy_crypt_stat(&inode_info->crypt_stat);
> kmem_cache_free(ecryptfs_inode_info_cache, inode_info);
> }
> @@ -101,6 +102,7 @@ static void ecryptfs_destroy_inode(struc
> */
> void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
> {
> + atomic_inc_return(&lower_inode->i_count);
> ecryptfs_set_inode_lower(inode, lower_inode);
> inode->i_ino = lower_inode->i_ino;
> inode->i_version++;
--
David Kleikamp
IBM Linux Technology Center
Dave Kleikamp wrote:
> cc'ing ecryptfs-devel. mhalcrow has moved on to the dark side^W^W^W
> another company.
>
> Shaggy
>
> On Tue, 2009-01-13 at 15:20 +0900, [email protected] wrote:
>> Here are several fixes for linux-2.6.27/fs/ecryptfs.
>>
>> - The ecryptfs inode holds a reference to the lower inode, but doesn't
>> increment the reference counter. When a user sets inotify to the
>> ecryptfs inode, it may live without the corresponding dentry. In this
>> case the referecen to the lower inode may be broken.
>> This patch maintains the reference of the lower inode.
Is this a problem that you've experienced or something you found during
a code review? How can it be reproduced? We get a reference to the
lower inode with the igrab() in ecryptfs_interpose() and put it back in
ecryptfs_clear_inode(). This part of the patch seems to just increment
the ref counts again.
>>
>> - follow the VFS unlink sequence in ecryptfs_unlink() which is
>> inrementing and decrementing the inode->i_count and the reference
>> counter for the dentry.
I see that do_unlinkat does this, but since eCryptfs already holds these
references, I don't think that we need to do it here.
>>
>> - maintain the link count and ctime in ecryptfs_rmdir() because a user
>> may issue fstat(2) later.
This part of the patch is valid, nice catch!
>>
>> - remove the unnecessary d_drop()s in ecryptfs_link().
I tend to agree that they look unnecessary, but one of them was added
for cifs (see ae56fb16) and the other two seem to be intentional (see
45ec4aba) as well. I'm working on adding support for networked
filesystems, I'll keep these d_drop()s in mind and see if we can drop
them. :)
>>
>> And I have experienced a strange behaviour. When ecryptfs gets -ENOSPC
>> from the lower fs, it converts and returns EINVAL to the userspace. Is
>> this an intended behaviour?
This is a bug. Check out ecryptfs_write_lower(), we discard the return
value of vfs_write() and return -EINVAL. I'm betting this is the
problem, I'll get a patch out for this. Thanks for reporting it.
>>
>>
>> J. R. Okajima
>>
>> Index: linux-2.6.27/fs/ecryptfs/inode.c
>> ===================================================================
>> retrieving revision 1.1
>> retrieving revision 1.2
>> diff -u -p -r1.1 -r1.2
>> --- linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 03:05:27 -0000 1.1
>> +++ linux-2.6.27/fs/ecryptfs/inode.c 19 Dec 2008 19:52:26 -0000 1.2
>> @@ -430,9 +430,6 @@ 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;
>> }
>>
>> @@ -444,7 +441,10 @@ static int ecryptfs_unlink(struct inode
>> struct dentry *lower_dir_dentry;
>>
>> lower_dir_dentry = lock_parent(lower_dentry);
>> + dget(lower_dentry);
>> + atomic_inc_return(&lower_dentry->d_inode->i_count);
atomic_inc() should probably be used here
>> rc = vfs_unlink(lower_dir_inode, lower_dentry);
>> + dput(lower_dentry);
>> if (rc) {
>> printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
>> goto out_unlock;
>> @@ -455,6 +455,7 @@ static int ecryptfs_unlink(struct inode
>> dentry->d_inode->i_ctime = dir->i_ctime;
>> d_drop(dentry);
>> out_unlock:
>> + iput(lower_dentry->d_inode);
>> unlock_dir(lower_dir_dentry);
>> return rc;
>> }
>> @@ -538,8 +539,12 @@ static int ecryptfs_rmdir(struct inode *
>> fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
>> dir->i_nlink = lower_dir_dentry->d_inode->i_nlink;
>> unlock_dir(lower_dir_dentry);
>> - if (!rc)
>> + if (!rc) {
>> + struct inode *inode = dentry->d_inode;
>> + inode->i_nlink = ecryptfs_inode_to_lower(inode)->i_nlink;
>> + inode->i_ctime = dir->i_ctime;
>> d_drop(dentry);
>> + }
>> dput(dentry);
>> return rc;
>> }
>> Index: linux-2.6.27/fs/ecryptfs/super.c
>> ===================================================================
>> retrieving revision 1.1
>> retrieving revision 1.2
>> diff -u -p -r1.1 -r1.2
>> --- linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 04:37:43 -0000 1.1
>> +++ linux-2.6.27/fs/ecryptfs/super.c 19 Dec 2008 19:52:26 -0000 1.2
>> @@ -89,6 +89,7 @@ static void ecryptfs_destroy_inode(struc
>> }
>> }
>> mutex_unlock(&inode_info->lower_file_mutex);
>> + iput(inode_info->wii_inode);
>> ecryptfs_destroy_crypt_stat(&inode_info->crypt_stat);
>> kmem_cache_free(ecryptfs_inode_info_cache, inode_info);
>> }
>> @@ -101,6 +102,7 @@ static void ecryptfs_destroy_inode(struc
>> */
>> void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
>> {
>> + atomic_inc_return(&lower_inode->i_count);
>> ecryptfs_set_inode_lower(inode, lower_inode);
>> inode->i_ino = lower_inode->i_ino;
>> inode->i_version++;
On Tue, 13 Jan 2009 15:20:21 +0900
[email protected] wrote:
>
> Here are several fixes for linux-2.6.27/fs/ecryptfs.
>
> - The ecryptfs inode holds a reference to the lower inode, but doesn't
> increment the reference counter. When a user sets inotify to the
> ecryptfs inode, it may live without the corresponding dentry. In this
> case the referecen to the lower inode may be broken.
> This patch maintains the reference of the lower inode.
>
> - follow the VFS unlink sequence in ecryptfs_unlink() which is
> inrementing and decrementing the inode->i_count and the reference
> counter for the dentry.
>
> - maintain the link count and ctime in ecryptfs_rmdir() because a user
> may issue fstat(2) later.
>
> - remove the unnecessary d_drop()s in ecryptfs_link().
>
> And I have experienced a strange behaviour. When ecryptfs gets -ENOSPC
> from the lower fs, it converts and returns EINVAL to the userspace. Is
> this an intended behaviour?
>
I am puzzled by the current ecryptfs maintenance situation.
I queued this for 2.6.30. It could be bumped up for 2.6.29 (and even
backported to 2.6.28 and earlier) with suitable acks from the
maintainer(?)
>
> ...
>
> + atomic_inc_return(&lower_dentry->d_inode->i_count);
> + atomic_inc_return(&lower_inode->i_count);
atomic_inc() would suffice here, yes?
From: Andrew Morton <[email protected]>
s/atomic_inc_return/atomic_inc/
Cc: Dave Kleikamp <[email protected]>
Cc: J. R. Okajima <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
fs/ecryptfs/inode.c | 2 +-
fs/ecryptfs/super.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff -puN fs/ecryptfs/inode.c~ecryptfs-some-inode-attr-fixes-fix fs/ecryptfs/inode.c
--- a/fs/ecryptfs/inode.c~ecryptfs-some-inode-attr-fixes-fix
+++ a/fs/ecryptfs/inode.c
@@ -479,7 +479,7 @@ static int ecryptfs_unlink(struct inode
lower_dir_dentry = lock_parent(lower_dentry);
dget(lower_dentry);
- atomic_inc_return(&lower_dentry->d_inode->i_count);
+ atomic_inc(&lower_dentry->d_inode->i_count);
rc = vfs_unlink(lower_dir_inode, lower_dentry);
dput(lower_dentry);
if (rc) {
diff -puN fs/ecryptfs/super.c~ecryptfs-some-inode-attr-fixes-fix fs/ecryptfs/super.c
--- a/fs/ecryptfs/super.c~ecryptfs-some-inode-attr-fixes-fix
+++ a/fs/ecryptfs/super.c
@@ -102,7 +102,7 @@ static void ecryptfs_destroy_inode(struc
*/
void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
{
- atomic_inc_return(&lower_inode->i_count);
+ atomic_inc(&lower_inode->i_count);
ecryptfs_set_inode_lower(inode, lower_inode);
inode->i_ino = lower_inode->i_ino;
inode->i_version++;
_
Andrew Morton wrote:
> On Tue, 13 Jan 2009 15:20:21 +0900
> [email protected] wrote:
>
>> Here are several fixes for linux-2.6.27/fs/ecryptfs.
>>
>> - The ecryptfs inode holds a reference to the lower inode, but doesn't
>> increment the reference counter. When a user sets inotify to the
>> ecryptfs inode, it may live without the corresponding dentry. In this
>> case the referecen to the lower inode may be broken.
>> This patch maintains the reference of the lower inode.
>>
>> - follow the VFS unlink sequence in ecryptfs_unlink() which is
>> inrementing and decrementing the inode->i_count and the reference
>> counter for the dentry.
>>
>> - maintain the link count and ctime in ecryptfs_rmdir() because a user
>> may issue fstat(2) later.
>>
>> - remove the unnecessary d_drop()s in ecryptfs_link().
>>
>> And I have experienced a strange behaviour. When ecryptfs gets -ENOSPC
>> from the lower fs, it converts and returns EINVAL to the userspace. Is
>> this an intended behaviour?
>>
>
> I am puzzled by the current ecryptfs maintenance situation.
>
http://article.gmane.org/gmane.linux.kernel/768122
The torch has been passed to me. :)
> I queued this for 2.6.30. It could be bumped up for 2.6.29 (and even
> backported to 2.6.28 and earlier) with suitable acks from the
> maintainer(?)
>
The changes to ecryptfs_rmdir() are valid, but not the rest of the patch.
>> ...
>>
>> + atomic_inc_return(&lower_dentry->d_inode->i_count);
>> + atomic_inc_return(&lower_inode->i_count);
>
> atomic_inc() would suffice here, yes?
>
> From: Andrew Morton <[email protected]>
>
> s/atomic_inc_return/atomic_inc/
>
> Cc: Dave Kleikamp <[email protected]>
> Cc: J. R. Okajima <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> fs/ecryptfs/inode.c | 2 +-
> fs/ecryptfs/super.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff -puN fs/ecryptfs/inode.c~ecryptfs-some-inode-attr-fixes-fix fs/ecryptfs/inode.c
> --- a/fs/ecryptfs/inode.c~ecryptfs-some-inode-attr-fixes-fix
> +++ a/fs/ecryptfs/inode.c
> @@ -479,7 +479,7 @@ static int ecryptfs_unlink(struct inode
>
> lower_dir_dentry = lock_parent(lower_dentry);
> dget(lower_dentry);
> - atomic_inc_return(&lower_dentry->d_inode->i_count);
> + atomic_inc(&lower_dentry->d_inode->i_count);
> rc = vfs_unlink(lower_dir_inode, lower_dentry);
> dput(lower_dentry);
> if (rc) {
> diff -puN fs/ecryptfs/super.c~ecryptfs-some-inode-attr-fixes-fix fs/ecryptfs/super.c
> --- a/fs/ecryptfs/super.c~ecryptfs-some-inode-attr-fixes-fix
> +++ a/fs/ecryptfs/super.c
> @@ -102,7 +102,7 @@ static void ecryptfs_destroy_inode(struc
> */
> void ecryptfs_init_inode(struct inode *inode, struct inode *lower_inode)
> {
> - atomic_inc_return(&lower_inode->i_count);
> + atomic_inc(&lower_inode->i_count);
> ecryptfs_set_inode_lower(inode, lower_inode);
> inode->i_ino = lower_inode->i_ino;
> inode->i_version++;
> _
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~ecryptfs-devel
> Post to : [email protected]
> Unsubscribe : https://launchpad.net/~ecryptfs-devel
> More help : https://help.launchpad.net/ListHelp
On Thu, 15 Jan 2009 17:39:08 -0600
Tyler Hicks <[email protected]> wrote:
> > I am puzzled by the current ecryptfs maintenance situation.
> >
>
> http://article.gmane.org/gmane.linux.kernel/768122
>
> The torch has been passed to me. :)
yup, thanks.
> > I queued this for 2.6.30. It could be bumped up for 2.6.29 (and even
> > backported to 2.6.28 and earlier) with suitable acks from the
> > maintainer(?)
> >
>
> The changes to ecryptfs_rmdir() are valid, but not the rest of the patch.
>
OK, I'll drop my copy. Please send something back at me sometime, with
indications about which kernel version(s) it should be merged into.
We should chase J.R. for a Signed-off-by:, which was omitted from the
original patch.
Tyler Hicks:
> >> - The ecryptfs inode holds a reference to the lower inode, but doesn't=
>
> >> increment the reference counter. When a user sets inotify to the
> >> ecryptfs inode, it may live without the corresponding dentry. In thi=
> s
> >> case the referecen to the lower inode may be broken.
> >> This patch maintains the reference of the lower inode.
>
> Is this a problem that you've experienced or something you found during
> a code review? How can it be reproduced? We get a reference to the
> lower inode with the igrab() in ecryptfs_interpose() and put it back in
> ecryptfs_clear_inode(). This part of the patch seems to just increment
> the ref counts again.
I could confirm the igrab() in ecryptfs_interpose(), so touching i_count
in my patch are unnecessary.
Although I don't remember the detail, I have experienced a trouble (and
I wrote the patch). If I can recall the problem and find the
reproducible way, I will write again.
One thing I remember is, vfs_unlink() in ecryptfs_unlink() made
lower_dentry->d_inode NULL unexpectedly.
I guess the problem was related to the i_count including my fixes for
ecryptfs_unlink() and ecryptfs_link(). Current ecryptfs_link() calls
ecryptfs_interpose(), but obviously the inode is not I_NEW and the
incremented i_count is decremented again (finally unchanged).
The current unnecessary d_drop()s may help hiding the problem, but I am
not sure.
> I see that do_unlinkat does this, but since eCryptfs already holds these
> references, I don't think that we need to do it here.
Whether the counter is one or more is more important than to hold or not
to hold simply.
> This part of the patch is valid, nice catch!
I am happy that my patch was not totally useless. :-)
And one more suggestion. It is better to set f_type in
ecryptfs_statfs(), and move ECRYPTFS_SUPER_MAGIC under include/linux/,
in order to be usable from userspace (or other modules).
J. R. Okajima
Andrew Morton:
> > + atomic_inc_return(&lower_dentry->d_inode->i_count);
> > + atomic_inc_return(&lower_inode->i_count);
>
> atomic_inc() would suffice here, yes?
I thought that ..._return() is smp safe and necessary here.
Because lower_inode may be touched by lower fs (outside of ecryptfs).
Anyway my original patch seemed to be already dropped.
J. R. Okajima
On Fri, 16 Jan 2009 16:42:31 +0900 [email protected] wrote:
> Andrew Morton:
> > > + atomic_inc_return(&lower_dentry->d_inode->i_count);
> > > + atomic_inc_return(&lower_inode->i_count);
> >
> > atomic_inc() would suffice here, yes?
>
> I thought that ..._return() is smp safe and necessary here.
> Because lower_inode may be touched by lower fs (outside of ecryptfs).
atomic_inc() is fully atomic too. atomic_inc_return() is "special",
in that it does an atomic_inc(), but also returns the result of that
increment to the caller.
Tyler Hicks:
> >> - remove the unnecessary d_drop()s in ecryptfs_link().
>
> I tend to agree that they look unnecessary, but one of them was added
> for cifs (see ae56fb16) and the other two seem to be intentional (see
> 45ec4aba) as well. I'm working on adding support for networked
> filesystems, I'll keep these d_drop()s in mind and see if we can drop
> them. :)
commit ae56fb16337c882c52806508f93ead4034004c7a
When CIFS is the lower filesystem, the old lower dentry needs to be explicitly
dropped from inside eCryptfs to force a revalidate. In addition, when CIFS is
the lower filesystem, the inode attributes need to be copied back up from the
lower inode to the eCryptfs inode on an eCryptfs revalidate.
Even with this commit, fstat(2) still returns incorrect attributes,
doesn't it? Because fstat(2) doesn't involve revalidate.
{
fd = open(fileA);
link(fileA, fileB);
fstat(fd);
}
commit 45ec4ababe999cb95f9c0cad03b2689cb0b77a2b
Fix the use of dget/dput calls to balance out on the lower filesystem.
:::
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 2f2c6cf..ff4865d 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
:::
@@ -475,8 +470,8 @@ out_lock:
unlock_dir(lower_dir_dentry);
dput(lower_new_dentry);
dput(lower_old_dentry);
- if (!new_dentry->d_inode)
- d_drop(new_dentry);
+ d_drop(new_dentry);
+ d_drop(old_dentry);
return rc;
}
Are these two d_drop()s really intentional?
I don't understand why they are necessary after the success of link.
It might be the same to the commit ae56fb16 (see above), ie. wanting to
force re-lookup and get the latest inode attributes. (Just a guess)
Actually they are maintained in ecryptfs_link() correctly...
J. R. Okajima
On Fri, 2009-01-16 at 16:36 +0900, [email protected] wrote:
> Tyler Hicks:
> I guess the problem was related to the i_count including my fixes for
> ecryptfs_unlink() and ecryptfs_link(). Current ecryptfs_link() calls
> ecryptfs_interpose(), but obviously the inode is not I_NEW and the
> incremented i_count is decremented again (finally unchanged).
> The current unnecessary d_drop()s may help hiding the problem, but I am
> not sure.
I think you're hitting on something here. I never understood the need
for the d_drop()s, but taking them out broke things. They probably are
just papering over bugs where the ecryptfs inode is not being properly
updated after changes are made to the lower inode.
Shaggy
--
David Kleikamp
IBM Linux Technology Center
Dave Kleikamp:
> I think you're hitting on something here. I never understood the need
> for the d_drop()s, but taking them out broke things. They probably are
> just papering over bugs where the ecryptfs inode is not being properly
> updated after changes are made to the lower inode.
As long as cifs_hardlink() calls d_drop() for the target dentry (as the
old version of NFS did), ecryptfs may have to call d_drop() too. But I
believe the d_drop() for the source dentry is unnecessary, as long as
the inode attributes are maintained correctly.
Additionally, when the lower filesystem does NOT call d_drop(), ecryptfs
has no necessary to call it. I'd like to suggest ecryptfs_link() to
check it by d_unhashed().
J. R. Okajima
On Sat, 2009-01-17 at 15:03 +0900, [email protected] wrote:
> Dave Kleikamp:
> > I think you're hitting on something here. I never understood the need
> > for the d_drop()s, but taking them out broke things. They probably are
> > just papering over bugs where the ecryptfs inode is not being properly
> > updated after changes are made to the lower inode.
>
> As long as cifs_hardlink() calls d_drop() for the target dentry (as the
> old version of NFS did), ecryptfs may have to call d_drop() too. But I
> believe the d_drop() for the source dentry is unnecessary, as long as
> the inode attributes are maintained correctly.
> Additionally, when the lower filesystem does NOT call d_drop(), ecryptfs
> has no necessary to call it. I'd like to suggest ecryptfs_link() to
> check it by d_unhashed().
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 ? */
}
}
--
David Kleikamp
IBM Linux Technology Center
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.
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)
- 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 <[email protected]>
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;
}
On Sun, 2009-01-18 at 02:42 +0900, [email protected] 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 <[email protected]>
>
> 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
> 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.
>
> And I found another bug in ecryptfs_link().
> Here is a patch including that.
I forgot i_count.
Here is the fixed one.
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)
- 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 <[email protected]>
diff -u -p -r1.1 inode.c
--- fs/ecryptfs/inode.c 19 Dec 2008 03:05:27 -0000 1.1
+++ fs/ecryptfs/inode.c 18 Jan 2009 05:22:35 -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,26 @@ 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) {
+ /* ecryptfs_interpose() doesn't increment */
+ atomic_inc(&inode->i_count);
+ 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;
}
Dave Kleikamp:
> 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.
Then it means updating i_size in ecryptfs_link() is unnecessary...
J. R. Okajima
On Mon, 2009-01-19 at 11:17 +0900, [email protected] wrote:
> Dave Kleikamp:
> > 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.
>
> Then it means updating i_size in ecryptfs_link() is unnecessary...
It's restoring i_size to the correct value after ecryptfs_interpose
updates it with the wrong value.
Shaggy
--
David Kleikamp
IBM Linux Technology Center
Dave Kleikamp:
> > > 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.
:::
> It's restoring i_size to the correct value after ecryptfs_interpose
> updates it with the wrong value.
Does "ecryptfs_interpose() will overwrite the correct upper inode size"
means ecryptfs_interpose() sets a wrong value?
If so, I can understand why ecryptfs_link() sets i_size.
J. R. Okajima
On Tue, 2009-01-20 at 00:25 +0900, [email protected] wrote:
> Dave Kleikamp:
> > > > 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.
> :::
> > It's restoring i_size to the correct value after ecryptfs_interpose
> > updates it with the wrong value.
>
> Does "ecryptfs_interpose() will overwrite the correct upper inode size"
> means ecryptfs_interpose() sets a wrong value?
Yes. ecryptfs_interpose() will copy the lower inode's size to the upper
inode. For an encrypted file, the lower inode will have a larger size,
since the lower file is prefixed with a header.
> If so, I can understand why ecryptfs_link() sets i_size.
Shaggy
--
David Kleikamp
IBM Linux Technology Center
Dave Kleikamp:
> > Does "ecryptfs_interpose() will overwrite the correct upper inode size"
> > means ecryptfs_interpose() sets a wrong value?
>
> Yes. ecryptfs_interpose() will copy the lower inode's size to the upper
> inode. For an encrypted file, the lower inode will have a larger size,
> since the lower file is prefixed with a header.
I see.
I am afraid my poor English made me misunderstood.
Thank you.
J. R. Okajima