Re: [PATCH ghak25 v2 4/9] audit: record nfcfg params

On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > Record the auditable parameters of any non-empty netfilter table > configuration change. > > See: https://github.com/linux-audit/audit-kernel/issues/25 > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > include/linux/audit.h | 11 +++++++++++ > kernel/auditsc.c | 16 ++++++++++++++++ > 2 files changed, 27 insertions(+) I can not see a good reason why this patch is separate from patches 5 and 6, please squash them down into one patch. As it currently stands the logging function introduced here has no caller so it is pointless by itself. Strive to make an individual patch have some significance on its own whenever possible. This will also help you write a better commit description, right now the commit description tells me nothing, but if you bring in the other patches you can talk about consolidating similar code into a common function.

> diff --git a/include/linux/audit.h b/include/linux/audit.h > index f9ceae57ca8d..96cabb095eed 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -379,6 +379,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, > extern void __audit_fanotify(unsigned int response); > extern void __audit_tk_injoffset(struct timespec64 offset); > extern void __audit_ntp_log(const struct audit_ntp_data *ad); > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries); > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > { > @@ -514,6 +515,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) > __audit_ntp_log(ad); > } > > +static inline void audit_nf_cfg(const char *name, u8 af, int nentries) > +{ > + if (!audit_dummy_context()) > + __audit_nf_cfg(name, af, nentries); See my comments below about audit_enabled.

> +} > + > extern int audit_n_rules; > extern int audit_signals; > #else /* CONFIG_AUDITSYSCALL */ > @@ -646,6 +653,10 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) > > static inline void audit_ptrace(struct task_struct *t) > { } > + > +static inline void audit_nf_cfg(const char *name, u8 af, int nentries) > +{ } > + > #define audit_n_rules 0 > #define audit_signals 0 > #endif /* CONFIG_AUDITSYSCALL */ > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 4effe01ebbe2..4e1df4233cd3 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2545,6 +2545,22 @@ void __audit_ntp_log(const struct audit_ntp_data *ad) > audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST); > } > > +void __audit_nf_cfg(const char *name, u8 af, int nentries) Should nentries be an unsigned int?

> +{ > + struct audit_buffer *ab; > + struct audit_context *context = audit_context(); This is a good example of why the context of a caller matters; taken alone I would say that we need a check for audit_enabled here, but if we look at the latter patches we can see that the caller already has the audit_enabled check. Considering that the caller is already doing an audit_enabled check, we might want to consider moving the audit_enabled check into audit_nf_cfg() where we do the dummy context check. It's a static inline so there shouldn't be a performance impact and it makes the caller's code cleaner. > + if (!nentries) > + return; > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG); Why do we need the context variable, why not just call audit_context() here directly?

> + if (!ab) > + return; /* audit_panic or being filtered */ We generally don't add comments when audit_log_start() fails elsewhere, please don't do it here.

> + audit_log_format(ab, "table=%s family=%u entries=%u", > + name, af, nentries); > + audit_log_end(ab); > +} > +EXPORT_SYMBOL_GPL(__audit_nf_cfg); > + > static void audit_log_task(struct audit_buffer *ab) > { > kuid_t auid, uid; > -- > 1.8.3.1 -- paul moore www.paul-moore.com