[RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient

Post by Richard Guy Briggs
Most uses of audit_enabled don't care about the distinction between
AUDIT_ON and AUDIT_LOCKED, so using audit_enabled as a boolean makes
more sense and is easier to read. Most uses of audit_enabled treat it as
a boolean, so switch the remaining AUDIT_OFF usage to simply use
audit_enabled as a boolean where applicable.
See: https://github.com/linux-audit/audit-kernel/issues/86
---
drivers/tty/tty_audit.c | 2 +-
include/net/xfrm.h | 2 +-
kernel/audit.c | 8 ++++----
net/netfilter/xt_AUDIT.c | 2 +-
net/netlabel/netlabel_user.c | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)

I'm not sure I like this idea. Yes, technically this change is
functionally equivalent but I worry that this will increase the chance
that non-audit folks will mistake audit_enabled as a true/false value
when it is not. It might work now, but I worry about some subtle
problem in the future.

If you are bothered by the comparison to 0 (magic numbers), you could
move the AUDIT_OFF/AUDIT_ON/AUDIT_LOCKED definitions into
include/linux/audit.h and convert the "audit_enabled == 0" to
"audit_enabled == AUDIT_OFF".

Post by Richard Guy Briggs
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index e30aa6b..1e48051 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -92,7 +92,7 @@ static void tty_audit_buf_push(struct tty_audit_buf *buf)
{
if (buf->valid == 0)
return;
- if (audit_enabled == 0) {
+ if (!audit_enabled) {
buf->valid = 0;
return;
}
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 7f2e31a..380542b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -734,7 +734,7 @@ static inline struct audit_buffer *xfrm_audit_start(const char *op)
{
struct audit_buffer *audit_buf = NULL;
- if (audit_enabled == 0)
+ if (!audit_enabled)
return NULL;
audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
AUDIT_MAC_IPSEC_EVENT);
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..3a18e59 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -424,7 +424,7 @@ static int audit_do_config_change(char *function_name, u32 *to_change, u32 new)
else
allow_changes = 1;
- if (audit_enabled != AUDIT_OFF) {
+ if (audit_enabled) {
rc = audit_log_config_change(function_name, new, old, allow_changes);
if (rc)
allow_changes = 0;
@@ -1097,7 +1097,7 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
{
struct audit_buffer *ab;
- if (audit_enabled == AUDIT_OFF)
+ if (!audit_enabled)
return;
ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FEATURE_CHANGE);
if (!ab)
@@ -1270,7 +1270,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
err = auditd_set(req_pid,
NETLINK_CB(skb).portid,
sock_net(NETLINK_CB(skb).sk));
- if (audit_enabled != AUDIT_OFF)
+ if (audit_enabled)
audit_log_config_change("audit_pid",
new_pid,
auditd_pid,
@@ -1281,7 +1281,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
/* try to process any backlog */
wake_up_interruptible(&kauditd_wait);
} else {
- if (audit_enabled != AUDIT_OFF)
+ if (audit_enabled)
audit_log_config_change("audit_pid",
new_pid,
auditd_pid, 1);
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index f368ee6..3921553 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -72,7 +72,7 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
struct audit_buffer *ab;
int fam = -1;
- if (audit_enabled == 0)
+ if (!audit_enabled)
goto errout;
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
index 2f328af..e68fa9d 100644
--- a/net/netlabel/netlabel_user.c
+++ b/net/netlabel/netlabel_user.c
@@ -101,7 +101,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
char *secctx;
u32 secctx_len;
- if (audit_enabled == 0)
+ if (!audit_enabled)
return NULL;
audit_buf = audit_log_start(audit_context(), GFP_ATOMIC, type);
--
1.8.3.1

--
paul moore
www.paul-moore.com

Richard Guy Briggs

2018-05-31 16:38:11 UTC

Would you prefer a patch to change the majority (18) of uses of
audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or
"audit_enabled != AUDIT_OFF")?

I prefer the approach in this patch because it makes the code smaller
and significantly easier to read, but either way, I'd like all uses to
be consistent so that it is easier to read all the code similarly.

Post by Paul Moore
If you are bothered by the comparison to 0 (magic numbers), you could
move the AUDIT_OFF/AUDIT_ON/AUDIT_LOCKED definitions into
include/linux/audit.h and convert the "audit_enabled == 0" to
"audit_enabled == AUDIT_OFF".

I'd be fine doing that if you really dislike this patch's approach.

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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

Paul Moore

2018-06-01 22:15:49 UTC

Would you prefer a patch to change the majority (18) of uses of
audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or
"audit_enabled != AUDIT_OFF")?
I prefer the approach in this patch because it makes the code smaller
and significantly easier to read, but either way, I'd like all uses to
be consistent so that it is easier to read all the code similarly.

I'd be fine doing that if you really dislike this patch's approach.

Like I said, I'm don't really care for the boolean-like approach of
this first patch.

--
paul moore
www.paul-moore.com

Richard Guy Briggs

2018-06-02 17:53:58 UTC

Would you prefer a patch to change the majority (18) of uses of
audit_enabled to be of the form "audit_enabled == AUDIT_OFF" (or
"audit_enabled != AUDIT_OFF")?
I prefer the approach in this patch because it makes the code smaller
and significantly easier to read, but either way, I'd like all uses to
be consistent so that it is easier to read all the code similarly.

I'd be fine doing that if you really dislike this patch's approach.

Like I said, I'm don't really care for the boolean-like approach of
this first patch.

That doesn't really address the original issue though.

Without any elaboration, I am not able to guess why you don't like this
or what possible future subtleties would cause a problem. Is there a
past example somewhere else that brings up this concern?
Can you explain the problem with "non-audit folks will mistake
audit_enabled as a true/false value when it is not"? Other subsystems
should not care about the distinction between locked and not.

While I realize people change their opinions given a broader context,
and the origninal reply was ambiguous, I went ahead with this patch
based on your "Sounds good." from:
https://www.redhat.com/archives/linux-audit/2018-April/msg00089.html

Would you accept a patch that defines a function by the same name as the
global variable that returns a boolean (and localizes and renames the
existing global with a "__" prefix? I'm not willing to offer a patch to
make the existing boolean usage harder to read to bring it all into
similar usage.

Post by Paul Moore
paul moore

- RGB

--
Richard Guy Briggs <***@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

Paul Moore

2018-06-04 23:57:17 UTC