Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693280 > unrolled thread

[RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com> - 2017-07-21 01:00 +0200
    Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint  data "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-25 21:50 +0200
      Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint  data Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-07-25 22:20 +0200
        Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint  data Stefan Berger <stefanb@linux.vnet.ibm.com> - 2017-07-25 22:30 +0200
        Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint  data "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-25 22:50 +0200

#1693280 — [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

This patch adds an rbtree to the IMA namespace structure that stores a
namespaced version of iint->flags in ns_status struct. Similar to the
integrity_iint_cache, both the iint ns_struct are looked up using the
inode pointer value. The lookup, allocate, and insertion code is also
similar, except ns_struct is not free'd when the inode is free'd.
Instead, the lookup verifies the i_ino and i_generation fields are also a
match. A lazy clean up of the rbtree that removes free'd inodes could be
implemented to reclaim the invalid entries.

Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
---
 include/linux/ima.h             |   3 +
 security/integrity/ima/ima.h    |  16 ++++++
 security/integrity/ima/ima_ns.c | 120 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 11e4841..3fdf56f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -111,6 +111,9 @@ struct ima_namespace {
 	struct user_namespace *user_ns;
 	struct ns_common ns;
 	struct ima_namespace *parent;
+	struct rb_root ns_status_tree;
+	rwlock_t ns_status_lock;
+	struct kmem_cache *ns_status_cache;
 };
 
 extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8a8234a..5ab769a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -128,6 +128,14 @@ static inline void ima_load_kexec_buffer(void) {}
  */
 extern bool ima_canonical_fmt;
 
+struct ns_status {
+	struct rb_node rb_node;
+	struct inode *inode;
+	ino_t i_ino;
+	u32 i_generation;
+	unsigned long flags;
+};
+
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
@@ -293,11 +301,19 @@ static inline int ima_read_xattr(struct dentry *dentry,
 
 #ifdef CONFIG_IMA_NS
 int ima_ns_init(void);
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode);
 #else
 static inline int ima_ns_init(void)
 {
 	return 0;
 }
+
+static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+						  struct inode *inode)
+{
+	return NULL;
+}
 #endif /* CONFIG_IMA_NS */
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 383217b..5ec5a4b 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -20,6 +20,9 @@ static void get_ima_ns(struct ima_namespace *ns);
 
 int ima_init_namespace(struct ima_namespace *ns)
 {
+	ns->ns_status_tree = RB_ROOT;
+	rwlock_init(&ns->ns_status_lock);
+	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
 	return 0;
 }
 
@@ -98,10 +101,24 @@ struct ima_namespace *copy_ima(unsigned long flags,
 	return new_ns;
 }
 
+static void free_ns_status_cache(struct ima_namespace *ns)
+{
+	struct ns_status *status, *next;
+
+	write_lock(&ns->ns_status_lock);
+	rbtree_postorder_for_each_entry_safe(status, next,
+					     &ns->ns_status_tree, rb_node)
+		kmem_cache_free(ns->ns_status_cache, status);
+	ns->ns_status_tree = RB_ROOT;
+	write_unlock(&ns->ns_status_lock);
+	kmem_cache_destroy(ns->ns_status_cache);
+}
+
 static void destroy_ima_ns(struct ima_namespace *ns)
 {
 	put_user_ns(ns->user_ns);
 	ns_free_inum(&ns->ns);
+	free_ns_status_cache(ns);
 	kfree(ns);
 }
 
@@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
 	.parent = NULL,
 };
 EXPORT_SYMBOL(init_ima_ns);
