Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb(a)redhat.com>
wrote:
> On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
>> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb(a)redhat.com>
wrote:
>> > Tracefs or debugfs were causing hundreds to thousands
of null PATH
>> > records to be associated with the init_module and finit_module SYSCALL
>> > records on a few modules when the following rule was in place for
>> >
>> > startup:
>> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>> >
>> > This happens because the parent inode is not found in the task's
>> > audit_names list and hence treats it as anonymous. This gives us no
>> > information other than a numerical device number that may no longer be
>> > visible upon log inspeciton, and an inode number.
>> >
>> > Fill in the filesystem type, filesystem magic number and full pathname
>> > from the filesystem mount point on previously null PATH records from
>> > entries that have an anonymous parent from the child dentry using
>> > dentry_path_raw().
>
> Late reply...but I just noticed that this changes the format of the
"name"
> field - which is undesirable. Please put the file system type in a field
> all by itself called "fstype". You can just leave it as the hex magic
> number prepended with 0x and user space can do the lookup from there,
>
> It might be simplest to just apply a corrective patch over top of this one
> so that you don't have to muck about with git branches and commit
> messages.
A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.
Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.
As for the objection itself: ungh. There is really no good reason
why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.
I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used
in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.
-Steve
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.