Message-ID: <20171109215638.rpawzftmese4wjyg@madcap2.tricolour.ca>
Date:   Thu, 9 Nov 2017 16:56:38 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Steve Grubb <sgrubb@...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 2017-11-09 16:47, Paul Moore wrote:
> 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).

Oh, I read that.  That's what informed my position.  That should help
you make your decision.

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

They are two fairly different solutions to the same problem.  It can
stand on its own.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Powered by blists - more mailing lists