| Message ID | 20180904160632.21210-10-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | audit: Fix various races when tagging and untagging mounts | expand |
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
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
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);
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.
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
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
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");
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(-)