audit: report audit wait metric in audit status reply

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is detected.

However, detection can be diffult using the currently available metrics. For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit
userspace: https://github.com/linux-audit/audit-userspace/pull/134.

Signed-off-by Max Englander <[email protected]>

---
Patch changelogs between v1 and v2:
- Instead of printing a warning when backlog waiting occurs, add duration of backlog waiting to cumulative sum, and report this sum in audit status reply.

 include/uapi/linux/audit.h | 7 ++++++-
 kernel/audit.c             | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

include/uapi/linux/audit.h +3 -1

 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
                                  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \

@@ -355,12 +357,14 @@ enum {
                                  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
                                  AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
                                  AUDIT_FEATURE_BITMAP_LOST_RESET | \
-                                 AUDIT_FEATURE_BITMAP_FILTER_FS)
+                                 AUDIT_FEATURE_BITMAP_FILTER_FS | \
+                                 AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
 #define AUDIT_VERSION_BACKLOG_LIMIT    AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
 #define AUDIT_VERSION_BACKLOG_WAIT_TIME        AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
+#define AUDIT_VERSION_BACKLOG_WAIT_SUM AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM
 
                                /* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT      0

kernel/audit.c +1 -0

 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 

@@ -1204,6 +1209,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                s.backlog               = skb_queue_len(&audit_queue);
                s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
                s.backlog_wait_time     = audit_backlog_wait_time;
+               s.backlog_wait_sum      = atomic_read(&audit_backlog_wait_sum);
                audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
                break;
        }

kernel/audit.c +0 -1

        }
 
        ab = audit_buffer_alloc(ctx, gfp_mask, type);
--

2.17.1

Quoted 30 lines · Max Englander

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is detected.

However, detection can be diffult using the currently available metrics. For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit
userspace: https://github.com/linux-audit/audit-userspace/pull/134.

Signed-off-by Max Englander <[email protected]>

---
Patch changelogs between v1 and v2:
- Instead of printing a warning when backlog waiting occurs, add duration of backlog waiting to cumulative sum, and report this sum in audit status reply.

Quoted 3 lines

 include/uapi/linux/audit.h | 7 ++++++-
 kernel/audit.c             | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Hi Max,

In general this looks better than the previous approach, but I do have a few specific comments (inline). It also important that in addition to the requisite userspace patch, we also need a test added to the audit-testsuite project so we can verify this functionality in the future.

* https://github.com/linux-audit/audit-testsuite

include/uapi/linux/audit.h enum { · line 343

Quoted 9 lines

include/uapi/linux/audit.h +1 -0

index a534d71e689a..ea0cc364beca 100644
@@ -340,6 +340,7 @@ enum {
 #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
 #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
 #define AUDIT_STATUS_LOST              0x0040
+#define AUDIT_STATUS_BACKLOG_WAIT_SUM  0x0080

Sooo ... you've defined this, but I don't see any of the corresponding AUDIT_SET code that I would expect, was that an oversight? If not, it is something we should support in the kernel as I'm sure admins will want to reset this value at some point.

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

include/uapi/linux/audit.h +1 -0

 #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
 #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask ;)

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

Quoted 16 lines

include/uapi/linux/audit.h +3 -1

 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
                                  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \

@@ -355,12 +357,14 @@ enum {
                                  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
                                  AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
                                  AUDIT_FEATURE_BITMAP_LOST_RESET | \
-                                 AUDIT_FEATURE_BITMAP_FILTER_FS)
+                                 AUDIT_FEATURE_BITMAP_FILTER_FS | \
+                                 AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
 #define AUDIT_VERSION_BACKLOG_LIMIT    AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
 #define AUDIT_VERSION_BACKLOG_WAIT_TIME        AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
+#define AUDIT_VERSION_BACKLOG_WAIT_SUM AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM
 

include/uapi/linux/audit.h struct audit_status { · line 473

Quoted 7 lines

include/uapi/linux/audit.h +1 -0

                                /* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT      0

@@ -466,6 +470,7 @@ struct audit_status {
                __u32   feature_bitmap; /* bitmap of kernel audit features */
        };
        __u32           backlog_wait_time;/* message queue wait timeout */
+       __u32           backlog_wait_sum;/* time spent waiting while message limit exceeded */

This is very nitpicky, but how about a rename to 'backlog_wait_time_actual'?

kernel/audit.c u32 audit_sig_sid = 0; · line 142

Quoted 12 lines

kernel/audit.c +4 -0

index 87f31bf1f0a0..301ea4f3d750 100644
@@ -136,6 +136,11 @@ u32                audit_sig_sid = 0;
 */
 static atomic_t        audit_lost = ATOMIC_INIT(0);
 
+/* Monotonically increasing sum of time the kernel has spent
+ * waiting while the backlog limit is exceeded.
+ */
+static atomic_t audit_backlog_wait_sum = ATOMIC_INIT(0);

Needless to say, this should be renamed too so we don't go crazy.

Quoted 12 lines

kernel/audit.c +1 -0

 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 

@@ -1204,6 +1209,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                s.backlog               = skb_queue_len(&audit_queue);
                s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
                s.backlog_wait_time     = audit_backlog_wait_time;
+               s.backlog_wait_sum      = atomic_read(&audit_backlog_wait_sum);
                audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
                break;
        }

kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1805

Quoted 6 lines

kernel/audit.c +3 -0

@@ -1794,6 +1800,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
                                return NULL;
                        }
                }
+
+               if (stime != audit_backlog_wait_time)
+                       atomic_add(audit_backlog_wait_time - stime, &audit_backlog_wait_sum);

Since stime can only be different in one place in the code above (after the schedule_timeout() call), why not move the atomic_add() up there and drop the "if"? Yes there is the potential of calling atomic_add() multiple times in this case, but the thread is waiting anyway and this way we don't impact other code paths.

kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1808

Quoted 3 lines

kernel/audit.c +0 -0

        }
 
        ab = audit_buffer_alloc(ctx, gfp_mask, type);

paul moore

Quoted 45 lines · Paul Moore

Quoted 30 lines · Max Englander

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is detected.

However, detection can be diffult using the currently available metrics. For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit
userspace: https://github.com/linux-audit/audit-userspace/pull/134.

Signed-off-by Max Englander <[email protected]>

---
Patch changelogs between v1 and v2:
- Instead of printing a warning when backlog waiting occurs, add duration of backlog waiting to cumulative sum, and report this sum in audit status reply.

Quoted 3 lines

 include/uapi/linux/audit.h | 7 ++++++-
 kernel/audit.c             | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Hi Max,

In general this looks better than the previous approach, but I do have a few specific comments (inline). It also important that in addition to the requisite userspace patch, we also need a test added to the audit-testsuite project so we can verify this functionality in the future.

* https://github.com/linux-audit/audit-testsuite

Quoted 14 lines

include/uapi/linux/audit.h enum { · line 343

Quoted 9 lines

include/uapi/linux/audit.h +1 -0

index a534d71e689a..ea0cc364beca 100644
@@ -340,6 +340,7 @@ enum {
 #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010
 #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
 #define AUDIT_STATUS_LOST              0x0040
+#define AUDIT_STATUS_BACKLOG_WAIT_SUM  0x0080

Sooo ... you've defined this, but I don't see any of the corresponding AUDIT_SET code that I would expect, was that an oversight? If not, it is something we should support in the kernel as I'm sure admins will want to reset this value at some point.

Have a look at the lost reset code as an example. It is tricky since it does an atomic reset while delivering a value back up the control plane and issuing a record. There were some fallout bug fixes because it wasn't as obvious as it looked.

Quoted 16 lines

include/uapi/linux/audit.h enum { · line 352

Quoted 8 lines

include/uapi/linux/audit.h +1 -0

 #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
 #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask ;)

It could key solely off the existance of AUDIT_STATUS_BACKLOG_WAIT_SUM via HAVE_DECL_AUDIT_STATUS_BACKLOG_WAIT_SUM which would be defined in configure.ac similar to AUDIT_STATUS_BACKLOG_WAIT_TIME as has already been done in Max' userspace patch.

It should be possible to drop AUDIT_VERSION_BACKLOG_WAIT_SUM in userspace and have it work.

Quoted 3 lines

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

I believe it won't even notice, but this should be tested.

Quoted 16 lines

Quoted 16 lines

include/uapi/linux/audit.h +3 -1

 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
                                  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \

@@ -355,12 +357,14 @@ enum {
                                  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
                                  AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
                                  AUDIT_FEATURE_BITMAP_LOST_RESET | \
-                                 AUDIT_FEATURE_BITMAP_FILTER_FS)
+                                 AUDIT_FEATURE_BITMAP_FILTER_FS | \
+                                 AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
 #define AUDIT_VERSION_BACKLOG_LIMIT    AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
 #define AUDIT_VERSION_BACKLOG_WAIT_TIME        AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
+#define AUDIT_VERSION_BACKLOG_WAIT_SUM AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM
 

Quoted 9 lines

include/uapi/linux/audit.h struct audit_status { · line 473

Quoted 7 lines

include/uapi/linux/audit.h +1 -0

                                /* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT      0

@@ -466,6 +470,7 @@ struct audit_status {
                __u32   feature_bitmap; /* bitmap of kernel audit features */
        };
        __u32           backlog_wait_time;/* message queue wait timeout */
+       __u32           backlog_wait_sum;/* time spent waiting while message limit exceeded */

This is very nitpicky, but how about a rename to 'backlog_wait_time_actual'?

How about 'backlog_wait_time_cumul' (or 'backlog_wait_time_cumulative')?

Quoted 41 lines

kernel/audit.c u32 audit_sig_sid = 0; · line 142

Quoted 13 lines

kernel/audit.c +4 -0

index 87f31bf1f0a0..301ea4f3d750 100644
@@ -136,6 +136,11 @@ u32                audit_sig_sid = 0;
 */
 static atomic_t        audit_lost = ATOMIC_INIT(0);
 
+/* Monotonically increasing sum of time the kernel has spent
+ * waiting while the backlog limit is exceeded.
+ */
+static atomic_t audit_backlog_wait_sum = ATOMIC_INIT(0);

Needless to say, this should be renamed too so we don't go crazy.

Quoted 12 lines

kernel/audit.c +1 -0

 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 

@@ -1204,6 +1209,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                s.backlog               = skb_queue_len(&audit_queue);
                s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
                s.backlog_wait_time     = audit_backlog_wait_time;
+               s.backlog_wait_sum      = atomic_read(&audit_backlog_wait_sum);
                audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
                break;
        }

kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1805

Quoted 6 lines

kernel/audit.c +3 -0

@@ -1794,6 +1800,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
                                return NULL;
                        }
                }