+
+/*
+ * __ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
+					      struct inode *inode)
+{
+	struct ns_status *status;
+	struct rb_node *n = ns->ns_status_tree.rb_node;
+
+	while (n) {
+		status = rb_entry(n, struct ns_status, rb_node);
+
+		if (inode < status->inode)
+			n = n->rb_left;
+		else if (inode->i_ino > status->i_ino)
+			n = n->rb_right;
+		else
+			break;
+	}
+	if (!n)
+		return NULL;
+
+	return status;
+}
+
+/*
+ * ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
+					    struct inode *inode)
+{
+	struct ns_status *status;
+
+	read_lock(&ns->ns_status_lock);
+	status = __ima_ns_status_find(ns, inode);
+	read_unlock(&ns->ns_status_lock);
+
+	return status;
+}
+
+void insert_ns_status(struct ima_namespace *ns, struct inode *inode,
+		      struct ns_status *status)
+{
+	struct rb_node **p;
+	struct rb_node *node, *parent = NULL;
+	struct ns_status *test_status;
+
+	p = &ns->ns_status_tree.rb_node;
+	while (*p) {
+		parent = *p;
+		test_status = rb_entry(parent, struct ns_status, rb_node);
+		if (inode < test_status->inode)
+			p = &(*p)->rb_left;
+		else
+			p = &(*p)->rb_right;
+	}
+	node = &status->rb_node;
+	rb_link_node(node, parent, p);
+	rb_insert_color(node, &ns->ns_status_tree);
+}
+
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode)
+{
+	struct ns_status *status;
+	int skip_insert = 0;
+
+	status = ima_ns_status_find(ns, inode);
+	if (status) {
+		/*
+		 * Unlike integrity_iint_cache we are not free'ing the
+		 * ns_status data when the inode is free'd. So, in addition to
+		 * checking the inode pointer, we need to make sure the
+		 * (i_generation, i_ino) pair matches as well. In the future
+		 * we might want to add support for lazily walking the rbtree
+		 * to clean it up.
+		 */
+		if (inode->i_ino == status->i_ino &&
+		    inode->i_generation == status->i_generation)
+			return status;
+
+		/* Same inode number is reused, overwrite the ns_status */
+		skip_insert = 1;
+	} else {
+		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
+		if (!status)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	write_lock(&ns->ns_status_lock);
+
+	if (!skip_insert)
+		insert_ns_status(ns, inode, status);
+
+	status->inode = inode;
+	status->i_ino = inode->i_ino;
+	status->i_generation = inode->i_generation;
+	status->flags = 0UL;
+	write_unlock(&ns->ns_status_lock);
+
+	return status;
+}
-- 
2.9.4

[toc] | [next] | [standalone]

#1696161 — Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

...
> +static void free_ns_status_cache(struct ima_namespace *ns)
> +{
> +	struct ns_status *status, *next;
> +
> +	write_lock(&ns->ns_status_lock);
> +	rbtree_postorder_for_each_entry_safe(status, next,
> +					     &ns->ns_status_tree, rb_node)
> +		kmem_cache_free(ns->ns_status_cache, status);
> +	ns->ns_status_tree = RB_ROOT;
> +	write_unlock(&ns->ns_status_lock);
> +	kmem_cache_destroy(ns->ns_status_cache);
> +}
> +
>  static void destroy_ima_ns(struct ima_namespace *ns)
>  {
>  	put_user_ns(ns->user_ns);
>  	ns_free_inum(&ns->ns);
> +	free_ns_status_cache(ns);
>  	kfree(ns);
>  }
>  
> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
>  	.parent = NULL,
>  };
>  EXPORT_SYMBOL(init_ima_ns);
> +
> +/*
> + * __ima_ns_status_find - return the ns_status associated with an inode
> + */
> +static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
> +					      struct inode *inode)
> +{
> +	struct ns_status *status;
> +	struct rb_node *n = ns->ns_status_tree.rb_node;
> +
> +	while (n) {
> +		status = rb_entry(n, struct ns_status, rb_node);
> +
> +		if (inode < status->inode)
> +			n = n->rb_left;
> +		else if (inode->i_ino > status->i_ino)
> +			n = n->rb_right;
> +		else
> +			break;
> +	}
> +	if (!n)
> +		return NULL;
> +
> +	return status;
> +}
> +
> +/*
> + * ima_ns_status_find - return the ns_status associated with an inode
> + */
> +static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
> +					    struct inode *inode)
> +{
> +	struct ns_status *status;
> +
> +	read_lock(&ns->ns_status_lock);
> +	status = __ima_ns_status_find(ns, inode);
> +	read_unlock(&ns->ns_status_lock);
> +
> +	return status;
> +}
...
> +
> +struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
> +				    struct inode *inode)
> +{
> +	struct ns_status *status;
> +	int skip_insert = 0;
> +
> +	status = ima_ns_status_find(ns, inode);
> +	if (status) {
> +		/*
> +		 * Unlike integrity_iint_cache we are not free'ing the
> +		 * ns_status data when the inode is free'd. So, in addition to
> +		 * checking the inode pointer, we need to make sure the
> +		 * (i_generation, i_ino) pair matches as well. In the future
> +		 * we might want to add support for lazily walking the rbtree
> +		 * to clean it up.
> +		 */
> +		if (inode->i_ino == status->i_ino &&
> +		    inode->i_generation == status->i_generation)
> +			return status;
> +
> +		/* Same inode number is reused, overwrite the ns_status */
> +		skip_insert = 1;
> +	} else {
> +		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
> +		if (!status)
> +			return ERR_PTR(-ENOMEM);
> +	}

What prevents the status from being freed between the read_lock
in ima_ns_status_find() and the write_lock in the following line?

IIUC it's that ns is always current's ima_ns, which will pin the ns
and cause no statuses to be freed.  But then the ns should probably
not be passed in here?  Or a comment should say that ns must be
pinned?

Just trying to make sure I understand the locking.

> +	write_lock(&ns->ns_status_lock);
> +
> +	if (!skip_insert)
> +		insert_ns_status(ns, inode, status);
> +
> +	status->inode = inode;
> +	status->i_ino = inode->i_ino;
> +	status->i_generation = inode->i_generation;
> +	status->flags = 0UL;
> +	write_unlock(&ns->ns_status_lock);
> +
> +	return status;
> +}
> -- 
> 2.9.4

