| Message ID | 20180513194829.GT30522@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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...
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