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