Re: [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem

[Amir Goldstein]

On Tue, Oct 25, 2022 at 9:02 PM Stephen Brennan
<stephen.s.brennan@xxxxxxxxxx> wrote:
>
> Amir Goldstein <amir73il@xxxxxxxxx> writes:
>
> > On Fri, Oct 21, 2022 at 11:22 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > ...
> >> > +/*
> >> > + * Objects may need some additional actions to be taken when the last reference
> >> > + * is dropped. Define flags to indicate which actions are necessary.
> >> > + */
> >> > +#define FSNOTIFY_OBJ_FLAG_NEED_IPUT            0x01
> >> > +#define FSNOTIFY_OBJ_FLAG_UPDATE_CHILDREN      0x02
> >>
> >> with changed_flags argument, you do not need these, you can use
> >> the existing CONN_FLAGS.
> >>
> >> It is a bit ugly that the direction of the change is not expressed
> >> in changed_flags, but for the current code, it is not needed, because
> >> update_children does care about the direction of the change and
> >> the direction of change to HAS_IREF is expressed by the inode
> >> object return value.
> >>
> >
> > Oh that is a lie...
> >
> > return value can be non NULL because of an added mark
> > that wants iref and also wants to watch children, but the
> > only practical consequence of this is that you can only
> > do the WARN_ON for the else case of update_children
> > in fsnotify_recalc_mask().
> >
> > I still think it is a win for code simplicity as I detailed
> > in my comments.
> >
> >> Maybe try it out in v3 to see how it works.
> >>
> >> Unless Jan has an idea that will be easier to read and maintain...
> >>
> >
> > Maybe fsnotify_update_inode_conn_flags() should return "update_flags"
> > and not "changed_flags", because actually the WATCHING_CHILDREN
> > flag is not changed by the helper itself.
>
> Yeah, this is the way I'd like to go. The approach of "orig_flags ^
> new_flags" doesn't work since we're not changing the WATCHING_CHILDREN
> flag.
>
> At the end of the day, I do believe that it's equivalent to what I had
> originally, except that we'd use FSNOTIFY_CONN_FLAG_* rather than my new
> FSNOTIFY_OBJ_FLAG_*, which works for me, the new constants are a bit of
> clutter.
>

Yeh, that is what I was aiming for :)

> > Then, HAS_IREF is not returned when helper did get_iref() and changed
> > HAS_IREF itself and then the comment that says:
> >      /* Unpin inode after detach of last mark that wanted iref */
> > will be even clearer:
> >
> >         if (want_iref) {
> >                 /* Pin inode if any mark wants inode refcount held */
> >                 fsnotify_get_inode_ref(fsnotify_conn_inode(conn));
> >                 conn->flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
> >         } else {
> >                 /* Unpin inode after detach of last mark that wanted iref */
> >                 ret = inode;
> >                 update_flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
>
> Is it possible that once the spinlock is dropped, another
> fsnotify_recalc_mask() finds that FSNOTIFY_CONN_FLAG_HAS_IREF is still
> set, and so it also sets FSNOTIFY_CONN_FLAG_HAS_IREF, causing two
> threads to both do an iput?
>

Yeh, that's a braino of mine.
What I wanted to emphasise is that update_flags does not
have HAS_IREF for the iget case, so there is no ambiguity
about what HAS_IREF means in update_flags.

> It may not be possible due to the current use of the functions, but I
> guess it would be safer to clear the connector flag here under the
> spinlock, and set the *update_flags accordingly so that only one thread
> performs the iput().
>

Absolutely.

Thanks,
Amir.