Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1534735 > unrolled thread

Re: fsnotify_mark_srcu wtf?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: fsnotify_mark_srcu wtf? Miklos Szeredi <miklos@szeredi.hu> - 2016-12-02 09:30 +0100
    Re: fsnotify_mark_srcu wtf? Jan Kara <jack@suse.cz> - 2016-12-02 11:50 +0100
      Re: fsnotify_mark_srcu wtf? Miklos Szeredi <miklos@szeredi.hu> - 2016-12-02 12:10 +0100
        Re: fsnotify_mark_srcu wtf? Jan Kara <jack@suse.cz> - 2016-12-07 10:00 +0100
      Re: fsnotify_mark_srcu wtf? Amir Goldstein <amir73il@gmail.com> - 2016-12-02 12:50 +0100
        Re: fsnotify_mark_srcu wtf? Amir Goldstein <amir73il@gmail.com> - 2016-12-02 13:00 +0100

#1534735 — Re: fsnotify_mark_srcu wtf?

On Thu, Nov 10, 2016 at 8:46 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 09-11-16 20:26:16, Amir Goldstein wrote:
>> On Wed, Nov 9, 2016 at 1:10 PM, Jan Kara <jack@suse.cz> wrote:

>> > And this does not work as well... Fanotify must notify groups by their
>> > priority so you cannot arbitrarily reorder ordering in which groups get
>> > notified. I'm currently pondering on using mark refcount to pin it when
>> > processing permission event but there are still some details to check.
>> >
>>
>> All right, mark refcount sound like the proper solution.
>
> Except it doesn't quite work. We can pin the current marks by a refcount
> but they can still be removed from the list so after we regain srcu lock,
> we are not sure their ->next pointers still point to still allocated marks
> :-| Sadly I realized this only after implementing all this.

Hmm, how about this: when removing mark from inode, drop refcount.  If
refcount is zero can remove from list.  Otherwise mark the mark "dead"
and leave it on the list.

And fsnotify can just skip dead marks.

Thanks,
Miklos

[toc] | [next] | [standalone]

#1534818

On Fri 02-12-16 09:26:51, Miklos Szeredi wrote:
> On Thu, Nov 10, 2016 at 8:46 PM, Jan Kara <jack@suse.cz> wrote:
> > On Wed 09-11-16 20:26:16, Amir Goldstein wrote:
> >> On Wed, Nov 9, 2016 at 1:10 PM, Jan Kara <jack@suse.cz> wrote:
> 
> >> > And this does not work as well... Fanotify must notify groups by their
> >> > priority so you cannot arbitrarily reorder ordering in which groups get
> >> > notified. I'm currently pondering on using mark refcount to pin it when
> >> > processing permission event but there are still some details to check.
> >> >
> >>
> >> All right, mark refcount sound like the proper solution.
> >
> > Except it doesn't quite work. We can pin the current marks by a refcount
> > but they can still be removed from the list so after we regain srcu lock,
> > we are not sure their ->next pointers still point to still allocated marks
> > :-| Sadly I realized this only after implementing all this.
> 
> Hmm, how about this: when removing mark from inode, drop refcount.  If
> refcount is zero can remove from list.  Otherwise mark the mark "dead"
> and leave it on the list.
> 
> And fsnotify can just skip dead marks.

I had this idea as well and when trying to implement this, I've stumbled
over some problems. I think the biggest problem was that destruction of a
notification mark is relatively complex operation (doing iput() for
example) and quite a few places dropping mark references are in a context
where this can cause problems. Also I don't want to defer iput() to a
workqueue as that will have unexpected consequences such as unlinked
watched inode lingering in the system (possibly colliding with umount
etc.).

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

[toc] | [prev] | [next] | [standalone]

#1534843

On Fri, Dec 2, 2016 at 11:48 AM, Jan Kara <jack@suse.cz> wrote:
> On Fri 02-12-16 09:26:51, Miklos Szeredi wrote:
>> On Thu, Nov 10, 2016 at 8:46 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Wed 09-11-16 20:26:16, Amir Goldstein wrote:
>> >> On Wed, Nov 9, 2016 at 1:10 PM, Jan Kara <jack@suse.cz> wrote:
>>
>> >> > And this does not work as well... Fanotify must notify groups by their
>> >> > priority so you cannot arbitrarily reorder ordering in which groups get
>> >> > notified. I'm currently pondering on using mark refcount to pin it when
>> >> > processing permission event but there are still some details to check.
>> >> >
>> >>
>> >> All right, mark refcount sound like the proper solution.
>> >
>> > Except it doesn't quite work. We can pin the current marks by a refcount
>> > but they can still be removed from the list so after we regain srcu lock,
>> > we are not sure their ->next pointers still point to still allocated marks
>> > :-| Sadly I realized this only after implementing all this.
>>
>> Hmm, how about this: when removing mark from inode, drop refcount.  If
>> refcount is zero can remove from list.  Otherwise mark the mark "dead"
>> and leave it on the list.
>>
>> And fsnotify can just skip dead marks.
>
> I had this idea as well and when trying to implement this, I've stumbled
> over some problems. I think the biggest problem was that destruction of a
> notification mark is relatively complex operation (doing iput() for
> example) and quite a few places dropping mark references are in a context
> where this can cause problems. Also I don't want to defer iput() to a
> workqueue as that will have unexpected consequences such as unlinked
> watched inode lingering in the system (possibly colliding with umount
> etc.).

Okay, but all we need from the deleted mark is the ->next pointer, no?
 So everything else related to destruction can be done.

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1537619

On Fri 02-12-16 12:02:33, Miklos Szeredi wrote:
> On Fri, Dec 2, 2016 at 11:48 AM, Jan Kara <jack@suse.cz> wrote:
> > On Fri 02-12-16 09:26:51, Miklos Szeredi wrote:
> >> On Thu, Nov 10, 2016 at 8:46 PM, Jan Kara <jack@suse.cz> wrote:
> >> > On Wed 09-11-16 20:26:16, Amir Goldstein wrote:
> >> >> On Wed, Nov 9, 2016 at 1:10 PM, Jan Kara <jack@suse.cz> wrote:
> >>
> >> >> > And this does not work as well... Fanotify must notify groups by their
> >> >> > priority so you cannot arbitrarily reorder ordering in which groups get
> >> >> > notified. I'm currently pondering on using mark refcount to pin it when
> >> >> > processing permission event but there are still some details to check.
> >> >> >
> >> >>
> >> >> All right, mark refcount sound like the proper solution.
> >> >
> >> > Except it doesn't quite work. We can pin the current marks by a refcount
> >> > but they can still be removed from the list so after we regain srcu lock,
> >> > we are not sure their ->next pointers still point to still allocated marks
> >> > :-| Sadly I realized this only after implementing all this.
> >>
> >> Hmm, how about this: when removing mark from inode, drop refcount.  If
> >> refcount is zero can remove from list.  Otherwise mark the mark "dead"
> >> and leave it on the list.
> >>
> >> And fsnotify can just skip dead marks.
> >
> > I had this idea as well and when trying to implement this, I've stumbled
> > over some problems. I think the biggest problem was that destruction of a
> > notification mark is relatively complex operation (doing iput() for
> > example) and quite a few places dropping mark references are in a context
> > where this can cause problems. Also I don't want to defer iput() to a
> > workqueue as that will have unexpected consequences such as unlinked
> > watched inode lingering in the system (possibly colliding with umount
> > etc.).
> 
> Okay, but all we need from the deleted mark is the ->next pointer, no?
>  So everything else related to destruction can be done.

Yes, all we need for continuing the iteration itself is a ->next pointer and
possibly a ->group pointer. However the list itself is starting at the
inode->i_fsnotify_marks and the removal of the mark from the list needs to
be protected by inode->i_lock so we do need inode reference while the mark
is still attached to the inode list. So more plausible looks to wait while
detaching mark from the object for all responses for that mark.

