All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
@ 2018-05-31 15:13 Richard Guy Briggs
  2018-05-31 15:48 ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Guy Briggs @ 2018-05-31 15:13 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML
  Cc: Eric Paris, Paul Moore, Steve Grubb, 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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 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(-)

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
  2018-05-31 15:13 [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient Richard Guy Briggs
@ 2018-05-31 15:48 ` Paul Moore
  2018-05-31 16:38   ` Richard Guy Briggs
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2018-05-31 15:48 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, Eric Paris, Steve Grubb

On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> 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
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  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".

> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
  2018-05-31 15:48 ` Paul Moore
@ 2018-05-31 16:38   ` Richard Guy Briggs
  2018-06-01 22:15     ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Guy Briggs @ 2018-05-31 16:38 UTC (permalink / raw)
  To: Paul Moore; +Cc: Linux-Audit Mailing List, LKML, Eric Paris, Steve Grubb

On 2018-05-31 11:48, Paul Moore wrote:
> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > 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
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  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.

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.

> 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.

> > 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

- 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
  2018-05-31 16:38   ` Richard Guy Briggs
@ 2018-06-01 22:15     ` Paul Moore
  2018-06-02 17:53       ` Richard Guy Briggs
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Moore @ 2018-06-01 22:15 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, Eric Paris, Steve Grubb

On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-05-31 11:48, Paul Moore wrote:
>> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > 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
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> >  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.
>
> 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.
>
>> 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.

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

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
  2018-06-01 22:15     ` Paul Moore
@ 2018-06-02 17:53       ` Richard Guy Briggs
  2018-06-04 23:57         ` Paul Moore
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Guy Briggs @ 2018-06-02 17:53 UTC (permalink / raw)
  To: Paul Moore; +Cc: Linux-Audit Mailing List, LKML, Eric Paris, Steve Grubb

On 2018-06-01 18:15, Paul Moore wrote:
> On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-05-31 11:48, Paul Moore wrote:
> >> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > 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
> >> >
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> >  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.
> >
> > 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.
> >
> >> 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.
> 
> 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.

> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient
  2018-06-02 17:53       ` Richard Guy Briggs
@ 2018-06-04 23:57         ` Paul Moore
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Moore @ 2018-06-04 23:57 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, Eric Paris, Steve Grubb

On Sat, Jun 2, 2018 at 1:53 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-06-01 18:15, Paul Moore wrote:
>> On Thu, May 31, 2018 at 12:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2018-05-31 11:48, Paul Moore wrote:
>> >> On Thu, May 31, 2018 at 11:13 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> >> > 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
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> >> > ---
>> >> >  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.
>> >
>> > 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.
>> >
>> >> 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.
>>
>> 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.

To be honest, there really isn't an issue to begin with, at least not
in my mind.  Sure, I understand you think all non-audit users of
audit_enabled should treat audit_enabled as a boolean; at this point
in time, I don't think that is necessary or desirable.

> Without any elaboration, I am not able to guess why you don't like this
> or what possible future subtleties would cause a problem.

As I said previously: "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.".

> Can you explain the problem with "non-audit folks will mistake
> audit_enabled as a true/false value when it is not"?

See the "it might work but ..." part above.

> 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

I think the confusion comes from what was meant by "clean them all
up".  We obviously have different understandings of what "cleaning"
meant.

> 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?

At this point I think I've been clear that I don't like treating it as
a boolean, regardless of if it is wrapped in a function or not.  Why?
Well, it's not a boolean for starters.

If you wanted to submit a patch that would swap out 0 for AUDIT_OFF I
would accept that.

> I'm not willing to offer a patch to make the existing boolean usage harder to read to bring it all into similar usage.

Okay ... ?  Patch submission has always been voluntary as far as I can
tell; if you aren't willing to submit a patch, that's fine.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-04 23:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 15:13 [RFC PATCH ghak86 V1] audit: use audit_enabled as a boolean where convenient Richard Guy Briggs
2018-05-31 15:48 ` Paul Moore
2018-05-31 16:38   ` Richard Guy Briggs
2018-06-01 22:15     ` Paul Moore
2018-06-02 17:53       ` Richard Guy Briggs
2018-06-04 23:57         ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.