Message-ID: <CAHC9VhQMzR06Tqr8WNDuV9zgnDfH8reaKam+e_sAFCm1ZK3QQQ@mail.gmail.com>
Date:   Thu, 9 Nov 2017 10:59:30 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Steve Grubb <sgrubb@...hat.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 Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@...hat.com> wrote:
> 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:

...

>> > 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.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left.  Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches.  Neither is a great option, and that
is why I'm rather annoyed.

>> 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.

There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways.  Regardless, as you say, the problem can likely be avoided
this time.

>> 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.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists