Message-ID: <2668378.rW6Oo1akt8@x2>
Date:   Thu, 09 Nov 2017 10:31:38 -0500
From:   Steve Grubb <sgrubb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Richard Guy Briggs <rgb@...hat.com>, linux-audit@...hat.com,
        linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>
Subject: 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@...hat.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@...hat.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.

Powered by blists - more mailing lists