[RFC 0/2] selinux: Stop looking up dentries from inodes

[RFC 0/2] selinux: Stop looking up dentries from inodes

[Andreas Gruenbacher]

Here is an attempt at getting rid of d_find_alias in SELinux.  The first patch
makes SELinux call getxattr with a NULL dentry when the dentry is unknown
instead of using a random alias dentry, and makes getxattr fail with -ECHILD on
filesystems that cannot do getxattr with only an inode.  The second patch
changes getxattr on overlayfs so that it works with just an inode.  This leaves
9p and cifs as the filesystems where getxattr without a dentry doesn't make
sense.

These patches are based on mainline + Miklos's overlayfs-next branch:

  https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next

Git version:

  https://git.kernel.org/cgit/linux/kernel/git/agruen/linux.git/log/?h=work.selinux

Comments?

Thanks,
Andreas

Andreas Gruenbacher (2):
  selinux: Stop looking up dentries from inodes
  overlayfs: Make getxattr work with inode only

 fs/9p/acl.c              |  3 +++
 fs/9p/xattr.c            |  3 +++
 fs/cifs/xattr.c          |  9 +++++++--
 fs/ecryptfs/inode.c      |  8 ++++++--
 fs/overlayfs/inode.c     | 26 +++++++++++++++++---------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 10 +++++++---
 net/socket.c             |  3 +++
 security/selinux/hooks.c | 43 +++++++++++++++----------------------------
 9 files changed, 62 insertions(+), 44 deletions(-)

-- 
2.5.5

[RFC 1/2] selinux: Stop looking up dentries from inodes

[Andreas Gruenbacher]

SELinux sometimes needs to load the security label of an inode without
knowing which dentry belongs to that inode (for example, in the
inode_permission hook).  The security label is stored in an xattr;
getxattr currently requires both the dentry and the inode.

So far, SELinux has been using d_find_alias to find any dentry for the
inode; there is no guarantee that d_find_alias finds a suitable dentry
on all types of filesystems, though.

This patch changes SELinux calls getxattr with a NULL dentry when the
dentry is unknown.  On filesystems that require a dentry, getxattr fails
with -ECHILD; on all others, it succeeds.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/9p/acl.c              |  3 +++
 fs/9p/xattr.c            |  3 +++
 fs/cifs/xattr.c          |  9 +++++++--
 fs/ecryptfs/inode.c      |  8 ++++++--
 fs/overlayfs/inode.c     |  6 +++++-
 net/socket.c             |  3 +++
 security/selinux/hooks.c | 43 +++++++++++++++----------------------------
 7 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/fs/9p/acl.c b/fs/9p/acl.c
