* Re: Possible memory leak via inotify_add_watch
2009-07-06 22:03 Possible memory leak via inotify_add_watch Catalin Marinas
@ 2009-07-06 22:59 ` Eric Paris
2009-07-07 2:09 ` Eric Paris
1 sibling, 0 replies; 7+ messages in thread
From: Eric Paris @ 2009-07-06 22:59 UTC (permalink / raw)
To: Catalin Marinas; +Cc: Linux Kernel Mailing List
On Mon, 2009-07-06 at 23:03 +0100, Catalin Marinas wrote:
> Hi Eric,
>
> I'm getting a few kmemleak reports like the one below (it may as well
> be just a false positive). All of these allocations happened during
> udevd.
Ok, I'll take a look. I know it highly unlikely but possible to leak in
sys_inotify_add_watch on some of the error paths. Seeing as how you
object has a mask it was at some point actually attached to an inode and
was removed. Since free_i_list is still unused that means it was
removed by an explicit request from inotify (inotify_rm_watch I guess),
not from the inode disappearing. I'll try to run it down exactly
tonight.
-Eric
> unreferenced object 0xc399fd80 (size 84):
> comm "udevd", pid 879, jiffies 4294897228
> backtrace:
> [<c01e0c3a>] create_object+0xfa/0x250
> [<c01e1e7d>] kmemleak_alloc+0x5d/0x70
> [<c01dac1b>] kmem_cache_alloc+0x14b/0x190
> [<c0213800>] sys_inotify_add_watch+0xc0/0x2a0
> [<c010300c>] sysenter_do_call+0x12/0x38
> [<ffffffff>] 0xffffffff
>
> Printing this object with gdb on /proc/kcore shows:
>
> (gdb) print {struct inotify_inode_mark_entry}0xc399fd80
> $2 = {fsn_entry = {mask = 134250504, refcnt = {counter = 1}, inode = 0x0,
> group = 0x0, i_list = {next = 0x0, pprev = 0x0}, g_list = {
> next = 0xc399fd98, prev = 0xc399fd98}, lock = {raw_lock = {
> slock = 1028}, magic = 3735899821, owner_cpu = 4294967295,
> owner = 0xffffffff, dep_map = {key = 0xc0d3e59c, class_cache = 0x0,
> name = 0xc068413d "&entry->lock"}}, free_i_list = {next = 0x6b6b6b6b,
> prev = 0x6b6b6b6b}, free_g_list = {next = 0x6b6b6b6b,
> prev = 0x6b6b6b6b}, free_mark = 0xc0213720 <inotify_free_mark>}, wd = 28}
>
> It seems that is was freed via fsnotify_destroy_mark_by_entry() since
> group and inode members are NULL and it was removed from any list. The
> fsn_entry.refcnt, however, is still 1. Kmemleak cannot find any
> pointer to this object (though it doesn't track alloc_pages memory
> blocks).
>
> Thanks.
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Possible memory leak via inotify_add_watch
2009-07-06 22:03 Possible memory leak via inotify_add_watch Catalin Marinas
2009-07-06 22:59 ` Eric Paris
@ 2009-07-07 2:09 ` Eric Paris
2009-07-07 13:03 ` Catalin Marinas
1 sibling, 1 reply; 7+ messages in thread
From: Eric Paris @ 2009-07-07 2:09 UTC (permalink / raw)
To: Catalin Marinas; +Cc: Linux Kernel Mailing List
On Mon, 2009-07-06 at 23:03 +0100, Catalin Marinas wrote:
> Hi Eric,
>
> I'm getting a few kmemleak reports like the one below (it may as well
> be just a false positive). All of these allocations happened during
> udevd.
Any chance you could give this a shot and see if it fixes it up for you?
-Eric
commit 27d13f5ef42fc4c976eae8318c860ea88c2076aa
Author: Eric Paris <eparis@redhat.com>
Date: Mon Jul 6 22:07:19 2009 -0400
inotify: do not leak inode marks on inotify_rm_watch
inotify had a ref cnt problem and inode marks were being leaked when they
were destroyed using inotify_rm_watch.
Signed-off-by: Eric Paris <eparis@redhat.com>
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 2808e30..3ba2e1a 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -365,6 +365,17 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
return error;
}
+static void inotify_remove_from_idr(struct fsnotify_group *group,
+ struct inotify_inode_mark_entry *ientry)
+{
+ struct idr *idr;
+
+ spin_lock(&group->inotify_data.idr_lock);
+ idr = &group->inotify_data.idr;
+ idr_remove(idr, ientry->wd);
+ spin_unlock(&group->inotify_data.idr_lock);
+ ientry->wd = -1;
+}
/*
* Send IN_IGNORED for this wd, remove this wd from the idr, and drop the
* internal reference help on the mark because it is in the idr.
@@ -375,7 +386,6 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark_entry *entry,
struct inotify_inode_mark_entry *ientry;
struct inotify_event_private_data *event_priv;
struct fsnotify_event_private_data *fsn_event_priv;
- struct idr *idr;
ientry = container_of(entry, struct inotify_inode_mark_entry, fsn_entry);
@@ -397,10 +407,7 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark_entry *entry,
skip_send_ignore:
/* remove this entry from the idr */
- spin_lock(&group->inotify_data.idr_lock);
- idr = &group->inotify_data.idr;
- idr_remove(idr, ientry->wd);
- spin_unlock(&group->inotify_data.idr_lock);
+ inotify_remove_from_idr(group, ientry);
/* removed from idr, drop that reference */
fsnotify_put_mark(entry);
@@ -422,6 +429,7 @@ static int inotify_update_watch(struct fsnotify_group *group, struct inode *inod
{
struct fsnotify_mark_entry *entry = NULL;
struct inotify_inode_mark_entry *ientry;
+ struct inotify_inode_mark_entry *tmp_ientry;
int ret = 0;
int add = (arg & IN_MASK_ADD);
__u32 mask;
@@ -432,54 +440,65 @@ static int inotify_update_watch(struct fsnotify_group *group, struct inode *inod
if (unlikely(!mask))
return -EINVAL;
- ientry = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
- if (unlikely(!ientry))
+ tmp_ientry = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
+ if (unlikely(!tmp_ientry))
return -ENOMEM;
/* we set the mask at the end after attaching it */
- fsnotify_init_mark(&ientry->fsn_entry, inotify_free_mark);
- ientry->wd = 0;
+ fsnotify_init_mark(&tmp_ientry->fsn_entry, inotify_free_mark);
+ tmp_ientry->wd = -1;
find_entry:
spin_lock(&inode->i_lock);
entry = fsnotify_find_mark_entry(group, inode);
spin_unlock(&inode->i_lock);
if (entry) {
- kmem_cache_free(inotify_inode_mark_cachep, ientry);
ientry = container_of(entry, struct inotify_inode_mark_entry, fsn_entry);
} else {
- if (atomic_read(&group->inotify_data.user->inotify_watches) >= inotify_max_user_watches) {
- ret = -ENOSPC;
- goto out_err;
- }
-
- ret = fsnotify_add_mark(&ientry->fsn_entry, group, inode, 0);
- if (ret == -EEXIST)
- goto find_entry;
- else if (ret)
+ ret = -ENOSPC;
+ if (atomic_read(&group->inotify_data.user->inotify_watches) >= inotify_max_user_watches)
goto out_err;
-
- entry = &ientry->fsn_entry;
retry:
ret = -ENOMEM;
if (unlikely(!idr_pre_get(&group->inotify_data.idr, GFP_KERNEL)))
goto out_err;
spin_lock(&group->inotify_data.idr_lock);
- /* if entry is added to the idr we keep the reference obtained
- * through fsnotify_mark_add. remember to drop this reference
- * when entry is removed from idr */
- ret = idr_get_new_above(&group->inotify_data.idr, entry,
- ++group->inotify_data.last_wd,
- &ientry->wd);
+ ret = idr_get_new_above(&group->inotify_data.idr, &tmp_ientry->fsn_entry,
+ group->inotify_data.last_wd,
+ &tmp_ientry->wd);
+
spin_unlock(&group->inotify_data.idr_lock);
if (ret) {
if (ret == -EAGAIN)
goto retry;
goto out_err;
}
+
+ ret = fsnotify_add_mark(&tmp_ientry->fsn_entry, group, inode, 0);
+ if (ret) {
+ inotify_remove_from_idr(group, tmp_ientry);
+ if (ret == -EEXIST)
+ goto find_entry;
+ goto out_err;
+ }
+
+ /* tmp_ientry has been added to the inode, so we are all set up.
+ * now we just need to make sure tmp_ientry doesn't get freed and
+ * we need to set up entry and ientry so the generic code can
+ * do its thing. */
+ ientry = tmp_ientry;
+ entry = &ientry->fsn_entry;
+ tmp_ientry = NULL;
atomic_inc(&group->inotify_data.user->inotify_watches);
+
+ group->inotify_data.last_wd = ientry->wd;
+
+ /* since this mark is on the idr, we should hold a reference */
+ fsnotify_get_mark(entry);
}
+ ret = ientry->wd;
+
spin_lock(&entry->lock);
old_mask = entry->mask;
@@ -510,14 +529,18 @@ retry:
fsnotify_recalc_group_mask(group);
}
- return ientry->wd;
-
+ /* this either matches fsnotify_find_mark_entry, or init_mark_entry
+ * depending on which path we took... */
+ fsnotify_put_mark(entry);
out_err:
- /* see this isn't supposed to happen, just kill the watch */
- if (entry) {
- fsnotify_destroy_mark_by_entry(entry);
- fsnotify_put_mark(entry);
+ /* could be an error, could be that we found an existing mark */
+ if (tmp_ientry) {
+ /* on the idr but didn't make it on the inode */
+ if (tmp_ientry->wd != -1)
+ inotify_remove_from_idr(group, tmp_ientry);
+ kmem_cache_free(inotify_inode_mark_cachep, tmp_ientry);
}
+
return ret;
}
^ permalink raw reply [flat|nested] 7+ messages in thread