On 2/10/23 3:59 PM, Paul Moore wrote: > On Fri, Feb 10, 2023 at 5:00 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote: >> On 2023-02-10 11:52, Paul Moore wrote: >>> On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@xxxxxxxxx> wrote: >>>> On 2/10/23 8:39?AM, Paul Moore wrote: >>>>> On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@xxxxxxxxx> wrote: >>>>>> On 2/9/23 3:54?PM, Steve Grubb wrote: >>>>>>> On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: >>>>>>>> On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote: >>>>>>>>> On 2023-02-01 16:18, Paul Moore wrote: >>>>>>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@xxxxxxxxxx> >>>>>>> wrote: >>>>>>>>>>> fadvise and madvise both provide hints for caching or access pattern >>>>>>>>>>> for file and memory respectively. Skip them. >>>>>>>>>> >>>>>>>>>> You forgot to update the first sentence in the commit description :/ >>>>>>>>> >>>>>>>>> I didn't forget. I updated that sentence to reflect the fact that the >>>>>>>>> two should be treated similarly rather than differently. >>>>>>>> >>>>>>>> Ooookay. Can we at least agree that the commit description should be >>>>>>>> rephrased to make it clear that the patch only adjusts madvise? Right >>>>>>>> now I read the commit description and it sounds like you are adjusting >>>>>>>> the behavior for both fadvise and madvise in this patch, which is not >>>>>>>> true. >>>>>>>> >>>>>>>>>> I'm still looking for some type of statement that you've done some >>>>>>>>>> homework on the IORING_OP_MADVISE case to ensure that it doesn't end >>>>>>>>>> up calling into the LSM, see my previous emails on this. I need more >>>>>>>>>> than "Steve told me to do this". >>>>>>>>>> >>>>>>>>>> I basically just want to see that some care and thought has gone into >>>>>>>>>> this patch to verify it is correct and good. >>>>>>>>> >>>>>>>>> Steve suggested I look into a number of iouring ops. I looked at the >>>>>>>>> description code and agreed that it wasn't necessary to audit madvise. >>>>>>>>> The rationale for fadvise was detemined to have been conflated with >>>>>>>>> fallocate and subsequently dropped. Steve also suggested a number of >>>>>>>>> others and after investigation I decided that their current state was >>>>>>>>> correct. *getxattr you've advised against, so it was dropped. It >>>>>>>>> appears fewer modifications were necessary than originally suspected. >>>>>>>> >>>>>>>> My concern is that three of the four changes you initially proposed >>>>>>>> were rejected, which gives me pause about the fourth. You mention >>>>>>>> that based on your reading of madvise's description you feel auditing >>>>>>>> isn't necessary - and you may be right - but based on our experience >>>>>>>> so far with this patchset I would like to hear that you have properly >>>>>>>> investigated all of the madvise code paths, and I would like that in >>>>>>>> the commit description. >>>>>>> >>>>>>> I think you're being unnecessarily hard on this. Yes, the commit message >>>>>>> might be touched up. But madvise is advisory in nature. It is not security >>>>>>> relevant. And a grep through the security directory doesn't turn up any >>>>>>> hooks. >>>>>> >>>>>> Agree, it's getting a bit anal... FWIW, patch looks fine to me. >>>>> >>>>> Call it whatever you want, but the details are often important at this >>>>> level of code, and when I see a patch author pushing back on verifying >>>>> that their patch is correct it makes me very skeptical. >>>> >>>> Maybe it isn't intended, but the replies have generally had a pretty >>>> condescending tone to them. That's not the best way to engage folks, and >>>> may very well be why people just kind of give up on it. Nobody likes >>>> debating one-liners forever, particularly not if it isn't inviting. >>> >>> I appreciate that you are coming from a different space, but I stand >>> by my comments. Of course you are welcome to your own opinion, but I >>> would encourage you to spend some time reading the audit mail archives >>> going back a few years before you make comments like the above ... or >>> not, that's your call; I recognize it is usually easier to criticize. >>> >>> On a quasi related note to the list/archives: unfortunately there was >>> continued resistance to opening up the linux-audit list so I've setup >>> audit@vger for upstream audit kernel work moving forward. The list >>> address in MAINTAINERS will get updated during the next merge window >>> so hopefully some of the problems you had in the beginning of this >>> discussion will be better in the future. >>> >>>>> I really would have preferred that you held off from merging this >>>>> until this was resolved and ACK'd ... oh well. >>>> >>>> It's still top of tree. If you want to ack it, let me know and I'll add >>>> it. If you want to nak it, give me something concrete to work off of. >>> >>> I can't in good conscience ACK it without some comment from Richard >>> that he has traced the code paths; this shouldn't be surprising at >>> this point. I'm not going to NACK it or post a revert, I would have >>> done that already if I felt that was appropriate. Right now this >>> patch is in a gray area for me in that I suspect it is good, but I >>> can't ACK it without some comment that it has been properly >>> researched. >> >> I feel a bit silly replying in this thread. My dad claims that I need >> to have the last word in any argument, so that way he gets it instead... >> >> I appear to have accidentally omitted the connector word "and" between >> "description" and "code" above, which may have led you to doubt I had >> gone back and re-looked at the code. > > Okay, as long as you've done the homework on this I'm good. If it's > still on the top of Jen's tree, here's my ACK: > > Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx> > > ... if it's not on top of the tree, it's not worth popping patches to > add the ACK IMHO. Thanks - and it is, so I added the acked-by. > Feel free to reply to this Richard if you want to have the last word > in this thread, I think I'm done ;) Let's close it up :-) -- Jens Axboe
