public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Paul Moore <[email protected]>
Cc: Steve Grubb <[email protected]>,
	Richard Guy Briggs <[email protected]>,
	Linux-Audit Mailing List <[email protected]>,
	LKML <[email protected]>,
	[email protected], Eric Paris <[email protected]>,
	Christian Brauner <[email protected]>,
	Stefan Roesch <[email protected]>
Subject: Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
Date: Fri, 10 Feb 2023 09:00:11 -0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHC9VhTGmGJ81M2CZWsTf1kNf8XNz2WsYFAP=5VAVSUfUiu1yQ@mail.gmail.com>

On 2/10/23 8:39?AM, Paul Moore wrote:
> On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <[email protected]> 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 <[email protected]> wrote:
>>>>> On 2023-02-01 16:18, Paul Moore wrote:
>>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <[email protected]>
>>> 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 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.

-- 
Jens Axboe


  reply	other threads:[~2023-02-10 16:00 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-01 20:33 [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE Richard Guy Briggs
2023-02-01 21:18 ` Paul Moore
2023-02-09 21:53   ` Richard Guy Briggs
2023-02-09 22:37     ` Paul Moore
2023-02-09 22:54       ` Steve Grubb
2023-02-10  0:15         ` Jens Axboe
2023-02-10 15:39           ` Paul Moore
2023-02-10 16:00             ` Jens Axboe [this message]
2023-02-10 16:52               ` Paul Moore
2023-02-10 16:58                 ` Jens Axboe
2023-02-10 22:00                 ` Richard Guy Briggs
2023-02-10 22:59                   ` Paul Moore
2023-02-10 23:01                     ` Jens Axboe
2023-02-10 15:33         ` Paul Moore
2023-02-10  1:31 ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox