Re: audit_status in kernel
On Mon, 2014-03-10 at 18:25 -0400, Steve Grubb wrote:
On Monday, March 10, 2014 05:48:06 PM Steve Grubb wrote:
> Hello,
>
> I was looking at a new kernel and see that the audit_status structure has
> changed. The first member of the structure is a bit mask that tells what all
> is in the structure.
That's not what the bit does. The bit tells what portions of the struct
contain updates the kernel should upon. Notice that the 'lost' and
'backlog' fields have no bit assigned.
So, if we add this:
>
> __u32 version; /* audit api version number */
> __u32 backlog_wait_time;/* message queue wait timeout */
> };
>
> Then we need to have a define for those two:
>
> #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
> +#define AUDIT_STATTUS_VERSION 0x0020
> -#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
> +#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0040
>
> IOW, each entry in the structure is supposed to have a mask value.
If the version were a writable field then a bit would be necessary. But
as of right now, it isn't writable, therefore the bit serves no purpose.
Just like 'lost' and 'backlog'
Actually, I think the problems are worse. We have the issue of an
expanding
data structure over time as new things get added. But yet we have a fixed sized
audit_status structure.
Sure, every program is compiled with a fixed size. This is very common.
I don't recognize the problem. You have to compile a new userspace to
make use of new functions/features. No matter what.
I could copy that into the audit package's source code
so that I have the new structure definition. But I will have to do the same
thing each time.
As userspace comes to understand new parts of the structure userspace
would need to grow the definition. Agreed. Every design has this
issue. Userspace can't understand a feature unless you tell it.
Also, how would an old kernel tolerate a bigger audit_status
structure being sent with AUDIT_SET commands by a new auditctl?
Unknown bits have always been ignored. I thought we returned EINVAL on
unknown bits, but it seems we don't. We could change that going
forward, but, it doesn't matter. Userspace must query the kernel for
the version. And can then obviously know what is/isn't supported.
What we should do is leave AUDIT_GET the way it was. We should then
define
AUDIT_GET_EXT and then use it a lot like audit_rule_data which has
struct audit_status_ext {
__u32 field_count;
__u32 fields[AUDIT_MAX_FIELDS];
__u32 values[AUDIT_MAX_FIELDS];
};
Then insert the field identifier in fields and the value in the other. This way
the format is defined once and we can reuse it for a long time.
That adds absolutely nothing except wasted memory and greater
readability problems... We'd be sending around gigantic empty stuctures
for no gain... (although on AUDIT_FEATURE_SET the current interface is
totally craptastic as all but one of the fields is typically useless,
but at least this is the craptastic we have today)
From user space, the migration would be easy. Old auditctl still uses
AUDIT_GET. New auditctl would try AUDIT_GET_EXT and if that's not recognized,
drop back to AUDIT_GET. Then one day down the road we remove AUDIT_GET in the
kernel.
This is how we did the migration from AUDIT_RULES to AUDIT_RULES_DATA.
Great. But this isn't needed here. This isn't an ABI change. Old
userspace will run just fine on new kernels. New userspace will run
just fine on old kernels. (Although (poorly coded userspace sending)
new features will be silently ignored instead of rejected, as I would
prefer)
It really is easy. On GET userspace just needs to use
struct audit_status s;
memset(&s, 0, sizeof(s));
/* guard against past and future API changes */
memcpy(&s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));
if (s.version == 0)
no new features, legacy api (notice this works because you memset the
userspace status struct before you copied in the kernel verion. So
stuff at the end is all 0's
if (s.version == AUDIT_VERSION_BACKLOG_LIMIT)
version field, but nothing else
if (s.version >= AUDIT_VERSION_BACKLOG_WAIT_TIME)
you can set the version field!
And the struct can keep growing and keep being used! Both ends know how
to handle things, all with no new API, no new deprecation, no breaking
working code. I have actually written kernel<->userspace APIs before.
Admittedly this time I was starting with junk, but we still found a way
to do it allowing us to grow into the future and keep everything working
just the way it always has.
-Eric