+
+               if (stime != audit_backlog_wait_time)
+                       atomic_add(audit_backlog_wait_time - stime, &audit_backlog_wait_sum);

Since stime can only be different in one place in the code above (after the schedule_timeout() call), why not move the atomic_add() up there and drop the "if"? Yes there is the potential of calling atomic_add() multiple times in this case, but the thread is waiting anyway and this way we don't impact other code paths.

Quoted 5 lines

kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1808

Quoted 3 lines

kernel/audit.c +0 -0

        }
 
        ab = audit_buffer_alloc(ctx, gfp_mask, type);

paul moore

- RGB

--
Richard Guy Briggs <[email protected]>
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

Quoted 30 lines · Max Englander

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is detected.

However, detection can be diffult using the currently available metrics. For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit
userspace: https://github.com/linux-audit/audit-userspace/pull/134.

Signed-off-by Max Englander <[email protected]>

---
Patch changelogs between v1 and v2:
- Instead of printing a warning when backlog waiting occurs, add duration of backlog waiting to cumulative sum, and report this sum in audit status reply.

Quoted 3 lines

 include/uapi/linux/audit.h | 7 ++++++-
 kernel/audit.c             | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Hi Max,

In general this looks better than the previous approach, but I do have a few specific comments (inline). It also important that in addition

to the requisite userspace patch, we also need a test added to the audit-testsuite project so we can verify this functionality in the future.

* https://github.com/linux-audit/audit-testsuite

Quoted 6 lines

include/uapi/linux/audit.h +0 -0

index a534d71e689a..ea0cc364beca 100644
@@ -340,6 +340,7 @@ enum {
 #define AUDIT_STATUS_BACKLOG_LIMIT     0x0010

include/uapi/linux/audit.h enum { · line 343

Quoted 3 lines

include/uapi/linux/audit.h +1 -0

 #define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
 #define AUDIT_STATUS_LOST              0x0040
+#define AUDIT_STATUS_BACKLOG_WAIT_SUM  0x0080

Sooo ... you've defined this, but I don't see any of the corresponding AUDIT_SET code that I would expect, was that an oversight? If not, it is something we should support in the kernel as I'm sure admins will want to reset this value at some point.

include/uapi/linux/audit.h enum { · line 352

Quoted 8 lines

include/uapi/linux/audit.h +1 -0

 #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
 #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask ;)

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

Quoted 16 lines

include/uapi/linux/audit.h +3 -1

 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
                                  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \

@@ -355,12 +357,14 @@ enum {
                                  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
                                  AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
                                  AUDIT_FEATURE_BITMAP_LOST_RESET | \
-                                 AUDIT_FEATURE_BITMAP_FILTER_FS)
+                                 AUDIT_FEATURE_BITMAP_FILTER_FS | \
+                                 AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST           AUDIT_FEATURE_BITMAP_ALL
 #define AUDIT_VERSION_BACKLOG_LIMIT    AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
 #define AUDIT_VERSION_BACKLOG_WAIT_TIME        AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
+#define AUDIT_VERSION_BACKLOG_WAIT_SUM AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM
 

include/uapi/linux/audit.h struct audit_status { · line 473

Quoted 7 lines

include/uapi/linux/audit.h +1 -0

                                /* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT      0

@@ -466,6 +470,7 @@ struct audit_status {
                __u32   feature_bitmap; /* bitmap of kernel audit features */
        };
        __u32           backlog_wait_time;/* message queue wait timeout */
+       __u32           backlog_wait_sum;/* time spent waiting while message limit exceeded */

This is very nitpicky, but how about a rename to 'backlog_wait_time_actual'?

Quoted 9 lines

kernel/audit.c +0 -0

index 87f31bf1f0a0..301ea4f3d750 100644
@@ -136,6 +136,11 @@ u32                audit_sig_sid = 0;
 */
 static atomic_t        audit_lost = ATOMIC_INIT(0);
 

kernel/audit.c u32 audit_sig_sid = 0; · line 142

Quoted 4 lines

kernel/audit.c +4 -0

+/* Monotonically increasing sum of time the kernel has spent
+ * waiting while the backlog limit is exceeded.
+ */
+static atomic_t audit_backlog_wait_sum = ATOMIC_INIT(0);

Needless to say, this should be renamed too so we don't go crazy.

Quoted 17 lines

kernel/audit.c +2 -0

 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 

@@ -1204,6 +1209,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                s.backlog               = skb_queue_len(&audit_queue);
                s.feature_bitmap        = AUDIT_FEATURE_BITMAP_ALL;
                s.backlog_wait_time     = audit_backlog_wait_time;
+               s.backlog_wait_sum      = atomic_read(&audit_backlog_wait_sum);
                audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
                break;
        }

@@ -1794,6 +1800,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
                                return NULL;
                        }
                }
