* [RFC][PATCH] fix breakage caused by d_find_alias() semantics change @ 2018-05-13 15:51 Al Viro 2018-05-13 18:35 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2018-05-13 15:51 UTC (permalink / raw) To: linux-fsdevel; +Cc: NeilBrown, Linus Torvalds [will go into #fixes, unless somebody yells] "VFS: don't keep disconnected dentries on d_anon" had a non-trivial side-effect - d_unhashed() now returns true for those dentries, making d_find_alias() skip them altogether. For most of its callers that's fine - we really want a connected alias there. However, there is a codepath where we relied upon picking such aliases if nothing else could be found - selinux delayed initialization of contexts for inodes on already mounted filesystems used to rely upon that. Cc: stable@kernel.org # f1ee616214cb "VFS: don't keep disconnected dentries on d_anon" Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4cafe6a19167..d3dd37578994 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1570,6 +1570,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent } else { /* Called from selinux_complete_init, try to find a dentry. */ dentry = d_find_alias(inode); + if (!dentry) + dentry = d_find_any_alias(inode); } if (!dentry) { /* @@ -1674,14 +1676,17 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { /* We must have a dentry to determine the label on * procfs inodes */ - if (opt_dentry) + if (opt_dentry) { /* Called from d_instantiate or * d_splice_alias. */ dentry = dget(opt_dentry); - else + } else { /* Called from selinux_complete_init, try to * find a dentry. */ dentry = d_find_alias(inode); + if (!dentry) + dentry = d_find_any_alias(inode); + } /* * This can be hit on boot when a file is accessed * before the policy is loaded. When we load policy we ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 15:51 [RFC][PATCH] fix breakage caused by d_find_alias() semantics change Al Viro @ 2018-05-13 18:35 ` Linus Torvalds 2018-05-13 18:56 ` Al Viro 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2018-05-13 18:35 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, NeilBrown On Sun, May 13, 2018 at 9:18 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > /* Called from selinux_complete_init, try to find a dentry. */ > dentry = d_find_alias(inode); > + if (!dentry) > + dentry = d_find_any_alias(inode); Can you please explain why this isn't just dentry = d_find_any_alias(inode); without the initial "try to find a hashed alias"? (Same question for the other case). Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 18:35 ` Linus Torvalds @ 2018-05-13 18:56 ` Al Viro 2018-05-13 18:59 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2018-05-13 18:56 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel, NeilBrown On Sun, May 13, 2018 at 11:35:15AM -0700, Linus Torvalds wrote: > On Sun, May 13, 2018 at 9:18 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > /* Called from selinux_complete_init, try to find > a dentry. */ > > dentry = d_find_alias(inode); > > + if (!dentry) > > + dentry = d_find_any_alias(inode); > > Can you please explain why this isn't just > > dentry = d_find_any_alias(inode); > > without the initial "try to find a hashed alias"? The whole reason why that thing is getting a dentry is that some filesystems really want a *connected* dentry for getxattr. Sure, saner ones will be happy with disconnected dentry, but... If you want to rearchitect the shitpile under security/* so that it would not rely upon accidental properties of filesystems, you are welcome to that and to the resulting psychiatrist bills ;-/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 18:56 ` Al Viro @ 2018-05-13 18:59 ` Linus Torvalds 2018-05-13 19:48 ` Al Viro 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2018-05-13 18:59 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, NeilBrown On Sun, May 13, 2018 at 11:56 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > The whole reason why that thing is getting a dentry is that some filesystems > really want a *connected* dentry for getxattr. Sure, saner ones will be > happy with disconnected dentry, but... Can we just add a big comment to that effect? Because I don't mind the complexity, but I do mind having code that _looks_ complex with no reason, where the natural reaction is "why is it bothering being complex, when it could just do X". Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 18:59 ` Linus Torvalds @ 2018-05-13 19:48 ` Al Viro 2018-05-13 20:24 ` Linus Torvalds 2018-05-13 22:02 ` NeilBrown 0 siblings, 2 replies; 8+ messages in thread From: Al Viro @ 2018-05-13 19:48 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-fsdevel, NeilBrown On Sun, May 13, 2018 at 11:59:58AM -0700, Linus Torvalds wrote: > On Sun, May 13, 2018 at 11:56 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > The whole reason why that thing is getting a dentry is that some > filesystems > > really want a *connected* dentry for getxattr. Sure, saner ones will be > > happy with disconnected dentry, but... > > Can we just add a big comment to that effect? > > Because I don't mind the complexity, but I do mind having code that _looks_ > complex with no reason, where the natural reaction is "why is it bothering > being complex, when it could just do X". Point taken. How about the following variant? diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4cafe6a19167..398d165f884e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1568,8 +1568,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent /* Called from d_instantiate or d_splice_alias. */ dentry = dget(opt_dentry); } else { - /* Called from selinux_complete_init, try to find a dentry. */ + /* + * Called from selinux_complete_init, try to find a dentry. + * Some filesystems really want a connected one, so try + * that first. We could split SECURITY_FS_USE_XATTR in + * two, depending upon that... + */ dentry = d_find_alias(inode); + if (!dentry) + dentry = d_find_any_alias(inode); } if (!dentry) { /* @@ -1674,14 +1681,19 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { /* We must have a dentry to determine the label on * procfs inodes */ - if (opt_dentry) + if (opt_dentry) { /* Called from d_instantiate or * d_splice_alias. */ dentry = dget(opt_dentry); - else + } else { /* Called from selinux_complete_init, try to - * find a dentry. */ + * find a dentry. Some filesystems really want + * a connected one, so try that first. + */ dentry = d_find_alias(inode); + if (!dentry) + dentry = d_find_any_alias(inode); + } /* * This can be hit on boot when a file is accessed * before the policy is loaded. When we load policy we ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 19:48 ` Al Viro @ 2018-05-13 20:24 ` Linus Torvalds 2018-05-13 22:02 ` NeilBrown 1 sibling, 0 replies; 8+ messages in thread From: Linus Torvalds @ 2018-05-13 20:24 UTC (permalink / raw) To: Al Viro; +Cc: linux-fsdevel, NeilBrown On Sun, May 13, 2018 at 12:48 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > Can we just add a big comment to that effect? > Point taken. How about the following variant? Thanks, LGTM. I get the feeling that this is something where we could do something clever ("put hashed/directory aliases at the beginning of the list, push unhashed ones at the end"), and avoid the whole loop in d_find_alias() (and then "d_find_any_alias()" would also automatically prefer a hashed one without even calling d_find_alias first). But I doubt it matters. And the alias list is a hlist, so maybe it would be too painful to try to be clever anyway. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 19:48 ` Al Viro 2018-05-13 20:24 ` Linus Torvalds @ 2018-05-13 22:02 ` NeilBrown 2018-05-13 22:17 ` Al Viro 1 sibling, 1 reply; 8+ messages in thread From: NeilBrown @ 2018-05-13 22:02 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 2581 bytes --] On Sun, May 13 2018, Al Viro wrote: > On Sun, May 13, 2018 at 11:59:58AM -0700, Linus Torvalds wrote: >> On Sun, May 13, 2018 at 11:56 AM Al Viro <viro@zeniv.linux.org.uk> wrote: >> >> > The whole reason why that thing is getting a dentry is that some >> filesystems >> > really want a *connected* dentry for getxattr. Sure, saner ones will be >> > happy with disconnected dentry, but... >> >> Can we just add a big comment to that effect? >> >> Because I don't mind the complexity, but I do mind having code that _looks_ >> complex with no reason, where the natural reaction is "why is it bothering >> being complex, when it could just do X". > > Point taken. How about the following variant? > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4cafe6a19167..398d165f884e 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1568,8 +1568,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent > /* Called from d_instantiate or d_splice_alias. */ > dentry = dget(opt_dentry); > } else { > - /* Called from selinux_complete_init, try to find a dentry. */ > + /* > + * Called from selinux_complete_init, try to find a dentry. > + * Some filesystems really want a connected one, so try > + * that first. We could split SECURITY_FS_USE_XATTR in > + * two, depending upon that... > + */ Could you say *which* file systems? That would make it easier to understand the bigger picture. thanks, NeilBrown > dentry = d_find_alias(inode); > + if (!dentry) > + dentry = d_find_any_alias(inode); > } > if (!dentry) { > /* > @@ -1674,14 +1681,19 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent > if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { > /* We must have a dentry to determine the label on > * procfs inodes */ > - if (opt_dentry) > + if (opt_dentry) { > /* Called from d_instantiate or > * d_splice_alias. */ > dentry = dget(opt_dentry); > - else > + } else { > /* Called from selinux_complete_init, try to > - * find a dentry. */ > + * find a dentry. Some filesystems really want > + * a connected one, so try that first. > + */ > dentry = d_find_alias(inode); > + if (!dentry) > + dentry = d_find_any_alias(inode); > + } > /* > * This can be hit on boot when a file is accessed > * before the policy is loaded. When we load policy we [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change 2018-05-13 22:02 ` NeilBrown @ 2018-05-13 22:17 ` Al Viro 0 siblings, 0 replies; 8+ messages in thread From: Al Viro @ 2018-05-13 22:17 UTC (permalink / raw) To: NeilBrown; +Cc: Linus Torvalds, linux-fsdevel On Mon, May 14, 2018 at 08:02:32AM +1000, NeilBrown wrote: > Could you say *which* file systems? That would make it easier to > understand the bigger picture. Anything that uses pathname-based protocols... ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-05-13 22:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-13 15:51 [RFC][PATCH] fix breakage caused by d_find_alias() semantics change Al Viro 2018-05-13 18:35 ` Linus Torvalds 2018-05-13 18:56 ` Al Viro 2018-05-13 18:59 ` Linus Torvalds 2018-05-13 19:48 ` Al Viro 2018-05-13 20:24 ` Linus Torvalds 2018-05-13 22:02 ` NeilBrown 2018-05-13 22:17 ` Al Viro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).