Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[William Roberts <bill.c.roberts@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3726 bytes --]

On Mon, Jul 8, 2013 at 2:55 PM, Eric Paris <eparis@redhat.com> wrote:

> On Mon, 2013-07-08 at 16:28 -0400, Steve Grubb wrote:
> > On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> > > The audit_status structure was not designed with extensibility in mind.
> > > Define a new AUDIT_SET_FEATURE message type which takes a new structure
> > > of bits where things can be enabled/disabled/locked one at a time.
> >
> > This changes how we have been doing things. The way that the audit system
> > settings have been done is to use the AUDIT_SET and AUDIT_GET commands.
> It
> > takes a bit map as the function to perform. We have only used 5 of the 32
> > bits.
> >
> > Do we really need another of the same thing?
>
> It's not the same thing.  This is an interface designed for options
> which have 4 states.  On/Off and Locked/Unlocked.  It is certainly the
> right solution for that problem if we want to solve it generically.
> (look at what it did to the other code who wanted an on/off option)
>
> AUDIT_SET/GET was designed around setting a kernel variable to a single
> value.  It does an ok job at this (although I'd argue that there could
> be a better design here as well, but we have this, so we live with it.)
> It certainly does not form naturally to the 4 states of the new
> interface.
>
> I can certainly shoehorn a 4 state interface into AUDIT_SET/GET.  But I
> believe it will be less obvious and less efficient code.  Maybe that
> doesn't matter too much since only you have to deal with it and it isn't
> run particularly often but I don't see a reason to solve a problem
> inefficiently.  No matter what we are going to have a new design
> pattern, so why not a natural pattern rather than one we are forcing to
> needlessly conform?
>
> Let's say we do want to force ourselves to use the AUDIT_SET/GET netlink
> message.  What do you think the interface should look like?
>
> I can just do the exact same thing inside AUDIT_SET/GET where I
> implement a usable bitmask of on/off/lockable options.  It'd be
> basically the exact same thing as I have now, just more multiplexing
> over the GET/SET message.  Certainly no better.
>
> I could add two new audit status bits, AUDIT_STATUS_LOGINUID_IMMUTABLE
> and AUDIT_STATUS_LOGINUID_IMMUTABLE_LOCKED, along with two __u8's to
> struct audit_status.  One __u8 could be loginuid_immutable and the other
> loginuid_immutable_locked.  I guess that keeps us in line with the
> GET/SET mentality.  I don't see a benefit, but I am biased towards my
> own code.
>
> I could also do it as one __u8 and have magic meaning to the bits inside
> the __u8, aka,
> 0 == off unlocked
> 1 == on unlocked
> 2 == off locked
> 3 == on locked
> but that seems to needlessly make the code more indecipherable to
> humans.  So I won't do it (I see audit_panic and audit_enabled as
> examples of this poor design pattern)
>
> What do you think it should look like and why is it better than mine
> which has proven quite nice for multiple people?
>
> <snip>

I had some code that I submitted to Eric on top of his generic feature
set/get patch, and reduced my change set to 8 lines. The nicest parts of it
is that it is bitpacked, so we wont really need to bump the revision of the
struct up for anything in the near future. AUDIT_SET/GET are unversioned
(if i remember right) so their is no way to add capability to it in a
portable sense. No matter what, it would be nice to get everything new
versioned, their are a bazillion flags in the switch, and its annoying.
Also, in my case, if I can avoid having to add a new flag, it just makes
enhancing the audit subsystem way easier. It felt dirty adding a new case
to the switch...

Bill



-- 
Respectfully,

William C Roberts

[-- Attachment #1.2: Type: text/html, Size: 4427 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]