On 2018-02-15 15:42, Paul Moore wrote: > On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs <r...@redhat.com> wrote: > > The arch_f pointer was added to the struct audit_krule in commit: > > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients") > > > > This is only used on addition and deletion of rules which isn't time > > critical and the arch field is likely to be one of the first fields, > > easily found iterating over the field type. This isn't worth the > > additional complexity and storage. Delete the field. > > > > Signed-off-by: Richard Guy Briggs <r...@redhat.com> > > --- > > include/linux/audit.h | 1 - > > kernel/auditfilter.c | 12 ++++++++---- > > 2 files changed, 8 insertions(+), 5 deletions(-) > > I haven't decided if I like the removal of arch_f or not, but I think > I might know where your oops/panic is coming from, thoughts below ...
Have you decided yet if you like the removal of the arch_f pointer or not? An updated v2 was provided the following day: https://www.redhat.com/archives/linux-audit/2018-February/msg00059.html I will send an updated patch if it seems worthwhile. > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index af410d9..64a3b0e 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -58,7 +58,6 @@ struct audit_krule { > > u32 field_count; > > char *filterkey; /* ties events to rules */ > > struct audit_field *fields; > > - struct audit_field *arch_f; /* quick access to arch field */ > > struct audit_field *inode_f; /* quick access to an inode field > > */ > > struct audit_watch *watch; /* associated watch */ > > struct audit_tree *tree; /* associated watched tree */ > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index 739a6d2..3343d1c 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, > > u32 *mask) > > > > static int audit_match_signal(struct audit_entry *entry) > > { > > - struct audit_field *arch = entry->rule.arch_f; > > + int i; > > + struct audit_field *arch; > > + > > + for (i = 0; i < entry->rule.field_count; i++) > > + if (entry->rule.fields[i].type == AUDIT_ARCH) { > > + arch = &entry->rule.fields[i]; > > + break; > > + } > > In the original code arch_f was initialized to NULL via the allocator > so the arch local variable was guaranteed to have a valid value or > NULL. Unfortunately, in your code if there is no AUDIT_ARCH field > arch could remain uninitialized which I believe could lead to the > oops/panic you are seeing. > > > if (!arch) { > > /* When arch is unspecified, we must check both masks on > > biarch > > @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct > > audit_rule_data *data, > > if (!gid_valid(f->gid)) > > goto exit_free; > > break; > > - case AUDIT_ARCH: > > - entry->rule.arch_f = f; > > - break; > > case AUDIT_SUBJ_USER: > > case AUDIT_SUBJ_ROLE: > > case AUDIT_SUBJ_TYPE: > > -- > > 1.8.3.1 > > -- > paul moore > www.paul-moore.com > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <r...@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 -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit