Message-ID: <CAHC9VhTtjB_BZSBQUYGYscVJ+LfEe4czohUMB_MtD_O9ASJYqQ@mail.gmail.com>
Date:   Thu, 9 Nov 2017 16:47:54 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>,
        Steve Grubb <sgrubb@...hat.com>
Cc:     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 3:52 PM, Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2017-11-09 10:59, Paul Moore wrote:
>> 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.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.

You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.

> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

That confuses me too.  My current thinking is that a partial, or
relative, path is not something we want.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?".

Not to mention we still have the relative path problem in this case.

> How are the analysis tools stymied by a text prefix to a path that it can't find anyways?

I've been wondering the same.  My gut feeling isn't a positive comment
so I'll refrain from sharing it here.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.

The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o.  As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).

I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.

>> >> 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.
>
> So why does this make it unsearchable?  I still don't understand any
> explanations that have been made so far.

Agree.

>> 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.
>
> The irony here stuns me.  2/2 was supposed to be the more controvertial
> one.

Yes, me too.  I never thought patch 1/2 would be the problematic one.
Oh well.  Do you have any objection to 2/2 going up to Linus?

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists