From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [Bugme-new] [Bug 12754] New: inotify doesn't free memory allocated to watches Date: Wed, 25 Feb 2009 03:13:04 +0000 Message-ID: <20090225031304.GL28946@ZenIV.linux.org.uk> References: <20090224130523.f6c4c450.akpm@linux-foundation.org> <20090224213800.GI28946@ZenIV.linux.org.uk> <20090224232337.GJ28946@ZenIV.linux.org.uk> <20090224154023.c003c35c.akpm@linux-foundation.org> <20090225002833.GK28946@ZenIV.linux.org.uk> <20090225023850.GA6269@unused.rdu.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Morton , russell@rickstewart.com, bugme-daemon@bugzilla.kernel.org, linux-fsdevel@vger.kernel.org, john@johnmccutchan.com, rlove@rlove.org To: Josef Bacik Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:48912 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753985AbZBYDNZ (ORCPT ); Tue, 24 Feb 2009 22:13:25 -0500 Content-Disposition: inline In-Reply-To: <20090225023850.GA6269@unused.rdu.redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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