Hi,
in private mail with Erez, Erez also came up with the same solution, except
that he only changes the first case, which is enough (tested it):
===================================================================
--- unionfs-2.5.8_for_2.6.32.28/fs/unionfs/inode.c
+++ unionfs-2.5.8_for_2.6.32.28/fs/unionfs/inode.c
@@ -927,7 +927,11 @@
err = -EINVAL;
goto out;
}
- lower_inode = unionfs_lower_inode(inode);
+ /*
+ * Get the lower inode directly from lower dentry, in case ibstart
+ * is -1 (which happens when the file is open but unlinked.
+ */
+ lower_inode = lower_dentry->d_inode;
/* check if user has permission to change lower inode */
err = inode_change_ok(lower_inode, ia);
One remaining code branch in unionfs_setattr() still exhibits a further issue,
with a very similar test case (open() is changed to not create the file):
#include <unistd.h>
#include <fcntl.h>
int main(int argc, char *argv[]) {
int fd = open(argv[1],O_RDWR|O_APPEND);
unlink(argv[1]);
ftruncate(fd, 0);
}
When the file, on which open+unlink+ftruncate runs, is found on a read-only
branch, a copyup needs to be done, which is taken care of by this code in
unionfs_setattr():
/* copyup if the file is on a read only branch */
if (is_robranch_super(dentry->d_sb, bstart)
|| __is_rdonly(lower_inode)) {
/* check if we have a branch to copy up to */
if (bstart <= 0) {
err = -EACCES;
goto out;
}
if (ia->ia_valid & ATTR_SIZE)
size = ia->ia_size;
else
size = i_size_read(inode);
/* copyup to next available branch */
for (bindex = bstart - 1; bindex >= 0; bindex--) {
err = copyup_dentry(parent->d_inode,
dentry, bstart, bindex,
dentry->d_name.name,
dentry->d_name.len,
NULL, size);
if (!err)
break;
}
if (err)
goto out;
/* get updated lower_dentry/inode after copyup */
lower_dentry = unionfs_lower_dentry(dentry);
lower_inode = unionfs_lower_inode(inode);
}
Note that, during the unlink() system call in the test case, unionfs created a
whiteout entry in the a writeable branch above the read-only branch to indicate
that the file on the read-only branch is marked deleted inside unionfs.
After the call to copyup_dentry() in the loop above succeeded, there is now a
new file in the same branch as the the whiteout, which is contradicting the
whiteout and unionfs_setattr().
copyup_dentry() also creates a new lower_dentry which is retrieved by this line:
/* get updated lower_dentry/inode after copyup */
lower_dentry = unionfs_lower_dentry(dentry);
This is then used to call notify_change() for the setattr:
mutex_lock(&lower_dentry->d_inode->i_mutex);
err = notify_change(lower_dentry, &lower_ia);
mutex_unlock(&lower_dentry->d_inode->i_mutex);
But if the dentry still exists (as it does now) after the return from setattr,
unionfs_lookup() will find this dentry and ignore the whiteout, showing the
file e.g. in ls -l, but with no file attributes because the stat() will not
work, likely due to the whiteout, so you see a file with ??? as perms etc,
and you cannot open, remove or replace the looked-up file.
Adding an call to "dput(lower_dentry);" for this specific case before the
call to unionfs_setfattr() causes unionfs_lookup to find the whiteout and
the file is not shown in ls after the call to ftruncate(). If possible, it
would likely be best to have the copyup_dentry take care of this but I do
not know if this is possible. For now, I can only present this finding.
Best Regards,
Bernhard
PS: There is a remaining open bugzilla entry for this crash:
https://bugzilla.filesystems.org/show_bug.cgi?id=637
PS: If somebody is only reading the archive, here are further links to
discussions on this issue (see the launchpad entries, they are 3 years old):
http://www.mail-archive.com/[email protected]/msg03061.html
http://www.mail-archive.com/[email protected]/msg03114.html
_______________________________________________
unionfs mailing list: http://unionfs.filesystems.org/
[email protected]
http://www.fsl.cs.sunysb.edu/mailman/listinfo/unionfs
