Hi,
The following backtrack was reported by our dev team.
[<8010ea60>] (unwind_backtrace) from [<8010c4d8>] (show_stack+0x10/0x14)
[<8010c4d8>] (show_stack) from [<803b590c>] (ubifs_xattr_set+0x210/0x3ec)
[<803b590c>] (ubifs_xattr_set) from [<80272134>] (__vfs_setxattr+0x70/0x80)
[<80272134>] (__vfs_setxattr) from [<803ecd8c>]
(smack_d_instantiate+0x41c/0x430)
[<803ecd8c>] (smack_d_instantiate) from [<803e4ef0>]
(security_d_instantiate+0x34/0x54)
[<803e4ef0>] (security_d_instantiate) from [<80263edc>]
(d_instantiate+0x28/0x4c)
[<80263edc>] (d_instantiate) from [<80392858>] (ubifs_mkdir+0x1f8/0x200)
[<80392858>] (ubifs_mkdir) from [<80257124>] (vfs_mkdir+0x148/0x1dc)
[<80257124>] (vfs_mkdir) from [<8025add8>] (SyS_mkdirat+0x84/0xec)
[<8025add8>] (SyS_mkdirat) from [<80107dfc>] (__sys_trace_return+0x0/0x10)
Looking at the code for smack_d_instantiate it's going down the following path
/*
* Transmuting directory
*/
if (S_ISDIR(inode->i_mode)) {
/*
* If this is a new directory and the label was
* transmuted when the inode was initialized
* set the transmute attribute on the directory
* and mark the inode.
*
* If there is a transmute attribute on the
* directory mark the inode.
*/
if (isp->smk_flags & SMK_INODE_CHANGED) {
isp->smk_flags &= ~SMK_INODE_CHANGED;
rc = __vfs_setxattr(dp, inode,
XATTR_NAME_SMACKTRANSMUTE,
TRANS_TRUE, TRANS_TRUE_SIZE,
0);
} else {
which ends up in ubifs_xattr_set which is expecting the inode
semaphore to be locked and hence the assert message.
I can reproduce this by creating a directory with SMACK64TRANSMUTE and
then adding a rule with 't' set and then creating a directory
mkdir test
chmod 777 test
chsmack -a update test
chsmack -t test
echo programmingapp > /proc/self/attr/current
echo programmingapp update rwxat > /sys/fs/smackfs/load2
Creating a directory in test which trigger the ubifs assert.
Looking at the code some more, don't we need to lock the inode when
calling __vfs_setxattr above as the inode will have already been
created (ubifs_init_security in ubifs_mkdir) and added to the inode
hash and another process could potentially set an extended attribute
on the directory whilst we are trying to add the transmute attribute
here?
I created a patch which put an inode_trylock around __vfs_setxattr and
the ubifs assert has now disappeared in the use case described.
Although this fixes things for a UBIFS I had a quick look at other
filesystems to see how they handle setting extended attributes and the
2 I looked at have a i_xattr_sem which is in some wrapper structure
around an inode so maybe this isn't the best solution but maybe the
inode_trylock should be moved into the ubifs_xattr_set function
instead of the
if (check_lock)
ubifs_assert(c, inode_is_locked(host));
to be more in line with what the other filesystems do. This is on a
4.9 vendor kernel for an i.MX system but looking at 5.2 I think the
problem would still exist but not entirely sure. Either way is
replacing the above check with inode_trylock a valid solution or is
there a better way of solving this?
Many Thanks,
Martin.
