Messages in this thread | | | Date | Thu, 9 Nov 2017 16:56:38 -0500 | From | Richard Guy Briggs <> | 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@redhat.com> wrote: > > On 2017-11-09 10:59, Paul Moore wrote: > >> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.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@redhat.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@redhat.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
| |