Re: [RFC][PATCH] audit: add feature audit_lost reset

On 2016-12-06 19:17, Paul Moore wrote: > On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > On 2016-12-05 12:48, Paul Moore wrote: > >> On Mon, Dec 5, 2016 at 11:52 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >> > On 2016-12-05 11:02, Paul Moore wrote: > >> >> On Mon, Dec 5, 2016 at 3:02 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >> >> > Add a method to reset the audit_lost value. > >> >> > > >> >> > An AUDIT_GET message will get the current audit_lost value and reset the > >> >> > counter to zero iff (if and only if) the AUDIT_FEATURE_LOST_RESET > >> >> > feature is set. > >> >> > > >> >> > If the flag AUDIT_FEATURE_BITMAP_LOST_RESET is present in the audit > >> >> > feature bitmap, the feature is settable by setting the > >> >> > AUDIT_FEATURE_LOST_RESET flag in the audit feature list with an > >> >> > AUDIT_SET_FEATURE call. This setting is lockable. > >> >> > > >> >> > See: https://github.com/linux-audit/audit-kernel/issues/3 > >> >> > > >> >> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > >> >> > --- > >> >> > Note: The AUDIT_FEATURE_BITMAP_LOST_RESET check may not be necessary if > >> >> > it is possible to read all the entries from audit_feature_names from > >> >> > userspace. > >> >> > --- > >> >> > include/uapi/linux/audit.h | 7 +++++-- > >> >> > kernel/audit.c | 9 ++++++--- > >> >> > 2 files changed, 11 insertions(+), 5 deletions(-) > >> >> > >> >> Instead of resetting the lost counter on an AUDIT_GET if the reset > >> >> feature is set, how about preserving the AUDIT_GET behavior, skipping > >> >> the AUDIT_FEATURE_* addition, and simply reset the lost value by > >> >> sending a AUDIT_SET message with AUDIT_STATUS_LOST (you obviously have > >> >> to add this to the uapi header). > >> > > >> > I realized as I was coding it up that we would potentially lose an > >> > accurate count if the read and reset were not atomic. This was the > >> > reason for using atomic_xchg(). > >> > >> Well, okay, but that isn't what I was talking about ... ? The > >> audit_cmd_mutex is held for both AUDIT_GET and AUDIT_SET so we should > >> never process these requests at the same time. > > > > I guess I still don't follow what you are talking about... I hadn't > > thought of including both an AUDIT_GET and an AUDIT_SET in the same skb, > > but that would ensure that no other command reads or resets the lost > > value. However, the lost value could still be incrementing on another > > CPU between these two commands, losing an accurate lost count. > > Okay, back up ... this whole mess about atomic_xchg() was always > unrelated to my original suggestion, let's focus on my original > comment ... don't reset the counter on a AUDIT_GET, reset it on a > AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense? I understood that. It sounds like a nice simple and straightforward method to do it but for the question of accuracy. Please rewind to my fundamental point: How do we get an accurate reading of the last value of audit_lost before resetting it?