[09/11] audit: Allocate fsnotify mark independently of chunk
diff mbox series

Message ID 20180904160632.21210-10-jack@suse.cz
State New
Headers show
Series
  • audit: Fix various races when tagging and untagging mounts
Related show

Commit Message

Jan Kara Sept. 4, 2018, 4:06 p.m. UTC
Allocate fsnotify mark independently instead of embedding it inside
chunk. This will allow us to just replace chunk attached to mark when
growing / shrinking chunk instead of replacing mark attached to inode
which is a more complex operation.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 14 deletions(-)

Comments

Richard Guy Briggs Sept. 14, 2018, 2:09 p.m. UTC | #1
On 2018-09-04 18:06, Jan Kara wrote:
> Allocate fsnotify mark independently instead of embedding it inside
> chunk. This will allow us to just replace chunk attached to mark when
> growing / shrinking chunk instead of replacing mark attached to inode
> which is a more complex operation.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 0cd08b3581f1..481fdc190c2f 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -25,7 +25,7 @@ struct audit_tree {
>  struct audit_chunk {
>  	struct list_head hash;
>  	unsigned long key;
> -	struct fsnotify_mark mark;
> +	struct fsnotify_mark *mark;
>  	struct list_head trees;		/* with root here */
>  	int dead;
>  	int count;
> @@ -38,6 +38,11 @@ struct audit_chunk {
>  	} owners[];
>  };
>  
> +struct audit_tree_mark {
> +	struct fsnotify_mark mark;
> +	struct audit_chunk *chunk;
> +};
> +
>  static LIST_HEAD(tree_list);
>  static LIST_HEAD(prune_list);
>  static struct task_struct *prune_thread;
> @@ -73,6 +78,7 @@ static struct task_struct *prune_thread;
>   */
>  
>  static struct fsnotify_group *audit_tree_group;
> +static struct kmem_cache *audit_tree_mark_cachep __read_mostly;
>  
>  static struct audit_tree *alloc_tree(const char *s)
>  {
> @@ -142,10 +148,33 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
>  	call_rcu(&chunk->head, __put_chunk);
>  }
>  
> +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry)
> +{
> +	return container_of(entry, struct audit_tree_mark, mark);
> +}
> +
> +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> +{
> +	return audit_mark(mark)->chunk;
> +}
> +
>  static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
>  {
> -	struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
> +	struct audit_chunk *chunk = mark_chunk(entry);
>  	audit_mark_put_chunk(chunk);
> +	kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry));
> +}
> +
> +static struct fsnotify_mark *alloc_mark(void)
> +{
> +	struct audit_tree_mark *mark;

Would it make sense to call this local variable "amark" to indicate it
isn't a struct fsnotify_mark, but in fact an audit helper variant?

> +
> +	mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
> +	if (!mark)
> +		return NULL;
> +	fsnotify_init_mark(&mark->mark, audit_tree_group);
> +	mark->mark.mask = FS_IN_IGNORED;
> +	return &mark->mark;

There are no other places where it is used in this patch to name a
variable, but this one I found a bit confusing to follow the
"mark->mark"

>  }
>  
>  static struct audit_chunk *alloc_chunk(int count)
> @@ -159,6 +188,13 @@ static struct audit_chunk *alloc_chunk(int count)
>  	if (!chunk)
>  		return NULL;
>  
> +	chunk->mark = alloc_mark();
> +	if (!chunk->mark) {
> +		kfree(chunk);
> +		return NULL;
> +	}
> +	audit_mark(chunk->mark)->chunk = chunk;
> +
>  	INIT_LIST_HEAD(&chunk->hash);
>  	INIT_LIST_HEAD(&chunk->trees);
>  	chunk->count = count;
> @@ -167,8 +203,6 @@ static struct audit_chunk *alloc_chunk(int count)
>  		INIT_LIST_HEAD(&chunk->owners[i].list);
>  		chunk->owners[i].index = i;
>  	}
> -	fsnotify_init_mark(&chunk->mark, audit_tree_group);
> -	chunk->mark.mask = FS_IN_IGNORED;
>  	return chunk;
>  }
>  
> @@ -278,7 +312,7 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
>  static void untag_chunk(struct node *p)
>  {
>  	struct audit_chunk *chunk = find_chunk(p);
> -	struct fsnotify_mark *entry = &chunk->mark;
> +	struct fsnotify_mark *entry = chunk->mark;
>  	struct audit_chunk *new = NULL;
>  	struct audit_tree *owner;
>  	int size = chunk->count - 1;
> @@ -298,7 +332,7 @@ static void untag_chunk(struct node *p)
>  	if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
>  		mutex_unlock(&entry->group->mark_mutex);
>  		if (new)
> -			fsnotify_put_mark(&new->mark);
> +			fsnotify_put_mark(new->mark);
>  		goto out;
>  	}
>  
> @@ -322,9 +356,9 @@ static void untag_chunk(struct node *p)
>  	if (!new)
>  		goto Fallback;
>  
> -	if (fsnotify_add_mark_locked(&new->mark, entry->connector->obj,
> +	if (fsnotify_add_mark_locked(new->mark, entry->connector->obj,
>  				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
> -		fsnotify_put_mark(&new->mark);
> +		fsnotify_put_mark(new->mark);
>  		goto Fallback;
>  	}
>  
> @@ -344,7 +378,7 @@ static void untag_chunk(struct node *p)
>  	fsnotify_detach_mark(entry);
>  	mutex_unlock(&entry->group->mark_mutex);
>  	fsnotify_free_mark(entry);
> -	fsnotify_put_mark(&new->mark);	/* drop initial reference */
> +	fsnotify_put_mark(new->mark);	/* drop initial reference */
>  	goto out;
>  
>  Fallback:
> @@ -375,7 +409,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
>  		return -ENOMEM;
>  	}
>  
> -	entry = &chunk->mark;
> +	entry = chunk->mark;
>  	if (fsnotify_add_inode_mark_locked(entry, inode, 0)) {
>  		mutex_unlock(&audit_tree_group->mark_mutex);
>  		fsnotify_put_mark(entry);
> @@ -426,7 +460,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>  	if (!old_entry)
>  		return create_chunk(inode, tree);
>  
> -	old = container_of(old_entry, struct audit_chunk, mark);
> +	old = mark_chunk(old_entry)->chunk;
>  
>  	/* are we already there? */
>  	spin_lock(&hash_lock);
> @@ -447,7 +481,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>  		return -ENOMEM;
>  	}
>  
> -	chunk_entry = &chunk->mark;
> +	chunk_entry = chunk->mark;
>  
>  	/*
>  	 * mark_mutex protects mark from getting detached and thus also from
> @@ -457,7 +491,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>  		/* old_entry is being shot, lets just lie */
>  		mutex_unlock(&audit_tree_group->mark_mutex);
>  		fsnotify_put_mark(old_entry);
> -		fsnotify_put_mark(&chunk->mark);
> +		fsnotify_put_mark(chunk->mark);
>  		return -ENOENT;
>  	}
>  
> @@ -1011,7 +1045,7 @@ static int audit_tree_handle_event(struct fsnotify_group *group,
>  
>  static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group)
>  {
> -	struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
> +	struct audit_chunk *chunk = mark_chunk(entry);
>  
>  	evict_chunk(chunk);
>  
> @@ -1032,6 +1066,8 @@ static int __init audit_tree_init(void)
>  {
>  	int i;
>  
> +	audit_tree_mark_cachep = KMEM_CACHE(audit_tree_mark, SLAB_PANIC);
> +
>  	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
>  	if (IS_ERR(audit_tree_group))
>  		audit_panic("cannot initialize fsnotify group for rectree watches");
> -- 
> 2.16.4
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Jan Kara Sept. 17, 2018, 4:46 p.m. UTC | #2
On Fri 14-09-18 10:09:09, Richard Guy Briggs wrote:
> On 2018-09-04 18:06, Jan Kara wrote:
> > Allocate fsnotify mark independently instead of embedding it inside
> > chunk. This will allow us to just replace chunk attached to mark when
> > growing / shrinking chunk instead of replacing mark attached to inode
> > which is a more complex operation.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
...
> > +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> > +{
> > +	return audit_mark(mark)->chunk;
> > +}
> > +
> >  static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
> >  {
> > -	struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
> > +	struct audit_chunk *chunk = mark_chunk(entry);
> >  	audit_mark_put_chunk(chunk);
> > +	kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry));
> > +}
> > +
> > +static struct fsnotify_mark *alloc_mark(void)
> > +{
> > +	struct audit_tree_mark *mark;
> 
> Would it make sense to call this local variable "amark" to indicate it
> isn't a struct fsnotify_mark, but in fact an audit helper variant?
> 
> > +
> > +	mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
> > +	if (!mark)
> > +		return NULL;
> > +	fsnotify_init_mark(&mark->mark, audit_tree_group);
> > +	mark->mark.mask = FS_IN_IGNORED;
> > +	return &mark->mark;
> 
> There are no other places where it is used in this patch to name a
> variable, but this one I found a bit confusing to follow the
> "mark->mark"

Yeah, makes sense. I can do the change.

								Honza
Paul Moore Oct. 3, 2018, 10:08 p.m. UTC | #3
On Tue, Sep 4, 2018 at 12:06 PM Jan Kara <jack@suse.cz> wrote:
> Allocate fsnotify mark independently instead of embedding it inside
> chunk. This will allow us to just replace chunk attached to mark when
> growing / shrinking chunk instead of replacing mark attached to inode
> which is a more complex operation.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 0cd08b3581f1..481fdc190c2f 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -142,10 +148,33 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
>         call_rcu(&chunk->head, __put_chunk);
>  }
>
> +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry)
> +{
> +       return container_of(entry, struct audit_tree_mark, mark);
> +}
> +
> +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> +{
> +       return audit_mark(mark)->chunk;
> +}
> +

