fix breakage caused by d_find_alias() semantics change

RFC PATCH In review Al Viro 3 participants 8 messages May 13, 2018

Subsystems (2)

VFS SELinux security module

lore

Share & tools

lore

Al Viro

author maintainer

2018-05-13 15:52 UTC #0 lore raw

RFC PATCH 1/1 fix breakage caused by d_find_alias() semantics change

[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 [email protected] # f1ee616214cb "VFS: don't keep disconnected dentries on d_anon"

Signed-off-by Al Viro <[email protected]>

security/selinux/hooks.c +7 -2

index 4cafe6a19167..d3dd37578994 100644
@@ -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

Linus Torvalds

2018-05-13 18:35 UTC #1 lore raw

Quoted 1 line · Al Viro

                         /* Called from selinux_complete_init, try to find

a dentry. */

Review

Quoted 3 lines

                         dentry = d_find_alias(inode);

(quoted diff) +2 -0

+                       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

Al Viro

author maintainer

2018-05-13 18:56 UTC #2 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 1 line · Al Viro

                         /* Called from selinux_complete_init, try to find

a dentry. */

Review

Quoted 3 lines

                         dentry = d_find_alias(inode);

(quoted diff) +2 -0

+                       if (!dentry)
+                               dentry = d_find_any_alias(inode);

Quoted 5 lines

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 ;-/

Linus Torvalds

2018-05-13 19:00 UTC #3 lore raw

Quoted 1 line · Al Viro

The whole reason why that thing is getting a dentry is that some

filesystems

Quoted 2 lines

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

Al Viro

author maintainer

2018-05-13 19:48 UTC #4 lore raw

Quoted 4 lines · Linus Torvalds

Quoted 1 line · Al Viro
The whole reason why that thing is getting a dentry is that some
filesystems

Quoted 8 lines

Quoted 2 lines
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?

security/selinux/hooks.c +16 -4

index 4cafe6a19167..398d165f884e 100644
@@ -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

Linus Torvalds

2018-05-13 20:24 UTC #5 lore raw

Quoted 4 lines · Al Viro

Quoted 2 lines
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

NeilBrown

2018-05-13 22:02 UTC #6 lore raw

In reply to Al Viro

Quoted 21 lines · Al Viro

Quoted 4 lines · Linus Torvalds
Quoted 1 line · Al Viro
The whole reason why that thing is getting a dentry is that some
filesystems
Quoted 8 lines
Quoted 2 lines
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?
security/selinux/hooks.c +0 -0
index 4cafe6a19167..398d165f884e 100644

security/selinux/hooks.c static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent · line 1576

Quoted 10 lines

security/selinux/hooks.c +6 -1

@@ -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

Quoted 29 lines

security/selinux/hooks.c +10 -3

 			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

Al Viro

author maintainer

2018-05-13 22:17 UTC #7 lore raw

Quoted 2 lines · NeilBrown

Could you say *which* file systems? That would make it easier to understand the bigger picture.

Anything that uses pathname-based protocols...