[toc] | [prev] | [next] | [standalone]

#1696285 — Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> ...
> > +static void free_ns_status_cache(struct ima_namespace *ns)
> > +{
> > +	struct ns_status *status, *next;
> > +
> > +	write_lock(&ns->ns_status_lock);
> > +	rbtree_postorder_for_each_entry_safe(status, next,
> > +					     &ns->ns_status_tree, rb_node)
> > +		kmem_cache_free(ns->ns_status_cache, status);
> > +	ns->ns_status_tree = RB_ROOT;
> > +	write_unlock(&ns->ns_status_lock);
> > +	kmem_cache_destroy(ns->ns_status_cache);
> > +}
> > +
> >  static void destroy_ima_ns(struct ima_namespace *ns)
> >  {
> >  	put_user_ns(ns->user_ns);
> >  	ns_free_inum(&ns->ns);
> > +	free_ns_status_cache(ns);
> >  	kfree(ns);
> >  }
> >  
> > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> >  	.parent = NULL,
> >  };
> >  EXPORT_SYMBOL(init_ima_ns);
> > +
> > +/*
> > + * __ima_ns_status_find - return the ns_status associated with an inode
> > + */
> > +static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
> > +					      struct inode *inode)
> > +{
> > +	struct ns_status *status;
> > +	struct rb_node *n = ns->ns_status_tree.rb_node;
> > +
> > +	while (n) {
> > +		status = rb_entry(n, struct ns_status, rb_node);
> > +
> > +		if (inode < status->inode)
> > +			n = n->rb_left;
> > +		else if (inode->i_ino > status->i_ino)
> > +			n = n->rb_right;
> > +		else
> > +			break;
> > +	}
> > +	if (!n)
> > +		return NULL;
> > +
> > +	return status;
> > +}
> > +
> > +/*
> > + * ima_ns_status_find - return the ns_status associated with an inode
> > + */
> > +static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
> > +					    struct inode *inode)
> > +{
> > +	struct ns_status *status;
> > +
> > +	read_lock(&ns->ns_status_lock);
> > +	status = __ima_ns_status_find(ns, inode);
> > +	read_unlock(&ns->ns_status_lock);
> > +
> > +	return status;
> > +}
> ...
> > +
> > +struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
> > +				    struct inode *inode)
> > +{
> > +	struct ns_status *status;
> > +	int skip_insert = 0;
> > +
> > +	status = ima_ns_status_find(ns, inode);
> > +	if (status) {
> > +		/*
> > +		 * Unlike integrity_iint_cache we are not free'ing the
> > +		 * ns_status data when the inode is free'd. So, in addition to
> > +		 * checking the inode pointer, we need to make sure the
> > +		 * (i_generation, i_ino) pair matches as well. In the future
> > +		 * we might want to add support for lazily walking the rbtree
> > +		 * to clean it up.
> > +		 */
> > +		if (inode->i_ino == status->i_ino &&
> > +		    inode->i_generation == status->i_generation)
> > +			return status;
> > +
> > +		/* Same inode number is reused, overwrite the ns_status */
> > +		skip_insert = 1;
> > +	} else {
> > +		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
> > +		if (!status)
> > +			return ERR_PTR(-ENOMEM);
> > +	}
> 
> What prevents the status from being freed between the read_lock
> in ima_ns_status_find() and the write_lock in the following line?
> 
> IIUC it's that ns is always current's ima_ns, which will pin the ns
> and cause no statuses to be freed.  But then the ns should probably
> not be passed in here?  Or a comment should say that ns must be
> pinned?
> 
> Just trying to make sure I understand the locking.

iint's are only freed after the last reference to the inode is deleted
in __fput().  Refer to ima_file_free().  ns_status is a bit different
in that they are freed on namespace cleanup.

Mimi

> > +	write_lock(&ns->ns_status_lock);
> > +
> > +	if (!skip_insert)
> > +		insert_ns_status(ns, inode, status);
> > +
> > +	status->inode = inode;
> > +	status->i_ino = inode->i_ino;
> > +	status->i_generation = inode->i_generation;
> > +	status->flags = 0UL;
> > +	write_unlock(&ns->ns_status_lock);
> > +
> > +	return status;
> > +}
> > -- 
> > 2.9.
> 