Another possibility would be to create separate object for the head of the
list of fsnotify marks (containing also the lock for the list). That would
allow us to detach list of marks from the object and thus we should not
have issues with conflicting lifetime needs... And the additional memory
overhead (about 3 longs) is not that big compared to the size of fsnotify
mark (currently 9 longs). We'll see.

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

[toc] | [prev] | [next] | [standalone]

#1534864

On Fri, Dec 2, 2016 at 12:48 PM, Jan Kara <jack@suse.cz> wrote:
> On Fri 02-12-16 09:26:51, Miklos Szeredi wrote:
...
>>
>> Hmm, how about this: when removing mark from inode, drop refcount.  If
>> refcount is zero can remove from list.  Otherwise mark the mark "dead"
>> and leave it on the list.
>>
>> And fsnotify can just skip dead marks.
>
> I had this idea as well and when trying to implement this, I've stumbled
> over some problems. I think the biggest problem was that destruction of a
> notification mark is relatively complex operation (doing iput() for
> example) and quite a few places dropping mark references are in a context
> where this can cause problems. Also I don't want to defer iput() to a
> workqueue as that will have unexpected consequences such as unlinked
> watched inode lingering in the system (possibly colliding with umount
> etc.).
>

I am wondering out loud if we are trying to solve a real problem or a made
up test case. I wonder if Miklos' test program truly represents the original
bug report. I am asking because fanotify permission events are usually
associated with system security software and it usually makes sense on
a vfsmount_mark and not an inode_mark.

Maybe the break even solution is not to split destroy lists per group priority,
but to split destroy lists by inode marks and vfsmount marks
and also keep 2 separate lists per group.

I am only asking this because you mentioned iput as a thorn in the solution.
Since vfsmount mark does not pin the mount, nor hold an elevated reference,
perhaps dealing with simpler destruction of vfsmount marks can solve the
problem for "rogue fanotify permission mount watch" and maybe that is
enough for all practical matters?

Amir.

[toc] | [prev] | [next] | [standalone]

#1534874

On Fri, Dec 2, 2016 at 1:41 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 12:48 PM, Jan Kara <jack@suse.cz> wrote:
>> On Fri 02-12-16 09:26:51, Miklos Szeredi wrote:
> ...
>>>
>>> Hmm, how about this: when removing mark from inode, drop refcount.  If
>>> refcount is zero can remove from list.  Otherwise mark the mark "dead"
>>> and leave it on the list.
>>>
>>> And fsnotify can just skip dead marks.
>>
>> I had this idea as well and when trying to implement this, I've stumbled
>> over some problems. I think the biggest problem was that destruction of a
>> notification mark is relatively complex operation (doing iput() for
>> example) and quite a few places dropping mark references are in a context
>> where this can cause problems. Also I don't want to defer iput() to a
>> workqueue as that will have unexpected consequences such as unlinked
>> watched inode lingering in the system (possibly colliding with umount
>> etc.).
>>
>
> I am wondering out loud if we are trying to solve a real problem or a made
> up test case. I wonder if Miklos' test program truly represents the original
> bug report. I am asking because fanotify permission events are usually
> associated with system security software and it usually makes sense on
> a vfsmount_mark and not an inode_mark.
>
> Maybe the break even solution is not to split destroy lists per group priority,
> but to split destroy lists by inode marks and vfsmount marks
> and also keep 2 separate lists per group.
>
> I am only asking this because you mentioned iput as a thorn in the solution.
> Since vfsmount mark does not pin the mount, nor hold an elevated reference,
> perhaps dealing with simpler destruction of vfsmount marks can solve the
> problem for "rogue fanotify permission mount watch" and maybe that is
> enough for all practical matters?
>

And before you comment about the need to merge the inode and vfsmount
lists by priority I'll suggest:

- Check if head of inode mark list is priority 0
  If it is, there is no need to merge the lists:
-- first iterate vfsmount list with vfsmount mark srcu
-- then iterate inode list with inode mark srcu
--- if high priority inode mark is found on the list we can either skip it or
     process it out of priority order, because it was just added, so we could
     have missed it anyway

If inode list head is high priority then resort to old problem, as I said,
this is supposed to be a break even solution for practical use cases.

Amir.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web