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@xxxxxxxxx>
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
...
> > -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@xxxxxxxx>
SUSE Labs, CR
