mbox series

[0/11,v3] audit: Fix various races when tagging and untagging mounts

Message ID 20180904160632.21210-1-jack@suse.cz (mailing list archive)
Headers show
Series audit: Fix various races when tagging and untagging mounts | expand

Message

Jan Kara Sept. 4, 2018, 4:06 p.m. UTC
Hello,

this is the third revision of the series that addresses problems I have
identified when trying to understand how exactly is kernel/audit_tree.c using
generic fsnotify framework. I hope I have understood all the interactions right
but careful review is certainly welcome.

The patches have been tested by a stress test I have written which mounts &
unmounts filesystems in the directory tree while adding and removing audit
rules for this tree in parallel and accessing the tree to generate events.
Still some real-world testing would be welcome.

Changes since v2:
* Fixed up mark freeing to use proper pointer as pointed out by Amir
* Changed some naming based on Paul's review

Changes since v1:
* Split the last patch to ease review
* Rewrite test script so that it can be included in audit testsuite
* Some cleanups and improvements suggested by Amir

								Honza

Comments

Richard Guy Briggs Sept. 14, 2018, 7:13 p.m. UTC | #1
On 2018-09-04 18:06, Jan Kara wrote:
> Hello,

Jan,

> this is the third revision of the series that addresses problems I have
> identified when trying to understand how exactly is kernel/audit_tree.c using
> generic fsnotify framework. I hope I have understood all the interactions right
> but careful review is certainly welcome.

I've tried to review it as carefully as I am able.  As best I understand
it, this all looks reasonable and an improvement over the previous
state.  Thanks for the hard work.

FWIW,
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> The patches have been tested by a stress test I have written which mounts &
> unmounts filesystems in the directory tree while adding and removing audit
> rules for this tree in parallel and accessing the tree to generate events.
> Still some real-world testing would be welcome.
> 
> Changes since v2:
> * Fixed up mark freeing to use proper pointer as pointed out by Amir
> * Changed some naming based on Paul's review
> 
> Changes since v1:
> * Split the last patch to ease review
> * Rewrite test script so that it can be included in audit testsuite
> * Some cleanups and improvements suggested by Amir
> 
> 								Honza

- 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
Jan Kara Sept. 17, 2018, 4:57 p.m. UTC | #2
On Fri 14-09-18 15:13:28, Richard Guy Briggs wrote:
> On 2018-09-04 18:06, Jan Kara wrote:
> > Hello,
> 
> Jan,
> 
> > this is the third revision of the series that addresses problems I have
> > identified when trying to understand how exactly is kernel/audit_tree.c using
> > generic fsnotify framework. I hope I have understood all the interactions right
> > but careful review is certainly welcome.
> 
> I've tried to review it as carefully as I am able.  As best I understand
> it, this all looks reasonable and an improvement over the previous
> state.  Thanks for the hard work.
> 
> FWIW,
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

Thanks for review! Paul should I send you updated patch 9 with that one
variable renamed or will you do that small change while merging the series?

								Honza
Paul Moore Oct. 4, 2018, 1:20 a.m. UTC | #3
On Mon, Sep 17, 2018 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> On Fri 14-09-18 15:13:28, Richard Guy Briggs wrote:
> > On 2018-09-04 18:06, Jan Kara wrote:
> > > Hello,
> >
> > Jan,
> >
> > > this is the third revision of the series that addresses problems I have
> > > identified when trying to understand how exactly is kernel/audit_tree.c using
> > > generic fsnotify framework. I hope I have understood all the interactions right
> > > but careful review is certainly welcome.
> >
> > I've tried to review it as carefully as I am able.  As best I understand
> > it, this all looks reasonable and an improvement over the previous
> > state.  Thanks for the hard work.
> >
> > FWIW,
> > Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
>
> Thanks for review! Paul should I send you updated patch 9 with that one
> variable renamed or will you do that small change while merging the series?

Hi Jan,

Thanks again for these patches and your patience; some travel,
holidays, and a job change delayed my review.  However, everything
looks okay to me (minus the one problem I noted in patch 09/11).  I've
added the patches to audit/working-fsnotify_fixes and I'm going to
start stressing them as soon as I get a test kernel built with the
idea of merging them into audit/next as soon as the upcoming merge
window closes.

As far as the variable rename is concerned, that's not something I
would prefer to change during a merge, but if you or Richard wanted to
submit a renaming patch I would be okay with that in this case.  If
you do submit the rename patch, please base it on top of this patchset
(or audit/working-fsnotify_fixes).

Thanks!
Jan Kara Oct. 4, 2018, 6:59 a.m. UTC | #4
On Wed 03-10-18 21:20:35, Paul Moore wrote:
> On Mon, Sep 17, 2018 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > On Fri 14-09-18 15:13:28, Richard Guy Briggs wrote:
> > > On 2018-09-04 18:06, Jan Kara wrote:
> > > > Hello,
> > >
> > > Jan,
> > >
> > > > this is the third revision of the series that addresses problems I have
> > > > identified when trying to understand how exactly is kernel/audit_tree.c using
> > > > generic fsnotify framework. I hope I have understood all the interactions right
> > > > but careful review is certainly welcome.
> > >
> > > I've tried to review it as carefully as I am able.  As best I understand
> > > it, this all looks reasonable and an improvement over the previous
> > > state.  Thanks for the hard work.
> > >
> > > FWIW,
> > > Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > Thanks for review! Paul should I send you updated patch 9 with that one
> > variable renamed or will you do that small change while merging the series?
> 
> Hi Jan,
> 
> Thanks again for these patches and your patience; some travel,
> holidays, and a job change delayed my review.  However, everything
> looks okay to me (minus the one problem I noted in patch 09/11).  I've
> added the patches to audit/working-fsnotify_fixes and I'm going to
> start stressing them as soon as I get a test kernel built with the
> idea of merging them into audit/next as soon as the upcoming merge
> window closes.
> 
> As far as the variable rename is concerned, that's not something I
> would prefer to change during a merge, but if you or Richard wanted to
> submit a renaming patch I would be okay with that in this case.  If
> you do submit the rename patch, please base it on top of this patchset
> (or audit/working-fsnotify_fixes).

Great, thanks. I will send the rename patch in a moment.

								Honza