Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment

From: Casey Schaufler <casey@schaufler-ca.com>
To: Al Viro <viro@ZenIV.linux.org.uk>,
	Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	jh80.chung@sungmsung.com, cw00.choi@samsung.com,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [BUG] Panic when systemd boot do mkdir on tmpfs mounted path with smack enabled environment
Date: Fri, 27 May 2016 12:03:37 -0700	[thread overview]
Message-ID: <9f2efc48-1778-d850-8bc3-a8ce77d6cdd7@schaufler-ca.com> (raw)
In-Reply-To: <20160527185150.GP14480@ZenIV.linux.org.uk>

On 5/27/2016 11:51 AM, Al Viro wrote:
> On Fri, May 27, 2016 at 04:11:41PM +0100, Al Viro wrote:
>
>>> After commit, "b968091 security_d_instantiate(): move to the point prior to attaching dentry to inode", booting on system with
>>> systemd and security smack, following kernel panic occurs.
>>                         /*
>>                          * 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 = inode->i_op->setxattr(dp,
>>                                         XATTR_NAME_SMACKTRANSMUTE,
>>                                         TRANS_TRUE, TRANS_TRUE_SIZE,
>>                                         0);
>>
>> Damnation ;-/  That change (separating inode and dentry arguments of
>> ->getxattr() so that security_d_instantiate() could be called before dentry
>> is hashed or attached to inode) had been discussed back in early March and
>> reaction of Casey back then had been basically "I believe that smack can
>> live with that, will verify that in about a week".  With no followup
>> objections - neither immediate, nor in a week.  As the matter of fact,
>> your posting is the first time anyone has reported stepping into that problem.
>> And that change had been present in linux-next since the beginning of May ;-/
>> Sigh...
>>
>>> It works fine if reverting the commit, "b968091 security_d_instantiate(): move to the point prior to attaching dentry to inode", for
>>> d_instantiate() like following.
>> Can't be reverted in mainline.  Not without shitloads of other stuff.
>>
>> There is a fairly straightforward way to handle that - do to ->setxattr()
>> what we'd already done to ->getxattr().  See vfs.git#smack-fix.  Warning:
>> it's only build-tested.  I'm going to have it go through LTP and xfstests
>> shortly; _please_ check if it works on your setup, because I've no idea
>> how to put together a testing setup for smack.
> FWIW, that couple of commits seems to survive the testing here and is
> pretty obvious.  I have _NOT_ tested it on smack setups, so I really want
> somebody (Casey or someone in Samsung) to check if it fixes the problem.
> The change itself isn't tricky, but I fucking _hate_ doing that this late
> in the merge window ;-/

I haven't actually seen the problem, but I've been having
real trouble getting a systemd configuration working properly.
The quickest validation will probably be coming from Seung-Woo Kim,
who reported the issue initially. I am working to verify both the
problem and the fix.