* [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
@ 2010-09-07 15:08 Tvrtko Ursulin
2010-09-08 8:24 ` Tvrtko Ursulin
0 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2010-09-07 15:08 UTC (permalink / raw)
To: Eric Paris; +Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-09-07 15:08 [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions Tvrtko Ursulin
@ 2010-09-08 8:24 ` Tvrtko Ursulin
2010-10-08 13:00 ` Andreas Gruenbacher
0 siblings, 1 reply; 7+ messages in thread
From: Tvrtko Ursulin @ 2010-09-08 8:24 UTC (permalink / raw)
To: Eric Paris; +Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-09-08 8:24 ` Tvrtko Ursulin
@ 2010-10-08 13:00 ` Andreas Gruenbacher
2010-10-08 16:11 ` Tvrtko Ursulin
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Gruenbacher @ 2010-10-08 13:00 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: Eric Paris, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-10-08 13:00 ` Andreas Gruenbacher
@ 2010-10-08 16:11 ` Tvrtko Ursulin
2010-10-08 16:51 ` Eric Paris
2010-10-08 21:05 ` Andreas Gruenbacher
0 siblings, 2 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2010-10-08 16:11 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Eric Paris, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-10-08 16:11 ` Tvrtko Ursulin
@ 2010-10-08 16:51 ` Eric Paris
2010-10-18 11:05 ` Tvrtko Ursulin
2010-10-08 21:05 ` Andreas Gruenbacher
1 sibling, 1 reply; 7+ messages in thread
From: Eric Paris @ 2010-10-08 16:51 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: Andreas Gruenbacher, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-10-08 16:51 ` Eric Paris
@ 2010-10-18 11:05 ` Tvrtko Ursulin
0 siblings, 0 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2010-10-18 11:05 UTC (permalink / raw)
To: Eric Paris
Cc: Andreas Gruenbacher, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions
2010-10-08 16:11 ` Tvrtko Ursulin
2010-10-08 16:51 ` Eric Paris
@ 2010-10-08 21:05 ` Andreas Gruenbacher
1 sibling, 0 replies; 7+ messages in thread
From: Andreas Gruenbacher @ 2010-10-08 21:05 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: Eric Paris, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-10-18 11:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-07 15:08 [BUG][PATCH][2.6.36-rc3] fanotify: Do not ignore result of permission decisions Tvrtko Ursulin
2010-09-08 8:24 ` Tvrtko Ursulin
2010-10-08 13:00 ` Andreas Gruenbacher
2010-10-08 16:11 ` Tvrtko Ursulin
2010-10-08 16:51 ` Eric Paris
2010-10-18 11:05 ` Tvrtko Ursulin
2010-10-08 21:05 ` Andreas Gruenbacher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).