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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
