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(a)redhat.com&gt; wrote: > On 2017-11-09 10:59, Paul Moore wrote: >> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb(a)redhat.com&gt; 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(a)redhat.com&gt; 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