inotify: fix error paths in inotify_update_watch

inotify_update_watch could leave things in a horrid state on a number of
error paths.  We could try to remove idr entries that didn't exist, we
could send an IN_IGNORED to userspace for watches that don't exist, and a
bit of other stupidity.  Clean these up by doing the idr addition before we
put the mark on the inode since we can clean that up on error and getting
off the inode's mark list is hard.

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 aff4214..726118a 100644 (file)
--- 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);
@@ -420,6 +427,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;
@@ -430,50 +438,60 @@ 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);
-               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);
-               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);
+               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);
 
+               /* update the idr hint */
+               group->inotify_data.last_wd = ientry->wd;
+
                /* we put the mark on the idr, take a reference */
                fsnotify_get_mark(entry);
        }
@@ -514,14 +532,15 @@ retry:
         * depending on which path we took... */
        fsnotify_put_mark(entry);
 
-       return ret;
-
 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;
 }