On Thu, 11 Mar 2010, Pascal Terjan wrote:
On Wed, Mar 10, 2010 at 11:15, Simon Thabuteau
<simon.thabuteau at gmail.com> wrote:
I do confirm that this bug is really annoying. I am using unionfs to
make my root file system read only and I get kernel oops all the time.
This bug is triggered by a number of applications (firefox, kde, ...)
I have another patch that prevents the oops here but I'd like to get
the opinion of someone who knows unionfs internals (I don't :) )
I don't think it should iput lower inodes after unlink while the file
is still open, and I would expect that it does not need to do so if
the file was closed
As you say, the patch you propose would really need to be done by someone
who knows the unionfs internals (I also don't :) )
But:
Your patch was the first patch which I see that fixes the problem properly,
at least from an application/POSIX perspective, because as your second
paragrapha also suggests, unionfs operations done on the file descriptor
of the unlinked file (such as ftruncate) must still succeed, despite
the file name which was opened being unlinked, as the inode is only really
deleted when the last link to it is dropped *and* the last file descriptor
pointing to it is *also* closed.
A colleague told me that there seems to be another way to fix the problem,
which is to deploy one of the changes which Erez Zadok deployed in his
patch from 2009 to fix the issue (but which apparently did'n catch all places):
http://www.mail-archive.com/[email protected]/msg03088.html
The patch from Erez was found in this mail (he committed it in 2009):
http://www.mail-archive.com/[email protected]/msg03086.html
Besides other changes, it adds this change to unionfs_setattr():
+ /*
+ * Note: we use lower_dentry->d_inode, because lower_inode may be
+ * unlinked (no inode->i_sb and i_ino==0. This happens if someone
+ * tries to open(), unlink(), then ftruncate() a file.
+ */
- mutex_lock(&lower_inode->i_mutex);
+ mutex_lock(&lower_dentry->d_inode->i_mutex);
err = notify_change(lower_dentry, ia);
- mutex_unlock(&lower_inode->i_mutex);
+ mutex_unlock(&lower_dentry->d_inode->i_mutex);
This means it does not use lower_inode but lower_dentry->d_inode, at this
place but it missed to also do the same for other places where lower_dentry
is used:
From:
http://www.mail-archive.com/[email protected]/msg03088.html
Quote:
The null-pointer dereference error still occurs,
from fs/unionfs/inode.c
909: err = -EINVAL;
910: goto out;
911: }
912: lower_inode = unionfs_lower_inode(inode);
=> unionfs_lower_inode returns null, bstart(inode) = -1
913:
914: /* check if user has permission to change lower inode */
915: err = inode_change_ok(lower_inode, ia);
916: if (err)
917: goto out;
918:
919: /* copyup if the file is on a read only branch */
920: if (is_robranch_super(dentry->d_sb, bstart)
921: || __is_rdonly(lower_inode)) {
=> null pointer dereference inside __is_rdonly
So instead of changing all places where lower_inode (which is a NULL pointer
in this test) occurs to lower_dentry->d_inode and thereby making the code
less readable as it already is, I propose to change all initialisations of
lower_inode to the lower_dentry->d_inode, having the same effect as the patch
of Erez, but for *all* of unionfs_setattr():
Signed-off-by: Bernhard Kaindl <[email protected]>
===================================================================
--- 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,8 @@
err = -EINVAL;
goto out;
}
- lower_inode = unionfs_lower_inode(inode);
+ lower_inode = lower_dentry->d_inode;
+ BUG_ON(!lower_inode);
/* check if user has permission to change lower inode */
err = inode_change_ok(lower_inode, ia);
@@ -961,7 +962,9 @@
goto out;
/* get updated lower_dentry/inode after copyup */
lower_dentry = unionfs_lower_dentry(dentry);
- lower_inode = unionfs_lower_inode(inode);
+ BUG_ON(!lower_dentry);
+ lower_inode = lower_dentry->d_inode;
+ BUG_ON(!lower_inode);
}
/*
I added the BUG_ON() just to be sure to get a good BUG message in case
lower_dentry->d_inode is not always valid, but it should be as the patch
committed by Erez in 2009 already makes use of it since a long time,
so I think we can remove these.
Bernhard
_______________________________________________
unionfs mailing list: http://unionfs.filesystems.org/
[email protected]
http://www.fsl.cs.sunysb.edu/mailman/listinfo/unionfs