+

kernel/audit.c struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, · line 1805

Quoted 2 lines

kernel/audit.c +2 -0

+               if (stime != audit_backlog_wait_time)
+                       atomic_add(audit_backlog_wait_time - stime, &audit_backlog_wait_sum);

Since stime can only be different in one place in the code above (after the schedule_timeout() call), why not move the atomic_add() up there and drop the "if"? Yes there is the potential of calling atomic_add() multiple times in this case, but the thread is waiting anyway and this way we don't impact other code paths.

Quoted 6 lines

kernel/audit.c +0 -1

        }
 
        ab = audit_buffer_alloc(ctx, gfp_mask, type);
--

2.17.1

Signature · 2 lines

paul moore
www.paul-moore.com

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

include/uapi/linux/audit.h +1 -0

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

Hello Paul,

Quoted 16 lines

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

include/uapi/linux/audit.h +1 -0

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask

Quoted 2 lines

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

Quoted 1 line · Steve Grubb

Hello Paul,

Steve.

Quoted 20 lines · EST Paul Moore

Quoted 18 lines

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

include/uapi/linux/audit.h +1 -0

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

Quoted 4 lines

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

I don't believe you need a new bitmap entry in this case, you should be able to examine the size of the reply from the AUDIT_GET request and make a determination from there.

paul moore

Quoted 9 lines · Paul Moore

Quoted 4 lines · Steve Grubb

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

I don't believe you need a new bitmap entry in this case, you should be able to examine the size of the reply from the AUDIT_GET request and make a determination from there.

The danger I see is if another feature is added to the audit status and that is backported to a distro rather than this one. It would be impossible to determine which feature it was from the size alone. Keying off specific fields in the kernel should be able to do this at build time if I understood correctly.

For the upstream kernel, this may be the case. But in the world where people backport patches, how do I know that the size is related to this patch and no other?

Quoted 16 lines · Richard Guy Briggs

Quoted 9 lines · Paul Moore

Quoted 4 lines · Steve Grubb

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

I don't believe you need a new bitmap entry in this case, you should be able to examine the size of the reply from the AUDIT_GET request and make a determination from there.

The danger I see is if another feature is added to the audit status and that is backported to a distro rather than this one. It would be impossible to determine which feature it was from the size alone. Keying off specific fields in the kernel should be able to do this at build time if I understood correctly.

...

Quoted 3 lines · Steve Grubb

For the upstream kernel, this may be the case. But in the world where people backport patches, how do I know that the size is related to this patch and no other?

We've discussed this in the past, and most of you should already know my answer to this, but I'll repeat my stance on this again.

My first priority is the upstream kernel, enterprise distribution kernels are secondary. The upstream kernels do not generally backport features, backports are limited to fixes. If an individual or a distribution decides to backport kernel features they are responsible for making things work; it is not upstream's responsibility to enable, or support, arbitrary combinations of patches. Any assistance we provide here is "best effort" and not guaranteed.

Bringing this back to the case at hand, the feature bitmap is a limited resource and it is my opinion that we need to limit its use to only those features which can not be determined through other means;

in this case this feature can be determine by the size of the AUDIT_GET reply buffer (the audit_status struct). Of course if more care and thought had been put into the audit kernel/userspace API in the first place we would not be in this situation, but we are and we need to deal with it as best we can.

paul moore

Quoted 36 lines · Paul Moore

Quoted 16 lines · Richard Guy Briggs

Quoted 9 lines · Paul Moore

Quoted 4 lines · Steve Grubb

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

I don't believe you need a new bitmap entry in this case, you should be able to examine the size of the reply from the AUDIT_GET request and make a determination from there.

The danger I see is if another feature is added to the audit status and that is backported to a distro rather than this one. It would be impossible to determine which feature it was from the size alone. Keying off specific fields in the kernel should be able to do this at build time if I understood correctly.

...

Quoted 3 lines · Steve Grubb

For the upstream kernel, this may be the case. But in the world where people backport patches, how do I know that the size is related to this patch and no other?

We've discussed this in the past, and most of you should already know my answer to this, but I'll repeat my stance on this again.

My first priority is the upstream kernel, enterprise distribution kernels are secondary. The upstream kernels do not generally backport features, backports are limited to fixes. If an individual or a distribution decides to backport kernel features they are responsible for making things work; it is not upstream's responsibility to enable, or support, arbitrary combinations of patches. Any assistance we provide here is "best effort" and not guaranteed.

Quoted 3 lines

Bringing this back to the case at hand, the feature bitmap is a limited resource and it is my opinion that we need to limit its use to only those features which can not be determined through other means;

So far there are only seven bits used out of 32, so it does not appear we are in danger of running out anytime soon.

It was introduced with commit 0288d7183c41c0192d2963d44590f346f4aee917
Author: Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit: Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500
("audit: convert status version to a feature bitmap")
It was introduced specifically to enable distributions to selectively backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of backlog_wait_time_actual as I mentioned above.

Quoted 2 lines

So far there are only seven bits used out of 32, so it does not appear we are in danger of running out anytime soon.

Quoted 9 lines