index 0576eae..197386e 100644
--- a/fs/9p/acl.c
+++ b/fs/9p/acl.c
@@ -220,6 +220,9 @@ static int v9fs_xattr_get_acl(const struct xattr_handler *handler,
 	struct posix_acl *acl;
 	int error;
 
+	if (!dentry)
+		return -ECHILD;
+
 	v9ses = v9fs_dentry2v9ses(dentry);
 	/*
 	 * We allow set/get/list of acl when access=client is not specified
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index a6bd349..9c88b6bc 100644
--- a/fs/9p/xattr.c
+++ b/fs/9p/xattr.c
@@ -143,6 +143,9 @@ static int v9fs_xattr_handler_get(const struct xattr_handler *handler,
 {
 	const char *full_name = xattr_full_name(handler, name);
 
+	if (!dentry)
+		return -ECHILD;
+
 	return v9fs_xattr_get(dentry, full_name, buffer, size);
 }
 
diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index 5e23f64..48c9f29 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -150,12 +150,17 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
 {
 	ssize_t rc = -EOPNOTSUPP;
 	unsigned int xid;
-	struct super_block *sb = dentry->d_sb;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
+	struct super_block *sb;
+	struct cifs_sb_info *cifs_sb;
 	struct tcon_link *tlink;
 	struct cifs_tcon *pTcon;
 	char *full_path;
 
+	if (!dentry)
+		return -ECHILD;
+
+	sb = dentry->d_sb;
+	cifs_sb = CIFS_SB(sb);
 	tlink = cifs_sb_tlink(cifs_sb);
 	if (IS_ERR(tlink))
 		return PTR_ERR(tlink);
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 9d153b6..dd90e5d 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -1043,8 +1043,12 @@ static ssize_t
 ecryptfs_getxattr(struct dentry *dentry, struct inode *inode,
 		  const char *name, void *value, size_t size)
 {
-	return ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
-				       ecryptfs_inode_to_lower(inode),
+	struct dentry *lower_dentry;
+	struct inode *lower_inode;
+
+	lower_dentry = dentry ? ecryptfs_dentry_to_lower(dentry) : NULL;
+	lower_inode = ecryptfs_inode_to_lower(inode);
+	return ecryptfs_getxattr_lower(lower_dentry, lower_inode,
 				       name, value, size);
 }
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 0ed7c40..8c3f985 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -251,8 +251,12 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
 		     const char *name, void *value, size_t size)
 {
 	struct path realpath;
-	enum ovl_path_type type = ovl_path_real(dentry, &realpath);
+	enum ovl_path_type type;
+
+	if (!dentry)
+		return -ECHILD;
 
+	type = ovl_path_real(dentry, &realpath);
 	if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
 		return -ENODATA;
 
diff --git a/net/socket.c b/net/socket.c
index a1bd161..1923a80 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -473,6 +473,9 @@ static ssize_t sockfs_getxattr(struct dentry *dentry, struct inode *inode,
 	size_t proto_size;
 	int error;
 
+	if (!dentry)
+		return -ECHILD;
+
 	error = -ENODATA;
 	if (!strncmp(name, XATTR_NAME_SOCKPROTONAME, XATTR_NAME_SOCKPROTONAME_LEN)) {
 		proto_name = dentry->d_name.name;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a86d537..dd509c8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1387,33 +1387,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	case SECURITY_FS_USE_NATIVE:
 		break;
 	case SECURITY_FS_USE_XATTR:
-		if (!inode->i_op->getxattr) {
-			isec->sid = sbsec->def_sid;
-			break;
-		}
-
-		/* Need a dentry, since the xattr API requires one.
-		   Life would be simpler if we could just pass the inode. */
-		if (opt_dentry) {
-			/* Called from d_instantiate or d_splice_alias. */
-			dentry = dget(opt_dentry);
-		} else {
-			/* Called from selinux_complete_init, try to find a dentry. */
-			dentry = d_find_alias(inode);
-		}
-		if (!dentry) {
-			/*
-			 * this is can be hit on boot when a file is accessed
-			 * before the policy is loaded.  When we load policy we
-			 * may find inodes that have no dentry on the
-			 * sbsec->isec_head list.  No reason to complain as these
-			 * will get fixed up the next time we go through
-			 * inode_doinit with a dentry, before these inodes could
-			 * be used again by userspace.
-			 */
-			goto out_unlock;
-		}
-
+		dentry = dget(opt_dentry);
 		len = INITCONTEXTLEN;
 		context = kmalloc(len+1, GFP_NOFS);
 		if (!context) {
@@ -1448,7 +1422,20 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 		}
 		dput(dentry);
 		if (rc < 0) {
-			if (rc != -ENODATA) {
+			if (rc == -ECHILD) {
+				/*
+				 * The dentry is NULL and this ->getxattr
+				 * requires a dentry.  We will keep trying
+				 * until we have a dentry and the label can be
+				 * loaded.
+				 *
+				 * (We could also remember when >getxattr
+				 * requires a dentry in the superblock if
+				 * retrying becomes too inefficient.)
+				 */
+				kfree(context);
+				goto out_unlock;
+			} else if (rc != -EOPNOTSUPP && rc != -ENODATA) {
 				printk(KERN_WARNING "SELinux: %s:  getxattr returned "
 				       "%d for dev=%s ino=%ld\n", __func__,
 				       -rc, inode->i_sb->s_id, inode->i_ino);
-- 
2.5.5

[RFC 2/2] overlayfs: Make getxattr work with inode only

[Andreas Gruenbacher]

Change the getxattr inode operation to only use its inode argument, and
ignore the dentry.  This is possible because on overlayfs, each dentry
has a separate inode and inodes are not shared among dentries.  Allows
SELinux to work on top of overlayfs.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/overlayfs/inode.c     | 26 +++++++++++++++-----------
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/super.c     | 10 +++++++---
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 8c3f985..7acc145 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -238,29 +238,32 @@ out:
 	return err;
 }
 
-static bool ovl_need_xattr_filter(struct dentry *dentry,
+static bool ovl_need_xattr_filter(struct inode *inode,
 				  enum ovl_path_type type)
 {
 	if ((type & (__OVL_PATH_PURE | __OVL_PATH_UPPER)) == __OVL_PATH_UPPER)
-		return S_ISDIR(dentry->d_inode->i_mode);
+		return S_ISDIR(inode->i_mode);
 	else
 		return false;
 }
 
-ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
+ssize_t ovl_getxattr(struct dentry *unused, struct inode *inode,
 		     const char *name, void *value, size_t size)
 {
-	struct path realpath;
+	struct ovl_entry *oe = inode->i_private;
 	enum ovl_path_type type;
+	struct dentry *realdentry;
+	bool is_upper;
 
-	if (!dentry)
-		return -ECHILD;
+	realdentry = ovl_entry_real(oe, &is_upper);
+	if (!realdentry->d_inode)
+		return -ENOENT;
 
-	type = ovl_path_real(dentry, &realpath);
-	if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
+	type = __ovl_path_type(oe, inode->i_mode);
+	if (ovl_need_xattr_filter(inode, type) && ovl_is_private_xattr(name))
 		return -ENODATA;
 
-	return vfs_getxattr(realpath.dentry, name, value, size);
+	return vfs_getxattr(realdentry, name, value, size);
 }
 
 ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
@@ -274,7 +277,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 	if (res <= 0 || size == 0)
 		return res;
 
-	if (!ovl_need_xattr_filter(dentry, type))
+	if (!ovl_need_xattr_filter(dentry->d_inode, type))
 		return res;
 
 	/* filter out private xattrs */
@@ -306,7 +309,8 @@ int ovl_removexattr(struct dentry *dentry, const char *name)
 		goto out;
 
 	err = -ENODATA;
-	if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
+	if (ovl_need_xattr_filter(dentry->d_inode, type) &&
+	    ovl_is_private_xattr(name))
 		goto out_drop_write;
 
 	if (!OVL_TYPE_UPPER(type)) {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 4bd9b5b..0d1430f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -131,6 +131,7 @@ static inline int ovl_do_whiteout(struct inode *dir, struct dentry *dentry)
 	return err;
 }
 
+enum ovl_path_type __ovl_path_type(struct ovl_entry *oe, umode_t mode);
 enum ovl_path_type ovl_path_type(struct dentry *dentry);
 u64 ovl_dentry_version_get(struct dentry *dentry);
 void ovl_dentry_version_inc(struct dentry *dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ce02f46..d04546e 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -70,9 +70,8 @@ static struct dentry *__ovl_dentry_lower(struct ovl_entry *oe)
 	return oe->numlower ? oe->lowerstack[0].dentry : NULL;
 }
 
-enum ovl_path_type ovl_path_type(struct dentry *dentry)
+enum ovl_path_type __ovl_path_type(struct ovl_entry *oe, umode_t mode)
 {
-	struct ovl_entry *oe = dentry->d_fsdata;
 	enum ovl_path_type type = 0;
 
 	if (oe->__upperdentry) {
@@ -82,7 +81,7 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 		 * Non-dir dentry can hold lower dentry from previous
 		 * location. Its purity depends only on opaque flag.
 		 */
-		if (oe->numlower && S_ISDIR(dentry->d_inode->i_mode))
+		if (oe->numlower && S_ISDIR(mode))
 			type |= __OVL_PATH_MERGE;
 		else if (!oe->opaque)
 			type |= __OVL_PATH_PURE;
@@ -93,6 +92,11 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 	return type;
 }
 
+enum ovl_path_type ovl_path_type(struct dentry *dentry)
+{
+	return __ovl_path_type(dentry->d_fsdata, dentry->d_inode->i_mode);
+}
+
 static struct dentry *ovl_upperdentry_dereference(struct ovl_entry *oe)
 {
 	return lockless_dereference(oe->__upperdentry);
-- 
2.5.5

Re: [RFC 1/2] selinux: Stop looking up dentries from inodes

[Stephen Smalley]

On 05/30/2016 09:59 AM, Andreas Gruenbacher wrote:
> SELinux sometimes needs to load the security label of an inode without
> knowing which dentry belongs to that inode (for example, in the
> inode_permission hook).  The security label is stored in an xattr;
> getxattr currently requires both the dentry and the inode.
> 
> So far, SELinux has been using d_find_alias to find any dentry for the
> inode; there is no guarantee that d_find_alias finds a suitable dentry
> on all types of filesystems, though.
> 
> This patch changes SELinux calls getxattr with a NULL dentry when the
> dentry is unknown.  On filesystems that require a dentry, getxattr fails
> with -ECHILD; on all others, it succeeds.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/9p/acl.c              |  3 +++
>  fs/9p/xattr.c            |  3 +++
>  fs/cifs/xattr.c          |  9 +++++++--
>  fs/ecryptfs/inode.c      |  8 ++++++--
>  fs/overlayfs/inode.c     |  6 +++++-
>  net/socket.c             |  3 +++
>  security/selinux/hooks.c | 43 +++++++++++++++----------------------------
>  7 files changed, 42 insertions(+), 33 deletions(-)
> 

> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 9d153b6..dd90e5d 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1043,8 +1043,12 @@ static ssize_t
>  ecryptfs_getxattr(struct dentry *dentry, struct inode *inode,
>  		  const char *name, void *value, size_t size)
>  {
> -	return ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
> -				       ecryptfs_inode_to_lower(inode),
> +	struct dentry *lower_dentry;
> +	struct inode *lower_inode;
> +
> +	lower_dentry = dentry ? ecryptfs_dentry_to_lower(dentry) : NULL;
> +	lower_inode = ecryptfs_inode_to_lower(inode);
> +	return ecryptfs_getxattr_lower(lower_dentry, lower_inode,
>  				       name, value, size);

ecryptfs_getxattr_lower() doesn't appear to handle a NULL lower_dentry.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a86d537..dd509c8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1387,33 +1387,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>  	case SECURITY_FS_USE_NATIVE:
>  		break;
>  	case SECURITY_FS_USE_XATTR:
> -		if (!inode->i_op->getxattr) {
> -			isec->sid = sbsec->def_sid;
> -			break;
> -		}

I don't think we can remove the above or we'll end up invoking
inode->i_op->getxattr == NULL below.

> -
> -		/* Need a dentry, since the xattr API requires one.
> -		   Life would be simpler if we could just pass the inode. */
> -		if (opt_dentry) {
> -			/* Called from d_instantiate or d_splice_alias. */
> -			dentry = dget(opt_dentry);
> -		} else {
> -			/* Called from selinux_complete_init, try to find a dentry. */
> -			dentry = d_find_alias(inode);
> -		}
> -		if (!dentry) {
> -			/*
> -			 * this is can be hit on boot when a file is accessed
> -			 * before the policy is loaded.  When we load policy we
> -			 * may find inodes that have no dentry on the
> -			 * sbsec->isec_head list.  No reason to complain as these
> -			 * will get fixed up the next time we go through
> -			 * inode_doinit with a dentry, before these inodes could
> -			 * be used again by userspace.
> -			 */
> -			goto out_unlock;
> -		}
> -
> +		dentry = dget(opt_dentry);
>  		len = INITCONTEXTLEN;
>  		context = kmalloc(len+1, GFP_NOFS);
>  		if (!context) {
> @@ -1448,7 +1422,20 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>  		}
>  		dput(dentry);
>  		if (rc < 0) {
> -			if (rc != -ENODATA) {
> +			if (rc == -ECHILD) {
> +				/*
> +				 * The dentry is NULL and this ->getxattr
> +				 * requires a dentry.  We will keep trying
> +				 * until we have a dentry and the label can be
> +				 * loaded.
> +				 *
> +				 * (We could also remember when >getxattr
> +				 * requires a dentry in the superblock if
> +				 * retrying becomes too inefficient.)
> +				 */
> +				kfree(context);
> +				goto out_unlock;
> +			} else if (rc != -EOPNOTSUPP && rc != -ENODATA) {
>  				printk(KERN_WARNING "SELinux: %s:  getxattr returned "
>  				       "%d for dev=%s ino=%ld\n", __func__,
>  				       -rc, inode->i_sb->s_id, inode->i_ino);
> 

Re: [RFC 1/2] selinux: Stop looking up dentries from inodes

[Andreas Gruenbacher]

On Tue, May 31, 2016 at 4:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 05/30/2016 09:59 AM, Andreas Gruenbacher wrote:
>> SELinux sometimes needs to load the security label of an inode without
>> knowing which dentry belongs to that inode (for example, in the
>> inode_permission hook).  The security label is stored in an xattr;
>> getxattr currently requires both the dentry and the inode.
>>
>> So far, SELinux has been using d_find_alias to find any dentry for the
>> inode; there is no guarantee that d_find_alias finds a suitable dentry
>> on all types of filesystems, though.
>>
>> This patch changes SELinux calls getxattr with a NULL dentry when the
>> dentry is unknown.  On filesystems that require a dentry, getxattr fails
>> with -ECHILD; on all others, it succeeds.
>>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> ---
>>  fs/9p/acl.c              |  3 +++
>>  fs/9p/xattr.c            |  3 +++
>>  fs/cifs/xattr.c          |  9 +++++++--
>>  fs/ecryptfs/inode.c      |  8 ++++++--
>>  fs/overlayfs/inode.c     |  6 +++++-
>>  net/socket.c             |  3 +++
>>  security/selinux/hooks.c | 43 +++++++++++++++----------------------------
>>  7 files changed, 42 insertions(+), 33 deletions(-)
>>
>
>> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
>> index 9d153b6..dd90e5d 100644
>> --- a/fs/ecryptfs/inode.c
>> +++ b/fs/ecryptfs/inode.c
>> @@ -1043,8 +1043,12 @@ static ssize_t
>>  ecryptfs_getxattr(struct dentry *dentry, struct inode *inode,
>>                 const char *name, void *value, size_t size)
>>  {
>> -     return ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
>> -                                    ecryptfs_inode_to_lower(inode),
>> +     struct dentry *lower_dentry;
>> +     struct inode *lower_inode;
>> +
>> +     lower_dentry = dentry ? ecryptfs_dentry_to_lower(dentry) : NULL;
>> +     lower_inode = ecryptfs_inode_to_lower(inode);
>> +     return ecryptfs_getxattr_lower(lower_dentry, lower_inode,
>>                                      name, value, size);
>
> ecryptfs_getxattr_lower() doesn't appear to handle a NULL lower_dentry.

Yes, it just calls the lower-layer filesystem's getxattr inode
operation with a NULL dentry, as intended.

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index a86d537..dd509c8 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1387,33 +1387,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>>       case SECURITY_FS_USE_NATIVE:
>>               break;
>>       case SECURITY_FS_USE_XATTR:
>> -             if (!inode->i_op->getxattr) {
>> -                     isec->sid = sbsec->def_sid;
>> -                     break;
>> -             }
>
> I don't think we can remove the above or we'll end up invoking
> inode->i_op->getxattr == NULL below.

Indeed, that's a bug, thanks. I've fixed that on my work.selinux branch.

With that fixed, could you possibly put this change to test?

Thanks,
Andreas

Re: [RFC 1/2] selinux: Stop looking up dentries from inodes

[Stephen Smalley]

On 05/31/2016 11:22 AM, Andreas Gruenbacher wrote:
> On Tue, May 31, 2016 at 4:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 05/30/2016 09:59 AM, Andreas Gruenbacher wrote:
>>> SELinux sometimes needs to load the security label of an inode without
>>> knowing which dentry belongs to that inode (for example, in the
>>> inode_permission hook).  The security label is stored in an xattr;
>>> getxattr currently requires both the dentry and the inode.
>>>
>>> So far, SELinux has been using d_find_alias to find any dentry for the
>>> inode; there is no guarantee that d_find_alias finds a suitable dentry
>>> on all types of filesystems, though.
>>>
>>> This patch changes SELinux calls getxattr with a NULL dentry when the
>>> dentry is unknown.  On filesystems that require a dentry, getxattr fails
>>> with -ECHILD; on all others, it succeeds.
>>>
>>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>>> ---
>>>  fs/9p/acl.c              |  3 +++
>>>  fs/9p/xattr.c            |  3 +++
>>>  fs/cifs/xattr.c          |  9 +++++++--
>>>  fs/ecryptfs/inode.c      |  8 ++++++--
>>>  fs/overlayfs/inode.c     |  6 +++++-
>>>  net/socket.c             |  3 +++
>>>  security/selinux/hooks.c | 43 +++++++++++++++----------------------------
>>>  7 files changed, 42 insertions(+), 33 deletions(-)
>>>
>>
>>> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
>>> index 9d153b6..dd90e5d 100644
>>> --- a/fs/ecryptfs/inode.c
>>> +++ b/fs/ecryptfs/inode.c
>>> @@ -1043,8 +1043,12 @@ static ssize_t
>>>  ecryptfs_getxattr(struct dentry *dentry, struct inode *inode,
>>>                 const char *name, void *value, size_t size)
>>>  {
>>> -     return ecryptfs_getxattr_lower(ecryptfs_dentry_to_lower(dentry),
>>> -                                    ecryptfs_inode_to_lower(inode),
>>> +     struct dentry *lower_dentry;
>>> +     struct inode *lower_inode;
>>> +
>>> +     lower_dentry = dentry ? ecryptfs_dentry_to_lower(dentry) : NULL;
>>> +     lower_inode = ecryptfs_inode_to_lower(inode);
>>> +     return ecryptfs_getxattr_lower(lower_dentry, lower_inode,
>>>                                      name, value, size);
>>
>> ecryptfs_getxattr_lower() doesn't appear to handle a NULL lower_dentry.
> 
> Yes, it just calls the lower-layer filesystem's getxattr inode
> operation with a NULL dentry, as intended.
> 
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index a86d537..dd509c8 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -1387,33 +1387,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>>>       case SECURITY_FS_USE_NATIVE:
>>>               break;
>>>       case SECURITY_FS_USE_XATTR:
>>> -             if (!inode->i_op->getxattr) {
>>> -                     isec->sid = sbsec->def_sid;
>>> -                     break;
>>> -             }
>>
>> I don't think we can remove the above or we'll end up invoking
>> inode->i_op->getxattr == NULL below.
> 
> Indeed, that's a bug, thanks. I've fixed that on my work.selinux branch.
> 
> With that fixed, could you possibly put this change to test?

Falls over during boot in generic_getxattr(), which still needs a
non-NULL dentry in the work.selinux branch.

Is there a reason that this being done separately from work.xattr?

Also, if we aren't going to call d_find_alias() there, we can likely
also drop the dget() and dput().

Re: [RFC 1/2] selinux: Stop looking up dentries from inodes

[Andreas Gruenbacher]

On Wed, Jun 1, 2016 at 3:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 05/31/2016 11:22 AM, Andreas Gruenbacher wrote:
>> With that fixed, could you possibly put this change to test?
>
> Falls over during boot in generic_getxattr(), which still needs a
> non-NULL dentry in the work.selinux branch.

dentry->d_sb needs to be changed to inode->i_sb there.

> Is there a reason that this being done separately from work.xattr?

I don't know how much work.xattr will shift still (and what I can
still add there), and this change is unrelated, at least so far.

> Also, if we aren't going to call d_find_alias() there, we can likely
> also drop the dget() and dput().

Ah, yes. I'll remove those, thanks.

Andreas

Re: [RFC 1/2] selinux: Stop looking up dentries from inodes

[Stephen Smalley]

On 06/01/2016 05:46 PM, Andreas Gruenbacher wrote:
> On Wed, Jun 1, 2016 at 3:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 05/31/2016 11:22 AM, Andreas Gruenbacher wrote:
>>> With that fixed, could you possibly put this change to test?
>>
>> Falls over during boot in generic_getxattr(), which still needs a
>> non-NULL dentry in the work.selinux branch.
> 
> dentry->d_sb needs to be changed to inode->i_sb there.
> 
>> Is there a reason that this being done separately from work.xattr?
> 
> I don't know how much work.xattr will shift still (and what I can
> still add there), and this change is unrelated, at least so far.
> 
>> Also, if we aren't going to call d_find_alias() there, we can likely
>> also drop the dget() and dput().
> 
> Ah, yes. I'll remove those, thanks.

Looks like you lost the assignment for dentry entirely when you removed
the dget/dput.  Still need to set it to opt_dentry or just use
opt_dentry directly.

BTW, SELinux will presently never call getxattr for 9p or cifs; those
filesystem types are not configured for xattrs in policy because they do
not truly support labeling (and if they did, we would probably use
SECURITY_LSM_NATIVE_LABELS => SECURITY_FS_USE_NATIVE as with nfsv4
rather than FS_USE_XATTR).  Just because they support xattrs does not
mean that they support security labeling.