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

On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > 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? Okay, I thought you were worried about a different race, which is why this discussion wasn't making much sense to me. I understand your point, but I really dislike the API; although that's not your fault, it's really the only way to do it via AUDIT_GET. I'd much prefer we go with the cleaner AUDIT_SET approach and just not worry about the small race window. It would only be an issue if you reset the count under heavy audit load, and why would you reset the lost value if you were under a heavy audit load? That just doesn't make sense.

I suppose we should hear from Steve on this since he was the one who has been asking for this feature, although I'm pretty sure I know what he is going to say. -- paul moore www.paul-moore.com