It was introduced with commit 0288d7183c41c0192d2963d44590f346f4aee917

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap")
It was introduced specifically to enable distributions to selectively backport features. It was converted away from AUDIT_VERSION.

Quoted 2 lines

There are other ways to detect the presence of backlog_wait_time_actual as I mentioned above.

Let me be blunt - I honestly don't care what Steve's audit userspace does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

Quoted 2 lines · EST Paul Moore

Quoted 2 lines

So far there are only seven bits used out of 32, so it does not appear we are in danger of running out anytime soon.

Exactly. Even capability bits are easier to get assigned. :-)

Quoted 12 lines

Quoted 11 lines

It was introduced with commit 0288d7183c41c0192d2963d44590f346f4aee917

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap")
It was introduced specifically to enable distributions to selectively backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of backlog_wait_time_actual as I mentioned above.

Quoted 5 lines

Let me be blunt - I honestly don't care what Steve's audit userspace does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

And guessing what might be in buffers of different sizes is good design? The FEATURE_BITMAP was introduced to get rid of this ambiguity.

Quoted 18 lines

Quoted 17 lines

Quoted 11 lines

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap")
It was introduced specifically to enable distributions to selectively backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of
backlog_wait_time_actual
as I mentioned above.

Let me be blunt - I honestly don't care what Steve's audit userspace does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

Quoted 2 lines

And guessing what might be in buffers of different sizes is good design? The FEATURE_BITMAP was introduced to get rid of this ambiguity.

There is just soo much to unpack in your comment Steve, but let me keep it short ...

- This is an enterprise distro problem, not an upstream problem. The problems you are talking about are not a problem for upstream.

Quoted 24 lines · EST Paul Moore

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap")
It was introduced specifically to enable distributions to selectively backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of
backlog_wait_time_actual
as I mentioned above.

Quoted 8 lines

Quoted 5 lines

Let me be blunt - I honestly don't care what Steve's audit userspace does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

And guessing what might be in buffers of different sizes is good design? The FEATURE_BITMAP was introduced to get rid of this ambiguity.

There is just soo much to unpack in your comment Steve, but let me keep it short ...

Quoted 2 lines

- This is an enterprise distro problem, not an upstream problem. The problems you are talking about are not a problem for upstream.

You may look at it that way. I do not. Audit -userspace is also an upstream for a lot of distros and I need to make this painless for them. So, while you may think of this being a backport problem for Red Hat to solve, I think of this as a generic problem that I'd like to solve for Debian, Suse, Ubuntu, Arch, Gentoo, anyone using audit. We both are upstream.

Quoted 7 lines · Steve Grubb

Quoted 6 lines · EST Paul Moore

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap") It was introduced specifically to enable distributions to

selectively

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of
backlog_wait_time_actual
as I mentioned above.

Let me be blunt - I honestly don't care what Steve's audit

userspace

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

And guessing what might be in buffers of different sizes is good

design?

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

The FEATURE_BITMAP was introduced to get rid of this ambiguity.

There is just soo much to unpack in your comment Steve, but let me keep it short ...

- This is an enterprise distro problem, not an upstream problem. The problems you are talking about are not a problem for upstream.

You may look at it that way. I do not. Audit -userspace is also an

upstream

Quoted 1 line

for a lot of distros and I need to make this painless for them. So,

while you

Quoted 1 line

may think of this being a backport problem for Red Hat to solve, I think

of

Quoted 1 line

this as a generic problem that I'd like to solve for Debian, Suse,

Ubuntu,

Quoted 1 line

Arch, Gentoo, anyone using audit. We both are upstream.

I intentionally said "enterprise Linux distributions", I never singled out RH/IBM. Contrary to what RH/IBM marketing may have me believe, I don't consider RHEL to be the only "enterprise Linux distribution" :)

Beyond that, while I haven't looked at all of the distros you list above, I know a few of them typically only backport fixes, not new features. Further, as I mentioned previously in this thread, there is a way to backport this feature in a safe manner without using the feature bits. Eeeeeven further, if there wasn't a way to backport this feature safely (and let me stress agai that you can backport this safely), I would still consider that to be a distro problem and not an upstream kernel problem. The upstream kernel is not responsible for enabling or supporting arbitrary combinations of patches.

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

It sounds like there's a decision to be made around whether or not to use the bitmap feature flags which I probably am probably not in a position to help decide. However, I'm more than happy to fix my userspace PR so that it does not rely on the feature flag space using the approach Paul outlined, in spite of the drawbacks, if that ends up being the decision.

Separately, since there is tension between these two approaches (structure size and bitmap), I wonder if Paul/Steve you would be open to a third way.

For example, I can imagine adding additional bitmap spaces (FEATURE_BITMAP_2, FEATURE_BITMAP_3, etc.). Alternately, I can imagine each feature being assigned a unique u64 ID, and user space programs querying the kernel to see whether a a particular feature is enabled.

Quoted 56 lines · Paul Moore

Quoted 7 lines · Steve Grubb

Quoted 6 lines · EST Paul Moore

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap") It was introduced specifically to enable distributions to

selectively

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

backport features. It was converted away from AUDIT_VERSION.

There are other ways to detect the presence of
backlog_wait_time_actual
as I mentioned above.