...

> @@ -426,7 +460,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
>         if (!old_entry)
>                 return create_chunk(inode, tree);
>
> -       old = container_of(old_entry, struct audit_chunk, mark);
> +       old = mark_chunk(old_entry)->chunk;

I'm pretty sure you mean the following instead?

  old = mark_chunk(old_entry);

>         /* are we already there? */
>         spin_lock(&hash_lock);
Paul Moore Oct. 3, 2018, 10:11 p.m. UTC | #4
On Mon, Sep 17, 2018 at 12:46 PM Jan Kara <jack@suse.cz> wrote:
> On Fri 14-09-18 10:09:09, Richard Guy Briggs wrote:
> > On 2018-09-04 18:06, Jan Kara wrote:
> > > Allocate fsnotify mark independently instead of embedding it inside
> > > chunk. This will allow us to just replace chunk attached to mark when
> > > growing / shrinking chunk instead of replacing mark attached to inode
> > > which is a more complex operation.
> > >
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> ...
> > > +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> > > +{
> > > +   return audit_mark(mark)->chunk;
> > > +}
> > > +
> > >  static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
> > >  {
> > > -   struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
> > > +   struct audit_chunk *chunk = mark_chunk(entry);
> > >     audit_mark_put_chunk(chunk);
> > > +   kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry));
> > > +}
> > > +
> > > +static struct fsnotify_mark *alloc_mark(void)
> > > +{
> > > +   struct audit_tree_mark *mark;
> >
> > Would it make sense to call this local variable "amark" to indicate it
> > isn't a struct fsnotify_mark, but in fact an audit helper variant?
> >
> > > +
> > > +   mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
> > > +   if (!mark)
> > > +           return NULL;
> > > +   fsnotify_init_mark(&mark->mark, audit_tree_group);
> > > +   mark->mark.mask = FS_IN_IGNORED;
> > > +   return &mark->mark;
> >
> > There are no other places where it is used in this patch to name a
> > variable, but this one I found a bit confusing to follow the
> > "mark->mark"
>
> Yeah, makes sense. I can do the change.

Unless you have to respin this patchset for some other reason I
wouldn't worry about it.

I've been working my way through the patchset this week (currently on
09/11) and I expect to finish it up today.  Assuming everything looks
good, I'm going to merge this into a working branch, include it in my
weekly -rc test builds, and beat on it for a couple of weeks.  If all
is good I'll merge it into audit/next after the upcoming merge window.

