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

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

[Tvrtko Ursulin]

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@sophos.com>
---
 fs/notify/fsnotify.c             |   25 +++++++++++++++++--------
 include/linux/fsnotify_backend.h |    4 ++++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 3680242..37fd072 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,18 +275,27 @@ 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);
+               }
+
+               /* If a listener denied on a permission hook 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. */
+               if (unlikely((res == -EPERM) && (mask & FS_ALL_PERM_EVENTS))) {
+                       mask &= ~FS_ALL_PERM_EVENTS;
+                       ret = res;
                }

                if (inode_group)
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)
+
 #define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
                             FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \
                             FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \

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

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

[Tvrtko Ursulin]

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.

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))) {
+                       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)
+
 #define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
                             FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \
                             FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \

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

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

[Andreas Gruenbacher]

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.

> 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.

> +                       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.

Thanks,
Andreas

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

[Tvrtko Ursulin]

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.

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

[Eric Paris]

On Fri, 2010-10-08 at 17:11 +0100, Tvrtko Ursulin wrote:
> On Friday 08 Oct 2010 14:00:40 Andreas Gruenbacher wrote:
> > Tvrtko,
> >
> > On Wednesday 08 September 2010 10:24:04 Tvrtko Ursulin wrote:

> > >                 }
> > >
> > > +#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?

No, you'll never have a PERM_EVENT and something else at the same time.
If it's a PERM_EVENT denial we wouldn't really want to even notify
others, since the operation won't have actually taken place (perm event
denials should be passed all the way back up the stack)

As to ENOMEM that's a little harder I guess.  We don't have to stop just
because inotify got ENOMEM, but we should stop if PERM_EVENTS got an
ENOMEM.

I'd really rather not put CONFIG_FANOTIFY_ACCESS_PERMISSIONS in this
file.  fsnotify.c should be notifier agnostic.

How about just:

if (unlikely(ret && (mask & ALL_PERM)))
	return ret

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

[Andreas Gruenbacher]

On Friday 08 October 2010 18:11:23 Tvrtko Ursulin wrote:
> 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.

Much better, thanks.

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

My point, exactly.

Thanks,
Andreas

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

[Tvrtko Ursulin]

On Friday 08 Oct 2010 17:51:00 Eric Paris wrote:
> On Fri, 2010-10-08 at 17:11 +0100, Tvrtko Ursulin wrote:
> > On Friday 08 Oct 2010 14:00:40 Andreas Gruenbacher wrote:
> > > Tvrtko,
> > >
> > > On Wednesday 08 September 2010 10:24:04 Tvrtko Ursulin wrote:
> > > >                 }
> > > >
> > > > +#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?
>
> No, you'll never have a PERM_EVENT and something else at the same time.
> If it's a PERM_EVENT denial we wouldn't really want to even notify
> others, since the operation won't have actually taken place (perm event
> denials should be passed all the way back up the stack)

Ok, I wasn't sure if event merging could somehow join more events on one
object into one event here.

> As to ENOMEM that's a little harder I guess.  We don't have to stop just
> because inotify got ENOMEM, but we should stop if PERM_EVENTS got an
> ENOMEM.
>
> I'd really rather not put CONFIG_FANOTIFY_ACCESS_PERMISSIONS in this
> file.  fsnotify.c should be notifier agnostic.
>
> How about just:
>
> if (unlikely(ret && (mask & ALL_PERM)))
>       return ret

Looks like it would work and on more thinking I also agree with the logic to
stop the perm chain on ENOMEM.

Tvrtko

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