Let me be blunt - I honestly don't care what Steve's audit

userspace

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

does to detect this. I've got my own opinion, but Steve's audit userspace is not my project to manage and I think we've established over the years that Steve and I have very different views on what constitutes good design.

And guessing what might be in buffers of different sizes is good

design?

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

The FEATURE_BITMAP was introduced to get rid of this ambiguity.

There is just soo much to unpack in your comment Steve, but let me keep it short ...

- This is an enterprise distro problem, not an upstream problem. The problems you are talking about are not a problem for upstream.

You may look at it that way. I do not. Audit -userspace is also an

upstream

Quoted 1 line

for a lot of distros and I need to make this painless for them. So,

while you

Quoted 1 line

may think of this being a backport problem for Red Hat to solve, I think

of

Quoted 1 line

this as a generic problem that I'd like to solve for Debian, Suse,

Ubuntu,

Quoted 1 line

Arch, Gentoo, anyone using audit. We both are upstream.

I intentionally said "enterprise Linux distributions", I never singled out RH/IBM. Contrary to what RH/IBM marketing may have me believe, I don't consider RHEL to be the only "enterprise Linux distribution" :)

Beyond that, while I haven't looked at all of the distros you list above, I know a few of them typically only backport fixes, not new features. Further, as I mentioned previously in this thread, there is a way to backport this feature in a safe manner without using the feature bits. Eeeeeven further, if there wasn't a way to backport this feature safely (and let me stress agai that you can backport this safely), I would still consider that to be a distro problem and not an upstream kernel problem. The upstream kernel is not responsible for enabling or supporting arbitrary combinations of patches.

Quoted 10 lines

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Hi Steve, Paul,

I'm replying with the Gmail UI since I don't have my Mutt setup handy, so apologies for any formatting which doesn't align with the mailing list best
practices!

First off, my apologies for being late to the thread, and for submitting code
to the kernel and user space which aren't playing nicely with each other.

It sounds like there's a decision to be made around whether or not to use the bitmap feature flags which I probably am probably not in a position to help decide. However, I'm more than happy to fix my userspace PR so that it does not rely on the feature flag space using the approach Paul outlined, in spite of the drawbacks, if that ends up being the decision.

Steve, I understand your preference to rely on the feature bitmap since it is a more reliable way to determine the availability of a feature than key size, but if you're open to Paul's recommendations in spite of the drawbacks, I'll make the changes to my patch as soon as I can to unblock your work.

Separately, since there is tension between these two approaches (structure size and bitmap), I wonder if Paul/Steve you would be open to a third way.

For example, I can imagine adding additional bitmap spaces (FEATURE_BITMAP_2, FEATURE_BITMAP_3, etc.). Alternately, I can imagine each feature being assigned a unique u64 ID, and user space programs querying the kernel to see whether a a particular feature is enabled.

I'm not familiar enough with the kernel to be able to judge how sound either idea is (or if these have been considered and rejected in the past) but if you all think a third way is viable, I'd be happy to start a separate mailing thread to try to thread the competing requirements of the kernel and userspace, and contribute code if we can find a solution.

Max

Quoted 13 lines · Max Englander

Quoted 11 lines · Paul Moore

Quoted 7 lines · Steve Grubb

Quoted 6 lines · EST Paul Moore

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Author:     Richard Guy Briggs <[email protected]>
AuthorDate: 2014-11-17 15:51:01 -0500
Commit:     Paul Moore <[email protected]>
CommitDate: 2014-11-17 16:53:51 -0500

("audit: convert status version to a feature bitmap") It was introduced specifically to enable distributions to

selectively

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

backport features. It was converted away from AUDIT_VERSION.

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

There are other ways to detect the presence of
backlog_wait_time_actual
as I mentioned above.

Let me be blunt - I honestly don't care what Steve's audit

userspace

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

does to detect this. I've got my own opinion, but Steve's

audit

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

userspace is not my project to manage and I think we've

established

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

over the years that Steve and I have very different views on

what

Quoted 10 lines

Quoted 10 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

constitutes good design.

And guessing what might be in buffers of different sizes is good

design?

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

The FEATURE_BITMAP was introduced to get rid of this ambiguity.

There is just soo much to unpack in your comment Steve, but let me keep it short ...

- This is an enterprise distro problem, not an upstream problem.

The

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

problems you are talking about are not a problem for upstream.

You may look at it that way. I do not. Audit -userspace is also an

upstream

Quoted 1 line

for a lot of distros and I need to make this painless for them. So,

while you

Quoted 1 line

may think of this being a backport problem for Red Hat to solve, I

think

Quoted 29 lines

Quoted 19 lines

of

Quoted 1 line

this as a generic problem that I'd like to solve for Debian, Suse,

Ubuntu,

Quoted 1 line

Arch, Gentoo, anyone using audit. We both are upstream.

I intentionally said "enterprise Linux distributions", I never singled out RH/IBM. Contrary to what RH/IBM marketing may have me believe, I don't consider RHEL to be the only "enterprise Linux distribution" :)

