| Message ID | b5dfdcd541115c86dbc774aa9dd502c964849c5f.1675282642.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] io_uring,audit: don't log IORING_OP_MADVISE | expand |
On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> 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'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. > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring") > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > changelog > v2: > - drop *GETXATTR patch > - drop FADVISE hunk > > io_uring/opdef.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/io_uring/opdef.c b/io_uring/opdef.c > index 3aa0d65c50e3..d3f36c633ceb 100644 > --- a/io_uring/opdef.c > +++ b/io_uring/opdef.c > @@ -312,6 +312,7 @@ const struct io_op_def io_op_defs[] = { > .issue = io_fadvise, > }, > [IORING_OP_MADVISE] = { > + .audit_skip = 1, > .name = "MADVISE", > .prep = io_madvise_prep, > .issue = io_madvise, > -- > 2.27.0
On 2023-02-01 16:18, Paul Moore wrote: > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> 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. > 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. > > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring") > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > changelog > > v2: > > - drop *GETXATTR patch > > - drop FADVISE hunk > > > > io_uring/opdef.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/io_uring/opdef.c b/io_uring/opdef.c > > index 3aa0d65c50e3..d3f36c633ceb 100644 > > --- a/io_uring/opdef.c > > +++ b/io_uring/opdef.c > > @@ -312,6 +312,7 @@ const struct io_op_def io_op_defs[] = { > > .issue = io_fadvise, > > }, > > [IORING_OP_MADVISE] = { > > + .audit_skip = 1, > > .name = "MADVISE", > > .prep = io_madvise_prep, > > .issue = io_madvise, > > -- > > 2.27.0 > > -- > paul-moore.com > - 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
On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2023-02-01 16:18, Paul Moore wrote: > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> 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.
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@redhat.com> wrote: > > On 2023-02-01 16:18, Paul Moore wrote: > > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> 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. -Steve
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@redhat.com> wrote: >>> On 2023-02-01 16:18, Paul Moore wrote: >>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > 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.
On Wed, 01 Feb 2023 15:33:33 -0500, Richard Guy Briggs wrote: > fadvise and madvise both provide hints for caching or access pattern for > file and memory respectively. Skip them. > > Applied, thanks! [1/1] io_uring,audit: don't log IORING_OP_MADVISE commit: 2bd59885eb9c2094d118b4321d5f74e12e77ef0f Best regards,
On Thu, Feb 9, 2023 at 5:54 PM Steve Grubb <sgrubb@redhat.com> 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@redhat.com> wrote: > > > On 2023-02-01 16:18, Paul Moore wrote: > > > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > 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. Asking that a patch author does the proper level of due diligence to ensure that the patch they are submitting is correct isn't being "unnecessarily hard", it's part of being a good code reviewer and maintainer. I'm a bit amazed that you and Richard would rather spend your time arguing about this rather than spending the hour (?) it would take to verify the change and make a proper statement about the correctness of the patch. > 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. You can't rely on grep, you need to chase the code paths to see what code might be exercised. For example, look at the truncate syscalls (truncate, truncate64, ftruncate64, etc.), if you grep through the SELinux hook code looking for some form of "truncate" you will not find anything relevant; SELinux doesn't provide implementations for either the security_file_truncate() or security_path_truncate() hooks. However, if you follow the syscall code paths you will see that the truncate syscalls end up calling security_inode_permission() which *is* implemented in SELinux. You need to do your homework; relying only on a gut feeling, a few lines from a manpage, or a code comment is a good way to introduce bugs.
On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: > >>> On 2023-02-01 16:18, Paul Moore wrote: > >>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > > 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. I really would have preferred that you held off from merging this until this was resolved and ACK'd ... oh well.
On 2/10/23 8:39?AM, Paul Moore wrote: > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: >>>>> On 2023-02-01 16:18, Paul Moore wrote: >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> >>> 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.
On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@kernel.dk> wrote: > On 2/10/23 8:39?AM, Paul Moore wrote: > > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: > >>>>> On 2023-02-01 16:18, Paul Moore wrote: > >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > >>> 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.
On 2/10/23 9:52?AM, Paul Moore wrote: > On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@kernel.dk> wrote: >> On 2/10/23 8:39?AM, Paul Moore wrote: >>> On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: >>>>>>> On 2023-02-01 16:18, Paul Moore wrote: >>>>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> >>>>> 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. I'm just saying how it was received on my end, you can take that as constructive feedback or ignore it. I don't need to read the archives for that as it is not related to anything but this thread, it was not meant to reflect a general concern outside of this thread. > 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. OK good, I keep forgetting to delete it from the replies and get annoyed at the spam I get back... Thanks for fixing that going forward. >>> 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. Richard, can you do the due diligence here? Steve did say: "But madvise is advisory in nature. It is not security relevant. And a grep through the security directory doesn't turn up any hooks." Seems to me if we're not currently auditing madvise outside of io_uring, then why would we do it here?
On 2023-02-10 11:52, Paul Moore wrote: > On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@kernel.dk> wrote: > > On 2/10/23 8:39?AM, Paul Moore wrote: > > > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: > > >>>>> On 2023-02-01 16:18, Paul Moore wrote: > > >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > > >>> 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. > paul-moore.com - 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
On Fri, Feb 10, 2023 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2023-02-10 11:52, Paul Moore wrote: > > On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@kernel.dk> wrote: > > > On 2/10/23 8:39?AM, Paul Moore wrote: > > > > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: > > > >>>>> On 2023-02-01 16:18, Paul Moore wrote: > > > >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> > > > >>> 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@paul-moore.com> ... if it's not on top of the tree, it's not worth popping patches to add the ACK IMHO. Feel free to reply to this Richard if you want to have the last word in this thread, I think I'm done ;)
On 2/10/23 3:59 PM, Paul Moore wrote: > On Fri, Feb 10, 2023 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote: >> On 2023-02-10 11:52, Paul Moore wrote: >>> On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe <axboe@kernel.dk> wrote: >>>> On 2/10/23 8:39?AM, Paul Moore wrote: >>>>> On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe <axboe@kernel.dk> 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@redhat.com> wrote: >>>>>>>>> On 2023-02-01 16:18, Paul Moore wrote: >>>>>>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs <rgb@redhat.com> >>>>>>> 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@paul-moore.com> > > ... 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 :-)
diff --git a/io_uring/opdef.c b/io_uring/opdef.c index 3aa0d65c50e3..d3f36c633ceb 100644 --- a/io_uring/opdef.c +++ b/io_uring/opdef.c @@ -312,6 +312,7 @@ const struct io_op_def io_op_defs[] = { .issue = io_fadvise, }, [IORING_OP_MADVISE] = { + .audit_skip = 1, .name = "MADVISE", .prep = io_madvise_prep, .issue = io_madvise,
fadvise and madvise both provide hints for caching or access pattern for file and memory respectively. Skip them. Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring") Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- changelog v2: - drop *GETXATTR patch - drop FADVISE hunk io_uring/opdef.c | 1 + 1 file changed, 1 insertion(+)