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

[Stefan Berger]

On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:

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 
---
  include/linux/ima.h |   3 +
  security/integrity/ima/ima.h|  16 ++
  security/integrity/ima/ima_ns.c | 120 
  3 files changed, 139 insertions(+)


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


Above you are comparing with the inode ptr, here with i_ino. Why can you 
not compare with inode both times. Also the insertion only seems to 
consider the inode ptr.


   Stefan

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

[Stefan Berger]

On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:

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 
---
  include/linux/ima.h |   3 +
  security/integrity/ima/ima.h|  16 ++
  security/integrity/ima/ima_ns.c | 120 
  3 files changed, 139 insertions(+)


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


Above you are comparing with the inode ptr, here with i_ino. Why can you 
not compare with inode both times. Also the insertion only seems to 
consider the inode ptr.


   Stefan

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

[Serge E. Hallyn]

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_status_lock);
> > > + rbtree_postorder_for_each_entry_safe(status, next,
> > > +  >ns_status_tree, rb_node)
> > > + kmem_cache_free(ns->ns_status_cache, status);
> > > + ns->ns_status_tree = RB_ROOT;
> > > + write_unlock(>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);
> > > + 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_status_lock);
> > > + status = __ima_ns_status_find(ns, inode);
> > > + read_unlock(>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.

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

[Serge E. Hallyn]

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_status_lock);
> > > + rbtree_postorder_for_each_entry_safe(status, next,
> > > +  >ns_status_tree, rb_node)
> > > + kmem_cache_free(ns->ns_status_cache, status);
> > > + ns->ns_status_tree = RB_ROOT;
> > > + write_unlock(>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);
> > > + 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_status_lock);
> > > + status = __ima_ns_status_find(ns, inode);
> > > + read_unlock(>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.

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

[Stefan Berger]

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_status_lock);
+   rbtree_postorder_for_each_entry_safe(status, next,
+>ns_status_tree, rb_node)
+   kmem_cache_free(ns->ns_status_cache, status);
+   ns->ns_status_tree = RB_ROOT;
+   write_unlock(>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);
+   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_status_lock);
+   status = __ima_ns_status_find(ns, inode);
+   read_unlock(>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_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_status_lock);
+
+   return status;
+}
--
2.9.

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

[Stefan Berger]

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_status_lock);
+   rbtree_postorder_for_each_entry_safe(status, next,
+>ns_status_tree, rb_node)
+   kmem_cache_free(ns->ns_status_cache, status);
+   ns->ns_status_tree = RB_ROOT;
+   write_unlock(>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);
+   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_status_lock);
+   status = __ima_ns_status_find(ns, inode);
+   read_unlock(>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_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_status_lock);
+
+   return status;
+}
--
2.9.

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

[Mimi Zohar]

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_status_lock);
> > +   rbtree_postorder_for_each_entry_safe(status, next,
> > +>ns_status_tree, rb_node)
> > +   kmem_cache_free(ns->ns_status_cache, status);
> > +   ns->ns_status_tree = RB_ROOT;
> > +   write_unlock(>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);
> > +   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_status_lock);
> > +   status = __ima_ns_status_find(ns, inode);
> > +   read_unlock(>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_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_status_lock);
> > +
> > +   return status;
> > +}
> > -- 
> > 2.9.
> 

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

[Mimi Zohar]

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_status_lock);
> > +   rbtree_postorder_for_each_entry_safe(status, next,
> > +>ns_status_tree, rb_node)
> > +   kmem_cache_free(ns->ns_status_cache, status);
> > +   ns->ns_status_tree = RB_ROOT;
> > +   write_unlock(>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);
> > +   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_status_lock);
> > +   status = __ima_ns_status_find(ns, inode);
> > +   read_unlock(>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_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_status_lock);
> > +
> > +   return status;
> > +}
> > -- 
> > 2.9.
> 

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

[Serge E. Hallyn]

...
> +static void free_ns_status_cache(struct ima_namespace *ns)
> +{
> + struct ns_status *status, *next;
> +
> + write_lock(>ns_status_lock);
> + rbtree_postorder_for_each_entry_safe(status, next,
> +  >ns_status_tree, rb_node)
> + kmem_cache_free(ns->ns_status_cache, status);
> + ns->ns_status_tree = RB_ROOT;
> + write_unlock(>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);
> + 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_status_lock);
> + status = __ima_ns_status_find(ns, inode);
> + read_unlock(>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_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_status_lock);
> +
> + return status;
> +}
> -- 
> 2.9.4

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

[Serge E. Hallyn]

...
> +static void free_ns_status_cache(struct ima_namespace *ns)
> +{
> + struct ns_status *status, *next;
> +
> + write_lock(>ns_status_lock);
> + rbtree_postorder_for_each_entry_safe(status, next,
> +  >ns_status_tree, rb_node)
> + kmem_cache_free(ns->ns_status_cache, status);
> + ns->ns_status_tree = RB_ROOT;
> + write_unlock(>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);
> + 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_status_lock);
> + status = __ima_ns_status_find(ns, inode);
> + read_unlock(>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_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_status_lock);
> +
> + return status;
> +}
> -- 
> 2.9.4