Beyond that, while I haven't looked at all of the distros you list above, I know a few of them typically only backport fixes, not new features. Further, as I mentioned previously in this thread, there is a way to backport this feature in a safe manner without using the feature bits. Eeeeeven further, if there wasn't a way to backport this feature safely (and let me stress agai that you can backport this safely), I would still consider that to be a distro problem and not an upstream kernel problem. The upstream kernel is not responsible for enabling or supporting arbitrary combinations of patches.

Quoted 10 lines

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 4 lines

Hi Steve, Paul,

I'm replying with the Gmail UI since I don't have my Mutt setup handy, so apologies for any formatting which doesn't align with the mailing list

best

Quoted 8 lines

practices!

First off, my apologies for being late to the thread, and for submitting code
to the kernel and user space which aren't playing nicely with each other.

It sounds like there's a decision to be made around whether or not to use the bitmap feature flags which I probably am probably not in a position

to

Quoted 5 lines

help decide. However, I'm more than happy to fix my userspace PR so that it does not rely on the feature flag space using the approach Paul outlined, in spite of the drawbacks, if that ends up being the decision.

Steve, I understand your preference to rely on the feature bitmap since

it

Quoted 8 lines

is a more reliable way to determine the availability of a feature than key size, but if you're open to Paul's recommendations in spite of the drawbacks, I'll make the changes to my patch as soon as I can to unblock your work.

Separately, since there is tension between these two approaches (structure size and bitmap), I wonder if Paul/Steve you would be open to a third way.

Max, Steve: have you looked at my reply in this thread from 2020-12-03 18:10?

There are two solutions in there that should work without relying on the unreliable test for structure size that work without changing the currently commited kernel code. Or have I missed something?

Quoted 8 lines

For example, I can imagine adding additional bitmap spaces (FEATURE_BITMAP_2, FEATURE_BITMAP_3, etc.). Alternately, I can imagine each feature being assigned a unique u64 ID, and user space programs querying the kernel to see whether a a particular feature is enabled.

I'm not familiar enough with the kernel to be able to judge how sound either idea is (or if these have been considered and rejected in the

past)

Quoted 1 line

but if you all think a third way is viable, I'd be happy to start a

separate

Quoted 4 lines

mailing thread to try to thread the competing requirements of the kernel and userspace, and contribute code if we can find a solution.

Max

- RGB

--
Richard Guy Briggs <[email protected]>
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

Steve, I'm happy to make changes to the userspace PR based on Richard's suggestions, if that sounds good to you. I'll follow up in the PR to discuss it more

Hello Max,

Quoted 3 lines · EST Max Englander

Steve, I'm happy to make changes to the userspace PR based on Richard's suggestions, if that sounds good to you. I'll follow up in the PR to discuss it more

The only issue is new userspace on old kernel. I think if we use both the configure macro in addition to a size check, then it will at least allow forward and backward compatibility.

Other metrics would be good. I'd like to see a max_backlog to know if we are wasting memory. It would just record the highwater mark since auditing was enabled.

-Steve

Quoted 7 lines · Steve Grubb

Hello Max,

Quoted 3 lines · EST Max Englander

Steve, I'm happy to make changes to the userspace PR based on Richard's suggestions, if that sounds good to you. I'll follow up in the PR to discuss it more

Quoted 3 lines

The only issue is new userspace on old kernel. I think if we use both the configure macro in addition to a size check, then it will at least allow forward and backward compatibility.

Are you talking about a new userspace compiled on a new kernel header file run on an old kernel?

That would be less reliable and need the
size check. The bitmap would be the most reliable in that scenario.

By configure macro are you talking about the presence of that audit status mask bit, or the presence of that struct audit_status member?

Quoted 12 lines · EST Richard Guy Briggs

Quoted 10 lines · Steve Grubb

Hello Max,

Quoted 3 lines · EST Max Englander

Steve, I'm happy to make changes to the userspace PR based on Richard's suggestions, if that sounds good to you. I'll follow up in the PR to discuss it more

The only issue is new userspace on old kernel. I think if we use both the configure macro in addition to a size check, then it will at least allow forward and backward compatibility.

Quoted 2 lines

Are you talking about a new userspace compiled on a new kernel header file run on an old kernel?

Yes. This is my worry. Someone compiles the code and the does a roll back. It can happen because the new kernel has some problems that a driver cannot handle.

Quoted 2 lines

That would be less reliable and need the
size check. The bitmap would be the most reliable in that scenario.

Right, but the person that can make that happen doesn't want to use this facility for what it was intended for. So, we are all trying to do the best.

Quoted 2 lines

By configure macro are you talking about the presence of that audit status mask bit, or the presence of that struct audit_status member?

Yes. But it doesn't apply to old kernels.

-Steve

Quoted 4 lines

Quoted 2 lines

By configure macro are you talking about the presence of that audit status mask bit, or the presence of that struct audit_status member?

Yes. But it doesn't apply to old kernels.

An "or" question usually needs one or the other reply unless both are true... Which one were you talking about?

On 2020-12-07 18:28, Steve Grubb wrote:

Quoted 3 lines

Other metrics would be good. I'd like to see a max_backlog to know if we are wasting memory. It would just record the highwater mark since auditing was enabled.

That would be covered with this issue:
https://github.com/linux-audit/audit-kernel/issues/63

Quoted 1 line · Steve Grubb

Hello Paul,

Steve.

