Re: [PATCH 20/33] fsnotify: Detach mark from object list when last reference is dropped

[Jan Kara <jack@suse.cz>]

On Fri 31-03-17 17:42:17, Miklos Szeredi wrote:
> 
> On Tue, Mar 28, 2017 at 06:13:19PM +0200, Jan Kara wrote:
> > Instead of removing mark from object list from fsnotify_detach_mark(),
> > remove the mark when last reference to the mark is dropped. This will
> > allow fanotify to wait for userspace response to event without having to
> > hold onto fsnotify_mark_srcu.
> > 
> > To avoid pinning inodes by elevated refcount (and thus e.g. delaying
> > file deletion) while someone holds mark reference, we detach connector
> > from the object also from fsnotify_destroy_marks() and not only after
> > removing last mark from the list as it was now.
> > 
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
...
> > -static struct inode *fsnotify_detach_from_object(struct fsnotify_mark *mark)
> > +static struct inode *fsnotify_detach_connector_from_object(
> > +					struct fsnotify_mark_connector *conn)
> > +{
> > +	struct inode *inode = NULL;
> > +
> > +	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) {
> > +		inode = conn->inode;
> > +		rcu_assign_pointer(inode->i_fsnotify_marks, NULL);
> > +		inode->i_fsnotify_mask = 0;
> > +		conn->inode = NULL;
> > +		conn->flags &= ~FSNOTIFY_OBJ_TYPE_INODE;
> > +	} else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
> > +		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify_marks,
> > +				   NULL);
> > +		real_mount(conn->mnt)->mnt_fsnotify_mask = 0;
> > +		conn->mnt = NULL;
> > +		conn->flags &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> > +	}
> > +
> > +	return inode;
> > +}
> 
> Could this helper been added in the previous patch where the code was
> introduced?

Yeah, possibly. I'll do that.

> > @@ -195,6 +221,9 @@ static struct inode *fsnotify_detach_from_object(struct fsnotify_mark *mark)
> >  	mark->connector = NULL;
> >  	spin_unlock(&conn->lock);
> >  
> > +	if (inode)
> > +		iput(inode);
> > +
> 
> iput() checks for non-NULL inode.

OK.

> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index c0e494fd8eca..152400e8d077 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -172,27 +172,15 @@ static unsigned long inode_to_key(const struct inode *inode)
> >  /*
> >   * Function to return search key in our hash from chunk. Key 0 is special and
> >   * should never be present in the hash.
> > - *
> > - * Must be called with chunk->mark.lock held to protect from connector
> > - * becoming NULL.
> >   */
> > -static unsigned long __chunk_to_key(struct audit_chunk *chunk)
> > +static unsigned long chunk_to_key(struct audit_chunk *chunk)
> >  {
> > -	if (!chunk->mark.connector)
> > +	/* We have a reference to the mark so it should be attached. */
> > +	if (WARN_ON_ONCE(!chunk->mark.connector))
> >  		return 0;
> 
> Hmm, lifetime of mark previously:
> 
> - created but not attached (connector is NULL, no FSNOTIFY_MARK_FLAG_ATTACHED)
> - attached (connector is non-NULL, FSNOTIFY_MARK_FLAG_ATTACHED)
> - detached (connector is NULL, no FSNOTIFY_MARK_FLAG_ATTACHED)
> 
> The created, and attached states remain the same but detached now changed to:
> 
> - detached (connector is non-NULL, no FSNOTIFY_MARK_FLAG_ATTACHED)
> 
> Considering that, the warning seems right but the comment above not so.
> Maybe something like:
> 
>       /* We have a reference to the mark and it has been attached so we should
>          have a valid connector. */

Good point. The mark can be attached to group (that is what
FSNOTIFY_MARK_FLAG_ATTACHED reflects) and to connector which is what I was
speaking about in my comment. And I tend to mix these two in comments which
is confusing. I'll clarify.

> > @@ -202,7 +190,7 @@ static inline struct list_head *chunk_hash(unsigned long key)
> >  /* hash_lock & entry->lock is held by caller */
> >  static void insert_hash(struct audit_chunk *chunk)
> >  {
> > -	unsigned long key = __chunk_to_key(chunk);
> > +	unsigned long key = chunk_to_key(chunk);
> >  	struct list_head *list;
> >  
> >  	if (!key)
> > @@ -263,7 +251,7 @@ static void untag_chunk(struct node *p)
> >  
> >  	mutex_lock(&entry->group->mark_mutex);
> >  	spin_lock(&entry->lock);
> > -	if (chunk->dead || !entry->connector) {
> > +	if (chunk->dead || !entry->connector || !entry->connector->inode) {
> 
> So should we be testing FSNOTIFY_MARK_FLAG_ATTACHED instead?  Without
> understanding what audit is trying to do, that would be a lot more
> logical.  Or maybe there is a reason this is correct, it just needs an
> explanation.

That's an interesting idea. AFAIU checking FSNOTIFY_MARK_FLAG_ATTACHED
should be good and doing so would somewhat simplify the patches as well.
I'll try to do that.

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