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

[Steve Grubb <sgrubb@redhat.com>]

On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2016-12-07 10:53, Steve Grubb wrote:
> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb@redhat.com> 
wrote:
> >> > > On 2016-12-06 19:17, Paul Moore wrote:
> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb@redhat.com>
> >> > >> 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.
> >> 
> >> To start with, this request comes from users of the audit system. I just
> >> passed along the request. The issue is that when you do auditctl -s, you
> >> get the number of records lost. If you do it the next day, you have to
> >> do math to see what the one day delta is. So, to make reporting easy,
> >> they want it to be reset whenever they do audictl -s.
> >> 
> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
> >> number atomically. Then I can add another commandline option to auditctl
> >> that allows an admin to say also reset the counters. If that command
> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
> >> AUDIT_GET. Thought?> 
> > This would be slightly simpler in kernel implementation than the method
> > I proposed and would work fine, off the top of my head.
> 
> I'd prefer not to introduce another command message type for something
> small like this.
> 
> Steve, do you have any objection to the AUDIT_SET based approach?

Either way, we'd need a feature flag so that I can tell if the kernel supports 
this or not. Also, it should accept "0" as the only valid value. We can do 
this and I can make auditctl do the two back to back before displaying the 
results to minimize the window of risk.

> Based on what you've said above, it would seem like the potential race
> condition with AUDIT_SET wouldn't be a significant issue.

All a matter of perspective. What I think is a reasonable risk someone else 
may disagree. Does anyone else on the list object? If not I'd say go with it.

-Steve