2010-11-09 18:20:41

by Lino Sanfilippo

Subject: [PATCH] fanotify: Do check against max_marks and increase number of group marks atomically

The number of group marks is checked against max_marks and increased afterwards
in a non atomic way. This may result in 2 or more processes passing the check
at the same time and increasing the number of group marks above the max_marks
limit afterwards.
With this patch the check against max_marks is done in fsnotify_add_mark(),
after the group lock has been aquired to ensure that concurrent processes
cant exceed the group marks limit.

Signed-off-by: Lino Sanfilippo <[email protected]>
---
This patch applies against branch 'origin/for-next' from
git.infradead.org/users/eparis/notify.git and depends on the patch "correct
broken ref counting in case adding a mark failed" i have sent before.

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 090790a..05095e5 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -609,9 +609,6 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,

fsn_mark = fsnotify_find_vfsmount_mark(group, mnt);
if (!fsn_mark) {
- if (atomic_read(&group->num_marks) > group->fanotify_data.max_marks)
- return -ENOSPC;
-
fsn_mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
if (!fsn_mark)
return -ENOMEM;
@@ -652,9 +649,6 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,

fsn_mark = fsnotify_find_inode_mark(group, inode);
if (!fsn_mark) {
- if (atomic_read(&group->num_marks) > group->fanotify_data.max_marks)
- return -ENOSPC;
-
fsn_mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);
if (!fsn_mark)
return -ENOMEM;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index efe16e4..12d09f8 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -220,21 +220,27 @@ int fsnotify_add_mark(struct fsnotify_mark *mark,
spin_lock(&mark->lock);
spin_lock(&group->mark_lock);

+ fsnotify_get_mark(mark); /* for i_list and g_list */
+
+ if (atomic_read(&group->num_marks) > group->fanotify_data.max_marks) {
+ ret = -ENOSPC;
+ goto err;
+ }
+
mark->flags |= FSNOTIFY_MARK_FLAG_ALIVE;

mark->group = group;
list_add(&mark->g_list, &group->marks_list);
atomic_inc(&group->num_marks);
- fsnotify_get_mark(mark); /* for i_list and g_list */

if (inode) {
ret = fsnotify_add_inode_mark(mark, group, inode, allow_dups);
if (ret)
- goto err;
+ goto err2;
} else if (mnt) {
ret = fsnotify_add_vfsmount_mark(mark, group, mnt, allow_dups);
if (ret)
- goto err;
+ goto err2;
} else {
BUG();
}
@@ -250,12 +256,12 @@ int fsnotify_add_mark(struct fsnotify_mark *mark,
__fsnotify_update_child_dentry_flags(inode);

return ret;
-err:
+err2:
mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
list_del_init(&mark->g_list);
mark->group = NULL;
atomic_dec(&group->num_marks);
-
+err:
spin_unlock(&group->mark_lock);
spin_unlock(&mark->lock);

2010-11-09 20:21:53

by Eric Paris

Subject: Re: [PATCH] fanotify: Do check against max_marks and increase number of group marks atomically

On Tue, 2010-11-09 at 19:19 +0100, Lino Sanfilippo wrote:
> The number of group marks is checked against max_marks and increased afterwards
> in a non atomic way. This may result in 2 or more processes passing the check
> at the same time and increasing the number of group marks above the max_marks
> limit afterwards.
> With this patch the check against max_marks is done in fsnotify_add_mark(),
> after the group lock has been aquired to ensure that concurrent processes
> cant exceed the group marks limit.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>

applied to http://git.infradead.org/users/eparis/notify.git/ #for-next

-Eric