Thanks for your patience.
Richard Guy Briggs Oct. 3, 2018, 10:39 p.m. UTC | #5
On 2018-10-03 18:08, Paul Moore wrote:
> On Tue, Sep 4, 2018 at 12:06 PM Jan Kara <jack@suse.cz> wrote:
> > Allocate fsnotify mark independently instead of embedding it inside
> > chunk. This will allow us to just replace chunk attached to mark when
> > growing / shrinking chunk instead of replacing mark attached to inode
> > which is a more complex operation.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 50 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index 0cd08b3581f1..481fdc190c2f 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -142,10 +148,33 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
> >         call_rcu(&chunk->head, __put_chunk);
> >  }
> >
> > +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry)
> > +{
> > +       return container_of(entry, struct audit_tree_mark, mark);
> > +}
> > +
> > +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> > +{
> > +       return audit_mark(mark)->chunk;
> > +}
> > +
> 
> ...
> 
> > @@ -426,7 +460,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> >         if (!old_entry)
> >                 return create_chunk(inode, tree);
> >
> > -       old = container_of(old_entry, struct audit_chunk, mark);
> > +       old = mark_chunk(old_entry)->chunk;
> 
> I'm pretty sure you mean the following instead?
> 
>   old = mark_chunk(old_entry);

Yup, nice catch.  This could have been
	"old = audit_mark(old_entry)->chunk"
