Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions

[Tvrtko Ursulin <tvrtko.ursulin@sophos.com>]

On Friday 08 Oct 2010 14:00:40 Andreas Gruenbacher wrote:
> Tvrtko,
>
> On Wednesday 08 September 2010 10:24:04 Tvrtko Ursulin wrote:
> > Improved version of the fix which does not include the check
> > when permission events are not enabled in configuration and
> > stops processing if no interesting events remain.
> >
> > Current code ignores access replies to permission decisions so
> > fix it in a way which will allow all listeners to still receive
> > non permission events.
>
> I agree with the patch (see comments below), but this explanation is close
> to incomprehensible and not good as a commit message.

How about this:

Current code incorrectly ignores responses to permission decisions. When a
single deny response has been received record it and do not send more
permission events. However still send non-permission events to other clients.

> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@sophos.com>
> > ---
> >
> >  fs/notify/fsnotify.c             |   32 ++++++++++++++++++++++++--------
> >  include/linux/fsnotify_backend.h |    4 ++++
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index 3680242..2350a53 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -224,7 +224,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, void
> > *data, int data_is,
> >
> >         struct fsnotify_group *inode_group, *vfsmount_group;
> >         struct fsnotify_event *event = NULL;
> >         struct vfsmount *mnt;
> >
> > -       int idx, ret = 0;
> > +       int idx, ret = 0, res;
> >
> >         /* global tests shouldn't care about events on child only the
> >         specific event */ __u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
> >
> > @@ -275,20 +275,36 @@ int fsnotify(struct inode *to_tell, __u32 mask,
> > void *data, int data_is,
> >
> >                 if (inode_group > vfsmount_group) {
> >
> >                         /* handle inode */
> >
> > -                       send_to_group(to_tell, NULL, inode_mark, NULL,
> > mask, data, -                                     data_is, cookie,
> > file_name, &event); +                       res = send_to_group(to_tell,
> > NULL, inode_mark, NULL, mask, data, +
> >        data_is, cookie, file_name, &event);
> >
> >                         /* we didn't use the vfsmount_mark */
> >                         vfsmount_group = NULL;
> >
> >                 } else if (vfsmount_group > inode_group) {
> >
> > -                       send_to_group(to_tell, mnt, NULL, vfsmount_mark,
> > mask, data, -                                     data_is, cookie,
> > file_name, &event); +                       res = send_to_group(to_tell,
> > mnt, NULL, vfsmount_mark, mask, data, +
> >          data_is, cookie, file_name, &event);
> >
> >                         inode_group = NULL;
> >
> >                 } else {
> >
> > -                       send_to_group(to_tell, mnt, inode_mark,
> > vfsmount_mark, -                                     mask, data,
> > data_is, cookie, file_name, -
> > &event);
> > +                       res = send_to_group(to_tell, mnt, inode_mark,
> > vfsmount_mark, +                                           mask, data,
> > data_is, cookie, file_name, +
> > &event);
> >
> >                 }
> >
> > +#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> > +               /* If a listener denied on a permission event we will
> > remember the reason +                  and run the rest with a
> > non-permission mask only. This allows other +                  listeners
> > to receive non-permission notifications but we do not care +
> >      about further permission checks and want to deny this event. */ +
> >             if (unlikely((res == -EPERM) && (mask &
> > FS_ALL_PERM_EVENTS))) {
>
> We should probably stop processing here whenever res != 0, for any mask.
> (The fanotify and inotify handle_event callbacks can return -ENOMEM right
> now, but this doesn't seem very useful and should probably be fixed:
> fsnotify has no way of doing anything about -ENOMEM.

I wasn't 100% sure of how fanotify works, or in other words can there be more
events in this mask after perm events are removed. If it can then we should
send them to other listeners. Eric?

Also if ENOMEM, why not try sending to other listeners?

> > +                       mask &= ~FS_ALL_PERM_EVENTS;
> > +                       ret = res;
> > +                       /* Stop processing if no interesting events
> > remains. */ +                       if (!(mask & (ALL_FSNOTIFY_EVENTS &
> > ~FS_EVENT_ON_CHILD))) +                               break;
> > +               }
> > +#else
> > +               (void)res;
> > +#endif
> > +
> >
> >                 if (inode_group)
> >
> >                         inode_node = srcu_dereference(inode_node->next,
> >
> >                                                       &fsnotify_mark_srcu
> >                                                       );
> >
> > diff --git a/include/linux/fsnotify_backend.h
> > b/include/linux/fsnotify_backend.h index e40190d..c8aea03 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -64,6 +64,10 @@
> >
> >  #define FS_MOVE                        (FS_MOVED_FROM | FS_MOVED_TO)
> >
> > +/* All events which require a permission response from userspace */
> > +#define FS_ALL_PERM_EVENTS (FS_OPEN_PERM |\
> > +                           FS_ACCESS_PERM)
>
> When dealing with the result of ->handle_event as I propose, this
> definition isn't needed.

Yep, I am just unsure what is overall right thing to do. Lets see what Eric
says..

Tvrtko

Sophos Plc, The Pentagon, Abingdon Science Park, Abingdon, OX14 3YP, United Kingdom.
Company Reg No 2096520. VAT Reg No GB 348 3873 20.