Quoted 17 lines · EST Paul Moore

Quoted 16 lines

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

include/uapi/linux/audit.h +1 -0

@@ -348,6 +349,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it *should* key off the structure size and not a new flag in the bitmask

Quoted 8 lines

Quoted 2 lines

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

We need this FEATURE_BITMAP to do anything in userspace. Max's instinct was right. Anything that changes the user space API needs to have a FEATURE_BITMAP so that user space can do the right thing. The lack of this is blocking acceptance of the pull request for the user space piece.

I don't believe you need a new bitmap entry in this case, you should be able to examine the size of the reply from the AUDIT_GET request and make a determination from there.

include/uapi/linux/audit.h enum { · line 352

Quoted 7 lines

  #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     0x00000001
  #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002

include/uapi/linux/audit.h +1 -0

@@ -348,6 +349,7 @@ enum {
  #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x00000010
  #define AUDIT_FEATURE_BITMAP_LOST_RESET                0x00000020
  #define AUDIT_FEATURE_BITMAP_FILTER_FS         0x00000040
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_SUM  0x00000080

In an effort not to exhaust the feature bitmap too quickly, I've been restricting it to only those features that would cause breakage with userspace. I haven't looked closely at Steve's userspace in quite a while, but I'm guessing it can key off the structure size and doesn't need this entry in the bitmap, right? Let me rephrase, if userspace needs to key off anything, it*should* key off the structure size and not a new flag in the bitmask;)

Also, I'm assuming that older userspace doesn't blow-up if it sees the larger structure size? That's even more important.

Paul,

This change does seem to the untrained eye to be in line with the existing FEATURE_BITMAP definitions. I appreciate your intent on not exhausting the available space, but at some point if that happens is there any reasonable way to expand? I'm sure you have some thoughts, or is this "it" as far as features could go (the last available bits)?

  In environments where the preservation of audit events and predictable
  usage of system memory are prioritized, admins may use a combination of>  --backlog_wait_time and -b options at the risk of degraded performance>  resulting from backlog waiting. In some cases, this risk may be>  preferred to lost events or unbounded memory usage. Ideally, this risk>  can be mitigated by making adjustments when backlog waiting is detected.>  >  However, detection can be diffult using the currently available metrics.>  For example, an admin attempting to debug degraded performance may>  falsely believe a full backlog indicates backlog waiting. It may turn>  out the backlog frequently fills up but drains quickly.>  >  To make it easier to reliably track degraded performance to backlog>  waiting, this patch makes the following changes:>  >  Add a new field backlog_wait_sum to the audit status reply. Initialize>  this field to zero. Add to this field the total time spent by the>  current task on scheduled timeouts while the backlog limit is exceeded.>  >  Tested on Ubuntu 18.04 using complementary changes to the audit>  userspace: https://github.com/linux-audit/audit-userspace/pull/134.

I would like to suggest providing a mechanism where admins can query the status or state of backlog issues (wait time, sums, etc...). Maybe the intent is to expand the output of status checking of auditd.

I believe further clarity is beneficial on the setting of the 'backlog_wait_sum' (or to whatever the name evolves to) initially.

(quoted diff) +0 -3

-  How it evolves over time
-  What the conditions in the system, or auditing, would change it
-  What conditions admins should pay attention to for informational

understanding of status
- What conditions admins should realize exist such that adjustments are needed
(and suggestions to what those adjustments should be)
- What new guidance will admins have for building adjusting audit.rules around this

Consider the scenario where auditing has been 'working fine' for days. Little to no active admin monitoring.
Events occur to spike the auditing such that backloging of audit records dramatically increases.
(for some reason) admins now come looking to investigate. Assuming they do: 'systemctl status auditd' the newly presented 'state' of the 'backlog_wait_sum' will show some evidence.

Q:  Is that just a moment in time?
Q:  What information here will give the perspective things are good/ok

'now', versus some action needs to be taken?

Maybe that isn't a great scenario, or good questions----it is what occurs to me at the moment.

Thank you.

R,
-Joe Wulf

Quoted 6 lines · Max Englander

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is

detected.

Quoted 2 lines

However, detection can be diffult using the currently available

metrics.

Quoted 7 lines

For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Quoted 6 lines

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit
userspace: https://github.com/linux-audit/audit-userspace/pull/134.

<snip>

In environments where the preservation of audit events and predictable usage of system memory are prioritized, admins may use a combination of --backlog_wait_time and -b options at the risk of degraded performance resulting from backlog waiting. In some cases, this risk may be preferred to lost events or unbounded memory usage. Ideally, this risk can be mitigated by making adjustments when backlog waiting is detected.

However, detection can be diffult using the currently available metrics. For example, an admin attempting to debug degraded performance may falsely believe a full backlog indicates backlog waiting. It may turn out the backlog frequently fills up but drains quickly.

To make it easier to reliably track degraded performance to backlog waiting, this patch makes the following changes:

Add a new field backlog_wait_sum to the audit status reply. Initialize this field to zero. Add to this field the total time spent by the current task on scheduled timeouts while the backlog limit is exceeded.

Tested on Ubuntu 18.04 using complementary changes to the audit userspace:https://github.com/linux-audit/audit-userspace/pull/134.