but the mark_chunk() helper avoids that.  (It compiles because it got
fixed/replaced in the following patch.)

This is why "old" should be called "old_chunk" and "old_entry" should be
called "old_mark" (which the latter is in the last patch).

> >         /* are we already there? */
> >         spin_lock(&hash_lock);
> 
> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Jan Kara Oct. 4, 2018, 6:57 a.m. UTC | #6
On Wed 03-10-18 18:08:14, Paul Moore wrote:
> On Tue, Sep 4, 2018 at 12:06 PM Jan Kara <jack@suse.cz> wrote:
> > Allocate fsnotify mark independently instead of embedding it inside
> > chunk. This will allow us to just replace chunk attached to mark when
> > growing / shrinking chunk instead of replacing mark attached to inode
> > which is a more complex operation.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 50 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index 0cd08b3581f1..481fdc190c2f 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -142,10 +148,33 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
> >         call_rcu(&chunk->head, __put_chunk);
> >  }
> >
> > +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry)
> > +{
> > +       return container_of(entry, struct audit_tree_mark, mark);
> > +}
> > +
> > +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
> > +{
> > +       return audit_mark(mark)->chunk;
> > +}
> > +
> 
> ...
> 
> > @@ -426,7 +460,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
> >         if (!old_entry)
> >                 return create_chunk(inode, tree);
> >
> > -       old = container_of(old_entry, struct audit_chunk, mark);
> > +       old = mark_chunk(old_entry)->chunk;
> 
> I'm pretty sure you mean the following instead?
> 
>   old = mark_chunk(old_entry);

Right, it gets fixed up in a later patch but it would be good to fix it here
(e.g. not to break bisection).

								Honza

Patch
diff mbox series

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 0cd08b3581f1..481fdc190c2f 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -25,7 +25,7 @@  struct audit_tree {
 struct audit_chunk {
 	struct list_head hash;
 	unsigned long key;
-	struct fsnotify_mark mark;
+	struct fsnotify_mark *mark;
 	struct list_head trees;		/* with root here */
 	int dead;
 	int count;
@@ -38,6 +38,11 @@  struct audit_chunk {
 	} owners[];
 };
 
+struct audit_tree_mark {
+	struct fsnotify_mark mark;
+	struct audit_chunk *chunk;
+};
+
 static LIST_HEAD(tree_list);
 static LIST_HEAD(prune_list);
 static struct task_struct *prune_thread;
@@ -73,6 +78,7 @@  static struct task_struct *prune_thread;
  */
 
 static struct fsnotify_group *audit_tree_group;
+static struct kmem_cache *audit_tree_mark_cachep __read_mostly;
 
 static struct audit_tree *alloc_tree(const char *s)
 {
@@ -142,10 +148,33 @@  static void audit_mark_put_chunk(struct audit_chunk *chunk)
 	call_rcu(&chunk->head, __put_chunk);
 }
 
+static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry)
+{
+	return container_of(entry, struct audit_tree_mark, mark);
+}
+
+static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark)
+{
+	return audit_mark(mark)->chunk;
+}
+
 static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
 {
-	struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+	struct audit_chunk *chunk = mark_chunk(entry);
 	audit_mark_put_chunk(chunk);
+	kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry));
+}
+
+static struct fsnotify_mark *alloc_mark(void)
+{
+	struct audit_tree_mark *mark;
+
+	mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
+	if (!mark)
+		return NULL;
+	fsnotify_init_mark(&mark->mark, audit_tree_group);
+	mark->mark.mask = FS_IN_IGNORED;
+	return &mark->mark;
 }
 
 static struct audit_chunk *alloc_chunk(int count)
@@ -159,6 +188,13 @@  static struct audit_chunk *alloc_chunk(int count)
 	if (!chunk)
 		return NULL;
 
+	chunk->mark = alloc_mark();
+	if (!chunk->mark) {
+		kfree(chunk);
+		return NULL;
+	}
+	audit_mark(chunk->mark)->chunk = chunk;
+
 	INIT_LIST_HEAD(&chunk->hash);
 	INIT_LIST_HEAD(&chunk->trees);
 	chunk->count = count;
@@ -167,8 +203,6 @@  static struct audit_chunk *alloc_chunk(int count)
 		INIT_LIST_HEAD(&chunk->owners[i].list);
 		chunk->owners[i].index = i;
 	}
-	fsnotify_init_mark(&chunk->mark, audit_tree_group);
-	chunk->mark.mask = FS_IN_IGNORED;
 	return chunk;
 }
 
@@ -278,7 +312,7 @@  static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old,
 static void untag_chunk(struct node *p)
 {
 	struct audit_chunk *chunk = find_chunk(p);
-	struct fsnotify_mark *entry = &chunk->mark;
+	struct fsnotify_mark *entry = chunk->mark;
 	struct audit_chunk *new = NULL;
 	struct audit_tree *owner;
 	int size = chunk->count - 1;
@@ -298,7 +332,7 @@  static void untag_chunk(struct node *p)
 	if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
 		mutex_unlock(&entry->group->mark_mutex);
 		if (new)
-			fsnotify_put_mark(&new->mark);
+			fsnotify_put_mark(new->mark);
 		goto out;
 	}
 
@@ -322,9 +356,9 @@  static void untag_chunk(struct node *p)
 	if (!new)
 		goto Fallback;
 
-	if (fsnotify_add_mark_locked(&new->mark, entry->connector->obj,
+	if (fsnotify_add_mark_locked(new->mark, entry->connector->obj,
 				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
-		fsnotify_put_mark(&new->mark);
+		fsnotify_put_mark(new->mark);
 		goto Fallback;
 	}
 
@@ -344,7 +378,7 @@  static void untag_chunk(struct node *p)
 	fsnotify_detach_mark(entry);
 	mutex_unlock(&entry->group->mark_mutex);
 	fsnotify_free_mark(entry);
-	fsnotify_put_mark(&new->mark);	/* drop initial reference */
+	fsnotify_put_mark(new->mark);	/* drop initial reference */
 	goto out;
 
 Fallback:
@@ -375,7 +409,7 @@  static int create_chunk(struct inode *inode, struct audit_tree *tree)
 		return -ENOMEM;
 	}
 
-	entry = &chunk->mark;
+	entry = chunk->mark;
 	if (fsnotify_add_inode_mark_locked(entry, inode, 0)) {
 		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(entry);
@@ -426,7 +460,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	if (!old_entry)
 		return create_chunk(inode, tree);
 
-	old = container_of(old_entry, struct audit_chunk, mark);
+	old = mark_chunk(old_entry)->chunk;
 
 	/* are we already there? */
 	spin_lock(&hash_lock);
@@ -447,7 +481,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		return -ENOMEM;
 	}
 
-	chunk_entry = &chunk->mark;
+	chunk_entry = chunk->mark;
 
 	/*
 	 * mark_mutex protects mark from getting detached and thus also from
@@ -457,7 +491,7 @@  static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		/* old_entry is being shot, lets just lie */
 		mutex_unlock(&audit_tree_group->mark_mutex);
 		fsnotify_put_mark(old_entry);
-		fsnotify_put_mark(&chunk->mark);
+		fsnotify_put_mark(chunk->mark);
 		return -ENOENT;
 	}
 
@@ -1011,7 +1045,7 @@  static int audit_tree_handle_event(struct fsnotify_group *group,
 
 static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group)
 {
-	struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
+	struct audit_chunk *chunk = mark_chunk(entry);
 
 	evict_chunk(chunk);
 
@@ -1032,6 +1066,8 @@  static int __init audit_tree_init(void)
 {
 	int i;
 
+	audit_tree_mark_cachep = KMEM_CACHE(audit_tree_mark, SLAB_PANIC);
+
 	audit_tree_group = fsnotify_alloc_group(&audit_tree_ops);
 	if (IS_ERR(audit_tree_group))
 		audit_panic("cannot initialize fsnotify group for rectree watches");