On Mon, Jul 8, 2013 at 2:55 PM, Eric Paris <[email protected]> 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
-- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