[toc] | [prev] | [next] | [standalone]

#1696322 — Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On 07/25/2017 04:15 PM, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
>> ...
>>> +static void free_ns_status_cache(struct ima_namespace *ns)
>>> +{
>>> +	struct ns_status *status, *next;
>>> +
>>> +	write_lock(&ns->ns_status_lock);
>>> +	rbtree_postorder_for_each_entry_safe(status, next,
>>> +					     &ns->ns_status_tree, rb_node)
>>> +		kmem_cache_free(ns->ns_status_cache, status);
>>> +	ns->ns_status_tree = RB_ROOT;
>>> +	write_unlock(&ns->ns_status_lock);
>>> +	kmem_cache_destroy(ns->ns_status_cache);
>>> +}
>>> +
>>>   static void destroy_ima_ns(struct ima_namespace *ns)
>>>   {
>>>   	put_user_ns(ns->user_ns);
>>>   	ns_free_inum(&ns->ns);
>>> +	free_ns_status_cache(ns);
>>>   	kfree(ns);
>>>   }
>>>   
>>> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
>>>   	.parent = NULL,
>>>   };
>>>   EXPORT_SYMBOL(init_ima_ns);
>>> +
>>> +/*
>>> + * __ima_ns_status_find - return the ns_status associated with an inode
>>> + */
>>> +static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
>>> +					      struct inode *inode)
>>> +{
>>> +	struct ns_status *status;
>>> +	struct rb_node *n = ns->ns_status_tree.rb_node;
>>> +
>>> +	while (n) {
>>> +		status = rb_entry(n, struct ns_status, rb_node);
>>> +
>>> +		if (inode < status->inode)
>>> +			n = n->rb_left;
>>> +		else if (inode->i_ino > status->i_ino)
>>> +			n = n->rb_right;
>>> +		else
>>> +			break;
>>> +	}
>>> +	if (!n)
>>> +		return NULL;
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +/*
>>> + * ima_ns_status_find - return the ns_status associated with an inode
>>> + */
>>> +static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
>>> +					    struct inode *inode)
>>> +{
>>> +	struct ns_status *status;
>>> +
>>> +	read_lock(&ns->ns_status_lock);
>>> +	status = __ima_ns_status_find(ns, inode);
>>> +	read_unlock(&ns->ns_status_lock);
>>> +
>>> +	return status;
>>> +}
>> ...
>>> +
>>> +struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
>>> +				    struct inode *inode)
>>> +{
>>> +	struct ns_status *status;
>>> +	int skip_insert = 0;
>>> +
>>> +	status = ima_ns_status_find(ns, inode);
>>> +	if (status) {
>>> +		/*
>>> +		 * Unlike integrity_iint_cache we are not free'ing the
>>> +		 * ns_status data when the inode is free'd. So, in addition to
>>> +		 * checking the inode pointer, we need to make sure the
>>> +		 * (i_generation, i_ino) pair matches as well. In the future
>>> +		 * we might want to add support for lazily walking the rbtree
>>> +		 * to clean it up.
>>> +		 */
>>> +		if (inode->i_ino == status->i_ino &&
>>> +		    inode->i_generation == status->i_generation)
>>> +			return status;
>>> +
>>> +		/* Same inode number is reused, overwrite the ns_status */
>>> +		skip_insert = 1;
>>> +	} else {
>>> +		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
>>> +		if (!status)
>>> +			return ERR_PTR(-ENOMEM);
>>> +	}
>> What prevents the status from being freed between the read_lock
>> in ima_ns_status_find() and the write_lock in the following line?
>>
>> IIUC it's that ns is always current's ima_ns, which will pin the ns
>> and cause no statuses to be freed.  But then the ns should probably
>> not be passed in here?  Or a comment should say that ns must be
>> pinned?
>>
>> Just trying to make sure I understand the locking.
> iint's are only freed after the last reference to the inode is deleted
> in __fput().  Refer to ima_file_free().  ns_status is a bit different
> in that they are freed on namespace cleanup.

It should be possible to move the write_lock() above the

status = ima_ns_status_find(ns, inode);


and instead call __ima_ns_status_find() with the write_lock() held.

     Stefan


>
> Mimi
>
>>> +	write_lock(&ns->ns_status_lock);
>>> +
>>> +	if (!skip_insert)
>>> +		insert_ns_status(ns, inode, status);
>>> +
>>> +	status->inode = inode;
>>> +	status->i_ino = inode->i_ino;
>>> +	status->i_generation = inode->i_generation;
>>> +	status->flags = 0UL;
>>> +	write_unlock(&ns->ns_status_lock);
>>> +
>>> +	return status;
>>> +}
>>> -- 
>>> 2.9.

