Re: [PATCH 09/10] audit: Allocate fsnotify mark independently of chunk
On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack(a)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(a)suse.cz>
---
kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 45 insertions(+), 14 deletions(-)
...
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index bce3b04a365d..aec9b27a20ff 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -38,6 +38,11 @@ struct audit_chunk {
} owners[];
};
+struct audit_tree_mark {
+ struct fsnotify_mark fsn_mark;
+ struct audit_chunk *chunk;
+};
It's probably okay to just call it "mark" considering we call
fsnotify_mark fields "mark" elsewhere. If we are going to change it
in one spot we should probably change it other places as well for the
sake of readability.
2,10 +148,28 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
call_rcu(&chunk->head, __put_chunk);
}
+static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
+{
+ return container_of(entry, struct audit_tree_mark, fsn_mark);
+}
When I see a symbol in all caps I think "macro definition", but this
isn't a macro definition. I would suggest a different name, dropping
the caps, or converting it into a macro.
Also, unless I missed a call, it seems like after patch 10 all callers
of AUDIT_M end up getting the chunk field; maybe it is better if
AUDIT_M() ends up returning the audit_chunk pointer instead of the
audit_tree_mark pointer (and of course a name change if this is a
reasonable change)?
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 = AUDIT_M(entry)->chunk;
audit_mark_put_chunk(chunk);
+ kmem_cache_free(audit_tree_mark_cachep, entry);
+}
I think you've said you've already fixed the above (thanks for the
review Amir!).
+static struct fsnotify_mark *alloc_fsnotify_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->fsn_mark, audit_tree_group);
+ mark->fsn_mark.mask = FS_IN_IGNORED;
+ return &mark->fsn_mark;
}
Can't we just call it alloc_mark()? Or did you create the function
earlier in the patchset and I'm missing it now?
[SIDE NOTE: I have some rather big disagreements with the current
naming in this file, but keeping things consistent seems to be a Good
Thing (once again, this is an existing problem not specific to your
patches).]
--
paul moore
www.paul-moore.com