Re: [RFC][PATCH] fix breakage caused by d_find_alias() semantics change

[NeilBrown <neilb@suse.com>]

[-- 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 --]