Re: [Bugme-new] [Bug 12754] New: inotify doesn't free memory allocated to watches

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Feb 24, 2009 at 09:38:50PM -0500, Josef Bacik wrote:

> > Actually, looking at inotify_user.c, we seem to be doing something rather
> > fishy.  Look: event gets triggered, we pick the watch, get inotify_device
> > (inotify_user-specific stuff) from it, grap mutex on it (dev->ev_mutex)
> > and drop reference to inotify_watch.  Which happily triggers ->destroy_watch,
> > which does put_inotify_dev().  Which is
> >         if (atomic_dec_and_test(&dev->count)) {
> >                 atomic_dec(&dev->user->inotify_devs);
> >                 free_uid(dev->user);
> >                 kfree(dev);
> >         }
> > What's to stop that from happening when we'd been holding the last reference
> > to that sucker?  kfree() while holding a mutex inside the structure being
> > freed is not nice...
> 
> That shouldn't happen, we should only be doing the last put on the dev when all
> watches have been removed.  When we do the inotify_init we do the get on the
> dev, and then for every watch theres a pair of get/put for instantiation and
> removal, so we can't free the dev when doing a put for a watch (well obviously
> we can, but we shouldn't be anyway).

Hold on.  Either that sucker can happen after inotify_release() has dropped
its reference or it can not.  In the former case, we are screwed since _we_
are holding the last remaining reference.  In the latter, WTF are we grabbing
references to ->dev at watch creation?

>  If the user is getting back -ENOSPC from
> inotify then it can only mean we've run out of watches

> 
>         if (atomic_read(&dev->user->inotify_watches) >=                       
>                         inotify_max_user_watches)
>                 return -ENOSPC;

... or that idr_pre_get() has failed in inotify.c