[toc] | [prev] | [next] | [standalone]

#1696406 — Re: [RFC PATCH 2/5] ima: Add ns_status for storing namespaced iint data

On Tue, Jul 25, 2017 at 04:15:25PM -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> > ...
> > > +static void free_ns_status_cache(struct ima_namespace *ns)
> > > +{
> > > +	struct ns_status *status, *next;
> > > +
> > > +	write_lock(&ns->ns_status_lock);
> > > +	rbtree_postorder_for_each_entry_safe(status, next,
> > > +					     &ns->ns_status_tree, rb_node)
> > > +		kmem_cache_free(ns->ns_status_cache, status);
> > > +	ns->ns_status_tree = RB_ROOT;
> > > +	write_unlock(&ns->ns_status_lock);
> > > +	kmem_cache_destroy(ns->ns_status_cache);
> > > +}
> > > +
> > >  static void destroy_ima_ns(struct ima_namespace *ns)
> > >  {
> > >  	put_user_ns(ns->user_ns);
> > >  	ns_free_inum(&ns->ns);
> > > +	free_ns_status_cache(ns);
> > >  	kfree(ns);
> > >  }
> > >  
> > > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > >  	.parent = NULL,
> > >  };
> > >  EXPORT_SYMBOL(init_ima_ns);
> > > +
> > > +/*
> > > + * __ima_ns_status_find - return the ns_status associated with an inode
> > > + */
> > > +static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
> > > +					      struct inode *inode)
> > > +{
> > > +	struct ns_status *status;
> > > +	struct rb_node *n = ns->ns_status_tree.rb_node;
> > > +
> > > +	while (n) {
> > > +		status = rb_entry(n, struct ns_status, rb_node);
> > > +
> > > +		if (inode < status->inode)
> > > +			n = n->rb_left;
> > > +		else if (inode->i_ino > status->i_ino)
> > > +			n = n->rb_right;
> > > +		else
> > > +			break;
> > > +	}
> > > +	if (!n)
> > > +		return NULL;
> > > +
> > > +	return status;
> > > +}
> > > +
> > > +/*
> > > + * ima_ns_status_find - return the ns_status associated with an inode
> > > + */
> > > +static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
> > > +					    struct inode *inode)
> > > +{
> > > +	struct ns_status *status;
> > > +
> > > +	read_lock(&ns->ns_status_lock);
> > > +	status = __ima_ns_status_find(ns, inode);
> > > +	read_unlock(&ns->ns_status_lock);
> > > +
> > > +	return status;
> > > +}
> > ...
> > > +
> > > +struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
> > > +				    struct inode *inode)
> > > +{
> > > +	struct ns_status *status;
> > > +	int skip_insert = 0;
> > > +
> > > +	status = ima_ns_status_find(ns, inode);
> > > +	if (status) {
> > > +		/*
> > > +		 * Unlike integrity_iint_cache we are not free'ing the
> > > +		 * ns_status data when the inode is free'd. So, in addition to
> > > +		 * checking the inode pointer, we need to make sure the
> > > +		 * (i_generation, i_ino) pair matches as well. In the future
> > > +		 * we might want to add support for lazily walking the rbtree
> > > +		 * to clean it up.
> > > +		 */
> > > +		if (inode->i_ino == status->i_ino &&
> > > +		    inode->i_generation == status->i_generation)
> > > +			return status;
> > > +
> > > +		/* Same inode number is reused, overwrite the ns_status */
> > > +		skip_insert = 1;
> > > +	} else {
> > > +		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
> > > +		if (!status)
> > > +			return ERR_PTR(-ENOMEM);
> > > +	}
> > 
> > What prevents the status from being freed between the read_lock
> > in ima_ns_status_find() and the write_lock in the following line?
> > 
> > IIUC it's that ns is always current's ima_ns, which will pin the ns
> > and cause no statuses to be freed.  But then the ns should probably
> > not be passed in here?  Or a comment should say that ns must be
> > pinned?
> > 
> > Just trying to make sure I understand the locking.
> 
> iint's are only freed after the last reference to the inode is deleted
> in __fput().  Refer to ima_file_free().  ns_status is a bit different
> in that they are freed on namespace cleanup.

Ok, thanks - that sounds ok then.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web