| Message ID | 001301d1b808$31a437f0$94eca7d0$@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 27, 2016 at 08:09:09PM +0900, Seung-Woo Kim 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;-/ -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 11:51 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Fri, May 27, 2016 at 04:11:41PM +0100, Al Viro wrote: >> 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 ;-/ Al, if you want Casey to help test, I think you should write out the full git repository address, rather than just say "See vfs.git#smack-fix". Anybody who isn't used to pulling for you will just wonder where you keep your tree. And even I, who _am_ used to pulling from you, would have to look it up, so it's a lot more convenient if you actually write out the whole thing, Casey, Al is talking about git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs smack-fix and Al, please make your commit messages more informative than just "switch ->setxattr() to passing dentry and inode separately". You can see that from the patch. Please add a _why_ something is done, not just what it does. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:01:05PM -0700, Linus Torvalds wrote: > Al, if you want Casey to help test, I think you should write out the > full git repository address, rather than just say "See > vfs.git#smack-fix". > > Anybody who isn't used to pulling for you will just wonder where you > keep your tree. And even I, who _am_ used to pulling from you, would > have to look it up, so it's a lot more convenient if you actually > write out the whole thing, Point taken. > Casey, Al is talking about > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs smack-fix > > and Al, please make your commit messages more informative than just > "switch ->setxattr() to passing dentry and inode separately". You can > see that from the patch. Please add a _why_ something is done, not > just what it does. Umm... Would something along the lines of switch ->setxattr() to passing inode and dentry separately smack ->d_instantiate() uses ->setxattr(), so to be able to call it before we'd hashed the new dentry and attached it to inode, we ->setxattr() instances get the inode as an explicit argument rather than obtaining it from dentry. Similar change for ->getxattr() had been done commit ce23e64. Unlike ->getxattr() (which is used both by selinux and smack instances of ->d_instantiate()) ->setxattr() is used only by smack one and unfortunately it had been missed back then. be detailed enough for the second one with switch xattr_handler->set() to passing inode and dentry separately preparation for similar switch in ->setxattr() (see the next commit for rationale) for the first one? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > Umm... Would something along the lines of [...] Sure. That makes me go "ahh, ok, I see why". Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:03:37PM -0700, Casey Schaufler wrote: > 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. To trigger it you need to end up in smack_d_instantiate() for a directory that had SMK_INODE_CHANGED set in smack_inode_init_security(). IOW, smk_inode_transmutable() being true for its parent and smk_access_entry() for that parent returning something with MAY_TRANSMUTE in it. I'm not familiar enough with smack guts to put together a reproducer, but *ANY* call of ->setxattr() from smack_d_instantiate() on xattr-supporting filesystem will blow up in the mainline. At that point dentry still has NULL ->d_inode, so ->setxattr() instances are going to oops as soon as they try to do anything with the inode. All it takes is getting to that method call. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:34:34PM -0700, Linus Torvalds wrote: > On Fri, May 27, 2016 at 12:26 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > Umm... Would something along the lines of [...] > > Sure. That makes me go "ahh, ok, I see why". Amended and force-pushed... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > Amended and force-pushed... Ok, I'll ignore that branch for now, in the hopes that there will be actual testing success. Please send a full pull request at that point, ok? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote: > On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > Amended and force-pushed... > > Ok, I'll ignore that branch for now, in the hopes that there will be > actual testing success. Please send a full pull request at that point, > ok? Sure. FWIW, the original bug report had been from +0900, so it's what, about 5am Saturday morning there? Let's hope somebody will be there today, weekend or not... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/27/2016 1:24 PM, Al Viro wrote: > On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote: >> On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: >>> Amended and force-pushed... >> Ok, I'll ignore that branch for now, in the hopes that there will be >> actual testing success. Please send a full pull request at that point, >> ok? > Sure. FWIW, the original bug report had been from +0900, so it's what, > about 5am Saturday morning there? Let's hope somebody will be there > today, weekend or not... > Al, I have verified that upstream has the problem and that your vfs.git smack-fix branch works correctly. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2016년 05월 28일 07:44, Casey Schaufler wrote: > On 5/27/2016 1:24 PM, Al Viro wrote: >> On Fri, May 27, 2016 at 12:48:23PM -0700, Linus Torvalds wrote: >>> On Fri, May 27, 2016 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: >>>> Amended and force-pushed... >>> Ok, I'll ignore that branch for now, in the hopes that there will be >>> actual testing success. Please send a full pull request at that point, >>> ok? >> Sure. FWIW, the original bug report had been from +0900, so it's what, >> about 5am Saturday morning there? Let's hope somebody will be there >> today, weekend or not... >> > Al, I have verified that upstream has the problem and that > your vfs.git smack-fix branch works correctly. > It seems a bit late, but it works without reported panic for my environment too. Thanks, - Seung-Woo Kim
--- a/fs/dcache.c +++ b/fs/dcache.c @@ -1793,11 +1793,11 @@ void d_instantiate(struct dentry *entry, struct inode * inode) { BUG_ON(!hlist_unhashed(&entry->d_u.d_alias)); if (inode) { - security_d_instantiate(entry, inode); spin_lock(&inode->i_lock); __d_instantiate(entry, inode); spin_unlock(&inode->i_lock); } + security_d_instantiate(entry, inode); } EXPORT_SYMBOL(d_instantiate);