From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jan Kara <jack@suse.cz>
Subject: Re: [PATCH 6/6] audit: Point to fsnotify mark instead of embedding it
Date: Wed, 4 Jul 2018 14:34:08 +0200
Message-ID: <20180704123408.rpgw7oa7sjodoufi@quack2.suse.cz>
References: <20180628164014.4925-1-jack@suse.cz>
	<20180628164014.4925-7-jack@suse.cz>
	<CAOQ4uxjg4zGj59G9rKybH=Qpvg0mU6dA=26LAuH=LSJQ16wzcQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <linux-audit-bounces@redhat.com>
Content-Disposition: inline
In-Reply-To: <CAOQ4uxjg4zGj59G9rKybH=Qpvg0mU6dA=26LAuH=LSJQ16wzcQ@mail.gmail.com>
List-Unsubscribe: <https://www.redhat.com/mailman/options/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/linux-audit>
List-Post: <mailto:linux-audit@redhat.com>
List-Help: <mailto:linux-audit-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/linux-audit>,
	<mailto:linux-audit-request@redhat.com?subject=subscribe>
Sender: linux-audit-bounces@redhat.com
Errors-To: linux-audit-bounces@redhat.com
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Richard Guy Briggs <rgb@redhat.com>, Linux Audit <linux-audit@redhat.com>, Al Viro <viro@zeniv.linux.org.uk>, linux-fsdevel <linux-fsdevel@vger.kernel.org>
List-Id: linux-audit@redhat.com

On Fri 29-06-18 16:20:12, Amir Goldstein wrote:
> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@suse.cz> wrote:
> > Audit tree code currently embeds fsnotify mark in audit_chunk. As chunk
> > attached to an inode is replace when new tag is added / removed, we also
> > need to remove old fsnotify mark and add a new one on such occasion.
> > This is cumbersome and makes locking rules somewhat difficult to follow.
> >
> > Fix these problems by allocating fsnotify mark independently and keeping
> > it all the time while there is some chunk attached to an inode. Also add
> > documentation about the locking rules so that things are easier to
> > follow.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> 
> Wow! this patch is a lot to take in at once.
> I wonder if it would be possible to split it to:
> - make mark not embedded and take chunk reference
> - replace_mark_chunk() and rid of cumbersome code

OK, I'll try something.

> Or any other simplification that would help me survive this review.
> 
> In the mean while just one nit below...
> 
> [...]
> > +       mutex_lock(&entry->group->mark_mutex);
> > +       spin_lock(&hash_lock);
> > +       chunk = AUDIT_M(entry)->chunk;
> > +       replace_mark_chunk(entry, NULL);
> > +       spin_unlock(&hash_lock);
> > +       if (chunk) {
> > +               mutex_unlock(&entry->group->mark_mutex);
> > +               evict_chunk(chunk);
> > +               audit_mark_put_chunk(chunk);
> > +       } else {
> > +               mutex_unlock(&entry->group->mark_mutex);
> > +       }
> 
> else case seems like a leftover or something.

Fixed. Thanks.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR