[PATCH 0/3] fanotify: Fix list corruption in fanotify_get_response()

[PATCH 0/3] fanotify: Fix list corruption in fanotify_get_response()

[Jan Kara]

Hi,

these patches should fix the corruption in fanotify_get_response() which
Miklos has spotted. Miklos, can you please test them? I have tried
writing my own stress-tester to hit this issue but I was not able to
trigger the bug...

								Honza

[PATCH 1/3] fsnotify: Use enum for return values of fsnotify_add_event()

[Jan Kara]

Currently there are three possible returns from fsnotify_add_event(). We
will be adding a fourth one. Let's change magic numbers to enum to make
things clearer.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify.c        |  7 ++++---
 fs/notify/inotify/inotify_fsnotify.c |  4 ++--
 fs/notify/notification.c             | 20 +++++++++++---------
 include/linux/fsnotify_backend.h     | 14 ++++++++++----
 4 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d2f97ecca6a5..3c6c81e0c2c8 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -192,6 +192,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 				 const unsigned char *file_name, u32 cookie)
 {
 	int ret = 0;
+	enum fsn_add_event_ret ae_ret;
 	struct fanotify_event_info *event;
 	struct fsnotify_event *fsn_event;
 
@@ -218,10 +219,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 		return -ENOMEM;
 
 	fsn_event = &event->fse;
-	ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
-	if (ret) {
+	ae_ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
+	if (ae_ret != AE_INSERTED) {
 		/* Permission events shouldn't be merged */
-		BUG_ON(ret == 1 && mask & FAN_ALL_PERM_EVENTS);
+		BUG_ON(ae_ret == AE_MERGED && mask & FAN_ALL_PERM_EVENTS);
 		/* Our event wasn't used in the end. Free it. */
 		fsnotify_destroy_event(group, fsn_event);
 
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 2cd900c2c737..ac37192c59c5 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -72,7 +72,7 @@ int inotify_handle_event(struct fsnotify_group *group,
 	struct inotify_inode_mark *i_mark;
 	struct inotify_event_info *event;
 	struct fsnotify_event *fsn_event;
-	int ret;
+	enum fsn_add_event_ret ret;
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 
@@ -109,7 +109,7 @@ int inotify_handle_event(struct fsnotify_group *group,
 		strcpy(event->name, file_name);
 
 	ret = fsnotify_add_event(group, fsn_event, inotify_merge);
-	if (ret) {
+	if (ret != AE_INSERTED) {
 		/* Our event wasn't used in the end. Free it. */
 		fsnotify_destroy_event(group, fsn_event);
 	}
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index a95d8e037aeb..12bfd6790fc4 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -84,12 +84,12 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
  * added to the queue, 1 if the event was merged with some other queued event,
  * 2 if the queue of events has overflown.
  */
-int fsnotify_add_event(struct fsnotify_group *group,
-		       struct fsnotify_event *event,
-		       int (*merge)(struct list_head *,
-				    struct fsnotify_event *))
+enum fsn_add_event_ret fsnotify_add_event(
+		struct fsnotify_group *group,
+		struct fsnotify_event *event,
+		int (*merge)(struct list_head *, struct fsnotify_event *))
 {
-	int ret = 0;
+	enum fsn_add_event_ret ret = AE_INSERTED;
 	struct list_head *list = &group->notification_list;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
@@ -97,7 +97,7 @@ int fsnotify_add_event(struct fsnotify_group *group,
 	mutex_lock(&group->notification_mutex);
 
 	if (group->q_len >= group->max_events) {
-		ret = 2;
+		ret = AE_OVERFLOW;
 		/* Queue overflow event only if it isn't already queued */
 		if (!list_empty(&group->overflow_event->list)) {
 			mutex_unlock(&group->notification_mutex);
@@ -108,10 +108,12 @@ int fsnotify_add_event(struct fsnotify_group *group,
 	}
 
 	if (!list_empty(list) && merge) {
-		ret = merge(list, event);
-		if (ret) {
+		int merge_ret;
+
+		merge_ret = merge(list, event);
+		if (merge_ret) {
 			mutex_unlock(&group->notification_mutex);
-			return ret;
+			return AE_MERGED;
 		}
 	}
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 58205f33af02..b948a52ce65f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -299,11 +299,17 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);
 /* Free event from memory */
 extern void fsnotify_destroy_event(struct fsnotify_group *group,
 				   struct fsnotify_event *event);
+/* Return values of fsnotify_add_event() */
+enum fsn_add_event_ret {
+	AE_INSERTED,	/* Event was added in the queue */
+	AE_MERGED,	/* Event was merged with another event, passed event unused */
+	AE_OVERFLOW,	/* Queue overflow, passed event unused */
+};
 /* attach the event to the group notification queue */
-extern int fsnotify_add_event(struct fsnotify_group *group,
-			      struct fsnotify_event *event,
-			      int (*merge)(struct list_head *,
-					   struct fsnotify_event *));
+extern enum fsn_add_event_ret fsnotify_add_event(
+		struct fsnotify_group *group,
+		struct fsnotify_event *event,
+		int (*merge)(struct list_head *, struct fsnotify_event *));
 /* Remove passed event from groups notification queue */
 extern void fsnotify_remove_event(struct fsnotify_group *group, struct fsnotify_event *event);
 /* true if the group notification queue is empty */
-- 
2.6.6

[PATCH 2/3] fsnotify: Add a way to stop queueing events on group shutdown

[Jan Kara]

Implement a function that can be called when a group is being shutdown
to stop queueing new events to the group. Fanotify will use this.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/group.c                | 19 +++++++++++++++++++
 fs/notify/notification.c         |  5 +++++
 include/linux/fsnotify_backend.h |  4 ++++
 3 files changed, 28 insertions(+)

diff --git a/fs/notify/group.c b/fs/notify/group.c
index 3e2dd85be5dd..b47f7cfdcaa4 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -40,6 +40,17 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
 }
 
 /*
+ * Stop queueing new events for this group. Once this function returns
+ * fsnotify_add_event() will not add any new events to the group's queue.
+ */
+void fsnotify_group_stop_queueing(struct fsnotify_group *group)
+{
+	mutex_lock(&group->notification_mutex);
+	group->shutdown = true;
+	mutex_unlock(&group->notification_mutex);
+}
+
+/*
  * Trying to get rid of a group. Remove all marks, flush all events and release
  * the group reference.
  * Note that another thread calling fsnotify_clear_marks_by_group() may still
@@ -47,6 +58,14 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
  */
 void fsnotify_destroy_group(struct fsnotify_group *group)
 {
+	/*
+	 * Stop queueing new events. The code below is careful enough to not
+	 * require this but fanotify needs to stop queuing events even before
+	 * fsnotify_destroy_group() is called and this makes the other callers
+	 * of fsnotify_destroy_group() to see the same behavior.
+	 */
+	fsnotify_group_stop_queueing(group);
+
 	/* clear all inode marks for this group, attach them to destroy_list */
 	fsnotify_detach_group_marks(group);
 
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 12bfd6790fc4..af0497cac06c 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -96,6 +96,11 @@ enum fsn_add_event_ret fsnotify_add_event(
 
 	mutex_lock(&group->notification_mutex);
 
+	if (group->shutdown) {
+		mutex_unlock(&group->notification_mutex);
+		return AE_SHUTDOWN;
+	}
+
 	if (group->q_len >= group->max_events) {
 		ret = AE_OVERFLOW;
 		/* Queue overflow event only if it isn't already queued */
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b948a52ce65f..bcba826a99fc 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -148,6 +148,7 @@ struct fsnotify_group {
 	#define FS_PRIO_1	1 /* fanotify content based access control */
 	#define FS_PRIO_2	2 /* fanotify pre-content access */
 	unsigned int priority;
+	bool shutdown;		/* group is being shut down, don't queue more events */
 
 	/* stores all fastpath marks assoc with this group so they can be cleaned on unregister */
 	struct mutex mark_mutex;	/* protect marks_list */
@@ -292,6 +293,8 @@ extern struct fsnotify_group *fsnotify_alloc_group(const struct fsnotify_ops *op
 extern void fsnotify_get_group(struct fsnotify_group *group);
 /* drop reference on a group from fsnotify_alloc_group */
 extern void fsnotify_put_group(struct fsnotify_group *group);
+/* group destruction begins, stop queuing new events */
+extern void fsnotify_group_stop_queueing(struct fsnotify_group *group);
 /* destroy group */
 extern void fsnotify_destroy_group(struct fsnotify_group *group);
 /* fasync handler function */
@@ -304,6 +307,7 @@ enum fsn_add_event_ret {
 	AE_INSERTED,	/* Event was added in the queue */
 	AE_MERGED,	/* Event was merged with another event, passed event unused */
 	AE_OVERFLOW,	/* Queue overflow, passed event unused */
+	AE_SHUTDOWN,	/* Group is being released, passed event unused */
 };
 /* attach the event to the group notification queue */
 extern enum fsn_add_event_ret fsnotify_add_event(
-- 
2.6.6

[PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Jan Kara]

fanotify_get_response() calls fsnotify_remove_event() when it finds that
group is being released from fanotify_release() (bypass_perm is set).
However the event it removes need not be only in the group's notification
queue but it can have already moved to access_list (userspace read the
event before closing the fanotify instance fd) which is protected by a
different lock. Thus when fsnotify_remove_event() races with
fanotify_release() operating on access_list, the list can get corrupted.

Fix the problem by moving all the logic removing permission events from
the lists to one place - fanotify_release().

Reported-by: Miklos Szeredi <mszeredi@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify.c      | 13 +------------
 fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++------------
 include/linux/fsnotify_backend.h   |  1 -
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 3c6c81e0c2c8..363540df7db9 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -67,18 +67,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	wait_event(group->fanotify_data.access_waitq, event->response ||
-				atomic_read(&group->fanotify_data.bypass_perm));
-
-	if (!event->response) {	/* bypass_perm set */
-		/*
-		 * Event was canceled because group is being destroyed. Remove
-		 * it from group's event list because we are responsible for
-		 * freeing the permission event.
-		 */
-		fsnotify_remove_event(group, &event->fae.fse);
-		return 0;
-	}
+	wait_event(group->fanotify_data.access_waitq, event->response);
 
 	/* userspace responded, convert to something usable */
 	switch (event->response) {
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8e8e6bcd1d43..a64313868d3a 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -358,16 +358,20 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 	struct fanotify_perm_event_info *event, *next;
+	struct fsnotify_event *fsn_event;
 
 	/*
-	 * There may be still new events arriving in the notification queue
-	 * but since userspace cannot use fanotify fd anymore, no event can
-	 * enter or leave access_list by now.
+	 * Stop new events from arriving in the notification queue. since
+	 * userspace cannot use fanotify fd anymore, no event can enter or
+	 * leave access_list by now either.
 	 */
-	spin_lock(&group->fanotify_data.access_lock);
-
-	atomic_inc(&group->fanotify_data.bypass_perm);
+	fsnotify_group_stop_queueing(group);
 
+	/*
+	 * Process all permission events on access_list and notification queue
+	 * and simulate reply from userspace.
+	 */
+	spin_lock(&group->fanotify_data.access_lock);
 	list_for_each_entry_safe(event, next, &group->fanotify_data.access_list,
 				 fae.fse.list) {
 		pr_debug("%s: found group=%p event=%p\n", __func__, group,
@@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 	spin_unlock(&group->fanotify_data.access_lock);
 
 	/*
-	 * Since bypass_perm is set, newly queued events will not wait for
-	 * access response. Wake up the already sleeping ones now.
-	 * synchronize_srcu() in fsnotify_destroy_group() will wait for all
-	 * processes sleeping in fanotify_handle_event() waiting for access
-	 * response and thus also for all permission events to be freed.
+	 * Destroy all non-permission events. For permission events just
+	 * dequeue them and set the response. They will be freed once the
+	 * response is consumed and fanotify_get_response() returns.
 	 */
+	mutex_lock(&group->notification_mutex);
+	while (!fsnotify_notify_queue_is_empty(group)) {
+		fsn_event = fsnotify_remove_first_event(group);
+		if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
+			fsnotify_destroy_event(group, fsn_event);
+		else
+			FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
+	}
+	mutex_unlock(&group->notification_mutex);
+
+	/* Response for all permission events it set, wakeup waiters */
 	wake_up(&group->fanotify_data.access_waitq);
 #endif
 
@@ -755,7 +768,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	spin_lock_init(&group->fanotify_data.access_lock);
 	init_waitqueue_head(&group->fanotify_data.access_waitq);
 	INIT_LIST_HEAD(&group->fanotify_data.access_list);
-	atomic_set(&group->fanotify_data.bypass_perm, 0);
 #endif
 	switch (flags & FAN_ALL_CLASS_BITS) {
 	case FAN_CLASS_NOTIF:
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index bcba826a99fc..7ca04e082002 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -180,7 +180,6 @@ struct fsnotify_group {
 			spinlock_t access_lock;
 			struct list_head access_list;
 			wait_queue_head_t access_waitq;
-			atomic_t bypass_perm;
 #endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
 			int f_flags;
 			unsigned int max_marks;
-- 
2.6.6

Re: [PATCH 1/3] fsnotify: Use enum for return values of fsnotify_add_event()

[Miklos Szeredi]

On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> wrote:
> Currently there are three possible returns from fsnotify_add_event(). We
> will be adding a fourth one. Let's change magic numbers to enum to make
> things clearer.

This cleanup should go last, otherwise it'll just make backporting the
fix harder.

Also I don't really see the value of having a big enum with 3 of the
values having the same meaning.   Yeah, there's that BUG_ON for having
merged a permission event.  But it checks the very obvious condition
at the top of fanotify_merge() with the big comment, not very
interesting.

So I'd kill the BUG_ON, drop the merged/overflowed/shutdown
distinction and return a two way value (doing it with an enum still
makes the code more readable, though).

Thanks,
Miklos

>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify.c        |  7 ++++---
>  fs/notify/inotify/inotify_fsnotify.c |  4 ++--
>  fs/notify/notification.c             | 20 +++++++++++---------
>  include/linux/fsnotify_backend.h     | 14 ++++++++++----
>  4 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index d2f97ecca6a5..3c6c81e0c2c8 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -192,6 +192,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>                                  const unsigned char *file_name, u32 cookie)
>  {
>         int ret = 0;
> +       enum fsn_add_event_ret ae_ret;
>         struct fanotify_event_info *event;
>         struct fsnotify_event *fsn_event;
>
> @@ -218,10 +219,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>                 return -ENOMEM;
>
>         fsn_event = &event->fse;
> -       ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
> -       if (ret) {
> +       ae_ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
> +       if (ae_ret != AE_INSERTED) {
>                 /* Permission events shouldn't be merged */
> -               BUG_ON(ret == 1 && mask & FAN_ALL_PERM_EVENTS);
> +               BUG_ON(ae_ret == AE_MERGED && mask & FAN_ALL_PERM_EVENTS);
>                 /* Our event wasn't used in the end. Free it. */
>                 fsnotify_destroy_event(group, fsn_event);
>
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 2cd900c2c737..ac37192c59c5 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -72,7 +72,7 @@ int inotify_handle_event(struct fsnotify_group *group,
>         struct inotify_inode_mark *i_mark;
>         struct inotify_event_info *event;
>         struct fsnotify_event *fsn_event;
> -       int ret;
> +       enum fsn_add_event_ret ret;
>         int len = 0;
>         int alloc_len = sizeof(struct inotify_event_info);
>
> @@ -109,7 +109,7 @@ int inotify_handle_event(struct fsnotify_group *group,
>                 strcpy(event->name, file_name);
>
>         ret = fsnotify_add_event(group, fsn_event, inotify_merge);
> -       if (ret) {
> +       if (ret != AE_INSERTED) {
>                 /* Our event wasn't used in the end. Free it. */
>                 fsnotify_destroy_event(group, fsn_event);
>         }
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index a95d8e037aeb..12bfd6790fc4 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -84,12 +84,12 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
>   * added to the queue, 1 if the event was merged with some other queued event,
>   * 2 if the queue of events has overflown.
>   */
> -int fsnotify_add_event(struct fsnotify_group *group,
> -                      struct fsnotify_event *event,
> -                      int (*merge)(struct list_head *,
> -                                   struct fsnotify_event *))
> +enum fsn_add_event_ret fsnotify_add_event(
> +               struct fsnotify_group *group,
> +               struct fsnotify_event *event,
> +               int (*merge)(struct list_head *, struct fsnotify_event *))
>  {
> -       int ret = 0;
> +       enum fsn_add_event_ret ret = AE_INSERTED;
>         struct list_head *list = &group->notification_list;
>
>         pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> @@ -97,7 +97,7 @@ int fsnotify_add_event(struct fsnotify_group *group,
>         mutex_lock(&group->notification_mutex);
>
>         if (group->q_len >= group->max_events) {
> -               ret = 2;
> +               ret = AE_OVERFLOW;
>                 /* Queue overflow event only if it isn't already queued */
>                 if (!list_empty(&group->overflow_event->list)) {
>                         mutex_unlock(&group->notification_mutex);
> @@ -108,10 +108,12 @@ int fsnotify_add_event(struct fsnotify_group *group,
>         }
>
>         if (!list_empty(list) && merge) {
> -               ret = merge(list, event);
> -               if (ret) {
> +               int merge_ret;
> +
> +               merge_ret = merge(list, event);
> +               if (merge_ret) {
>                         mutex_unlock(&group->notification_mutex);
> -                       return ret;
> +                       return AE_MERGED;
>                 }
>         }
>
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 58205f33af02..b948a52ce65f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -299,11 +299,17 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);
>  /* Free event from memory */
>  extern void fsnotify_destroy_event(struct fsnotify_group *group,
>                                    struct fsnotify_event *event);
> +/* Return values of fsnotify_add_event() */
> +enum fsn_add_event_ret {
> +       AE_INSERTED,    /* Event was added in the queue */
> +       AE_MERGED,      /* Event was merged with another event, passed event unused */
> +       AE_OVERFLOW,    /* Queue overflow, passed event unused */
> +};
>  /* attach the event to the group notification queue */
> -extern int fsnotify_add_event(struct fsnotify_group *group,
> -                             struct fsnotify_event *event,
> -                             int (*merge)(struct list_head *,
> -                                          struct fsnotify_event *));
> +extern enum fsn_add_event_ret fsnotify_add_event(
> +               struct fsnotify_group *group,
> +               struct fsnotify_event *event,
> +               int (*merge)(struct list_head *, struct fsnotify_event *));
>  /* Remove passed event from groups notification queue */
>  extern void fsnotify_remove_event(struct fsnotify_group *group, struct fsnotify_event *event);
>  /* true if the group notification queue is empty */
> --
> 2.6.6
>

Re: [PATCH 2/3] fsnotify: Add a way to stop queueing events on group shutdown

[Miklos Szeredi]

s/AE_SHUTDOWN/1/

Reviewed-by: Miklos Szeredi <mszeredi@redhat.com>

On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> wrote:
> Implement a function that can be called when a group is being shutdown
> to stop queueing new events to the group. Fanotify will use this.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/group.c                | 19 +++++++++++++++++++
>  fs/notify/notification.c         |  5 +++++
>  include/linux/fsnotify_backend.h |  4 ++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index 3e2dd85be5dd..b47f7cfdcaa4 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -40,6 +40,17 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
>  }
>
>  /*
> + * Stop queueing new events for this group. Once this function returns
> + * fsnotify_add_event() will not add any new events to the group's queue.
> + */
> +void fsnotify_group_stop_queueing(struct fsnotify_group *group)
> +{
> +       mutex_lock(&group->notification_mutex);
> +       group->shutdown = true;
> +       mutex_unlock(&group->notification_mutex);
> +}
> +
> +/*
>   * Trying to get rid of a group. Remove all marks, flush all events and release
>   * the group reference.
>   * Note that another thread calling fsnotify_clear_marks_by_group() may still
> @@ -47,6 +58,14 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
>   */
>  void fsnotify_destroy_group(struct fsnotify_group *group)
>  {
> +       /*
> +        * Stop queueing new events. The code below is careful enough to not
> +        * require this but fanotify needs to stop queuing events even before
> +        * fsnotify_destroy_group() is called and this makes the other callers
> +        * of fsnotify_destroy_group() to see the same behavior.
> +        */
> +       fsnotify_group_stop_queueing(group);
> +
>         /* clear all inode marks for this group, attach them to destroy_list */
>         fsnotify_detach_group_marks(group);
>
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 12bfd6790fc4..af0497cac06c 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -96,6 +96,11 @@ enum fsn_add_event_ret fsnotify_add_event(
>
>         mutex_lock(&group->notification_mutex);
>
> +       if (group->shutdown) {
> +               mutex_unlock(&group->notification_mutex);
> +               return AE_SHUTDOWN;
> +       }
> +
>         if (group->q_len >= group->max_events) {
>                 ret = AE_OVERFLOW;
>                 /* Queue overflow event only if it isn't already queued */
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b948a52ce65f..bcba826a99fc 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -148,6 +148,7 @@ struct fsnotify_group {
>         #define FS_PRIO_1       1 /* fanotify content based access control */
>         #define FS_PRIO_2       2 /* fanotify pre-content access */
>         unsigned int priority;
> +       bool shutdown;          /* group is being shut down, don't queue more events */
>
>         /* stores all fastpath marks assoc with this group so they can be cleaned on unregister */
>         struct mutex mark_mutex;        /* protect marks_list */
> @@ -292,6 +293,8 @@ extern struct fsnotify_group *fsnotify_alloc_group(const struct fsnotify_ops *op
>  extern void fsnotify_get_group(struct fsnotify_group *group);
>  /* drop reference on a group from fsnotify_alloc_group */
>  extern void fsnotify_put_group(struct fsnotify_group *group);
> +/* group destruction begins, stop queuing new events */
> +extern void fsnotify_group_stop_queueing(struct fsnotify_group *group);
>  /* destroy group */
>  extern void fsnotify_destroy_group(struct fsnotify_group *group);
>  /* fasync handler function */
> @@ -304,6 +307,7 @@ enum fsn_add_event_ret {
>         AE_INSERTED,    /* Event was added in the queue */
>         AE_MERGED,      /* Event was merged with another event, passed event unused */
>         AE_OVERFLOW,    /* Queue overflow, passed event unused */
> +       AE_SHUTDOWN,    /* Group is being released, passed event unused */
>  };
>  /* attach the event to the group notification queue */
>  extern enum fsn_add_event_ret fsnotify_add_event(
> --
> 2.6.6
>

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Miklos Szeredi]

On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> wrote:
> fanotify_get_response() calls fsnotify_remove_event() when it finds that
> group is being released from fanotify_release() (bypass_perm is set).
> However the event it removes need not be only in the group's notification
> queue but it can have already moved to access_list (userspace read the
> event before closing the fanotify instance fd) which is protected by a
> different lock. Thus when fsnotify_remove_event() races with
> fanotify_release() operating on access_list, the list can get corrupted.
>
> Fix the problem by moving all the logic removing permission events from
> the lists to one place - fanotify_release().

Reviewed-by: Miklos Szeredi <mszeredi@redhat.com>

This should fix the reported issue, and it's cleaner than my solution.
I'll backport and ask QA to test.  I haven't tried reproducing it
myself.

However the theoretical memory access ordering issue (which might
possibly be impossible to trigger) is still there AFAICS:

CPU1:
list_del_init(&event->fae.fse.list);
event->response = FAN_ALLOW;

CPU2:
wait_event(group->fanotify_data.access_waitq, event->response);
...
WARN_ON(!list_empty(&event->list));

So I think we still need a separate patch adding smp_wmb() before
setting event->response and smp_rmb() after the wait_event().

Thanks,
Miklos

>
> Reported-by: Miklos Szeredi <mszeredi@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify.c      | 13 +------------
>  fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++------------
>  include/linux/fsnotify_backend.h   |  1 -
>  3 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 3c6c81e0c2c8..363540df7db9 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -67,18 +67,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
>
>         pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>
> -       wait_event(group->fanotify_data.access_waitq, event->response ||
> -                               atomic_read(&group->fanotify_data.bypass_perm));
> -
> -       if (!event->response) { /* bypass_perm set */
> -               /*
> -                * Event was canceled because group is being destroyed. Remove
> -                * it from group's event list because we are responsible for
> -                * freeing the permission event.
> -                */
> -               fsnotify_remove_event(group, &event->fae.fse);
> -               return 0;
> -       }
> +       wait_event(group->fanotify_data.access_waitq, event->response);
>
>         /* userspace responded, convert to something usable */
>         switch (event->response) {
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8e8e6bcd1d43..a64313868d3a 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -358,16 +358,20 @@ static int fanotify_release(struct inode *ignored, struct file *file)
>
>  #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>         struct fanotify_perm_event_info *event, *next;
> +       struct fsnotify_event *fsn_event;
>
>         /*
> -        * There may be still new events arriving in the notification queue
> -        * but since userspace cannot use fanotify fd anymore, no event can
> -        * enter or leave access_list by now.
> +        * Stop new events from arriving in the notification queue. since
> +        * userspace cannot use fanotify fd anymore, no event can enter or
> +        * leave access_list by now either.
>          */
> -       spin_lock(&group->fanotify_data.access_lock);
> -
> -       atomic_inc(&group->fanotify_data.bypass_perm);
> +       fsnotify_group_stop_queueing(group);
>
> +       /*
> +        * Process all permission events on access_list and notification queue
> +        * and simulate reply from userspace.
> +        */
> +       spin_lock(&group->fanotify_data.access_lock);
>         list_for_each_entry_safe(event, next, &group->fanotify_data.access_list,
>                                  fae.fse.list) {
>                 pr_debug("%s: found group=%p event=%p\n", __func__, group,
> @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
>         spin_unlock(&group->fanotify_data.access_lock);
>
>         /*
> -        * Since bypass_perm is set, newly queued events will not wait for
> -        * access response. Wake up the already sleeping ones now.
> -        * synchronize_srcu() in fsnotify_destroy_group() will wait for all
> -        * processes sleeping in fanotify_handle_event() waiting for access
> -        * response and thus also for all permission events to be freed.
> +        * Destroy all non-permission events. For permission events just
> +        * dequeue them and set the response. They will be freed once the
> +        * response is consumed and fanotify_get_response() returns.
>          */
> +       mutex_lock(&group->notification_mutex);
> +       while (!fsnotify_notify_queue_is_empty(group)) {
> +               fsn_event = fsnotify_remove_first_event(group);
> +               if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
> +                       fsnotify_destroy_event(group, fsn_event);
> +               else
> +                       FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
> +       }
> +       mutex_unlock(&group->notification_mutex);
> +
> +       /* Response for all permission events it set, wakeup waiters */
>         wake_up(&group->fanotify_data.access_waitq);
>  #endif
>
> @@ -755,7 +768,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>         spin_lock_init(&group->fanotify_data.access_lock);
>         init_waitqueue_head(&group->fanotify_data.access_waitq);
>         INIT_LIST_HEAD(&group->fanotify_data.access_list);
> -       atomic_set(&group->fanotify_data.bypass_perm, 0);
>  #endif
>         switch (flags & FAN_ALL_CLASS_BITS) {
>         case FAN_CLASS_NOTIF:
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index bcba826a99fc..7ca04e082002 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -180,7 +180,6 @@ struct fsnotify_group {
>                         spinlock_t access_lock;
>                         struct list_head access_list;
>                         wait_queue_head_t access_waitq;
> -                       atomic_t bypass_perm;
>  #endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
>                         int f_flags;
>                         unsigned int max_marks;
> --
> 2.6.6
>

Re: [PATCH 1/3] fsnotify: Use enum for return values of fsnotify_add_event()

[Jan Kara]

On Fri 09-09-16 15:01:46, Miklos Szeredi wrote:
> On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> wrote:
> > Currently there are three possible returns from fsnotify_add_event(). We
> > will be adding a fourth one. Let's change magic numbers to enum to make
> > things clearer.
> 
> This cleanup should go last, otherwise it'll just make backporting the
> fix harder.
> 
> Also I don't really see the value of having a big enum with 3 of the
> values having the same meaning.   Yeah, there's that BUG_ON for having
> merged a permission event.  But it checks the very obvious condition
> at the top of fanotify_merge() with the big comment, not very
> interesting.
> 
> So I'd kill the BUG_ON, drop the merged/overflowed/shutdown
> distinction and return a two way value (doing it with an enum still
> makes the code more readable, though).

Fair enough, I'll fix this up and resend the series (likely early next
week).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Jan Kara]

On Fri 09-09-16 15:39:44, Miklos Szeredi wrote:
> On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@suse.cz> wrote:
> > fanotify_get_response() calls fsnotify_remove_event() when it finds that
> > group is being released from fanotify_release() (bypass_perm is set).
> > However the event it removes need not be only in the group's notification
> > queue but it can have already moved to access_list (userspace read the
> > event before closing the fanotify instance fd) which is protected by a
> > different lock. Thus when fsnotify_remove_event() races with
> > fanotify_release() operating on access_list, the list can get corrupted.
> >
> > Fix the problem by moving all the logic removing permission events from
> > the lists to one place - fanotify_release().
> 
> Reviewed-by: Miklos Szeredi <mszeredi@redhat.com>
> 
> This should fix the reported issue, and it's cleaner than my solution.
> I'll backport and ask QA to test.  I haven't tried reproducing it
> myself.

OK, thanks!

> However the theoretical memory access ordering issue (which might
> possibly be impossible to trigger) is still there AFAICS:
> 
> CPU1:
> list_del_init(&event->fae.fse.list);
> event->response = FAN_ALLOW;
> 
> CPU2:
> wait_event(group->fanotify_data.access_waitq, event->response);
> ...
> WARN_ON(!list_empty(&event->list));
> 
> So I think we still need a separate patch adding smp_wmb() before
> setting event->response and smp_rmb() after the wait_event().

Ugh, this is really theoretical ;) But I agree nothing really prevents it.
The core reason for this seems to be the lockless check of event->list. I
dislike adding barriers just to accommodate that WARN_ON() (although that
is a useful one so I wouldn't like to lose it either). I'll think about it.
Thanks for spotting this.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Lino Sanfilippo]

Hi,

On 08.09.2016 16:25, Jan Kara wrote:
> fanotify_get_response() calls fsnotify_remove_event() when it finds that
> group is being released from fanotify_release() (bypass_perm is set).
> However the event it removes need not be only in the group's notification
> queue but it can have already moved to access_list (userspace read the
> event before closing the fanotify instance fd) which is protected by a
> different lock. Thus when fsnotify_remove_event() races with
> fanotify_release() operating on access_list, the list can get corrupted.
> 
> Fix the problem by moving all the logic removing permission events from
> the lists to one place - fanotify_release().
> 
> Reported-by: Miklos Szeredi <mszeredi@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

I think all in all this patchset implements a sane solution. Nevertheless 
some comments/questions concerning the new code in fanotify_release below:

> @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
>  	spin_unlock(&group->fanotify_data.access_lock);
>  
>  	/*
> -	 * Since bypass_perm is set, newly queued events will not wait for
> -	 * access response. Wake up the already sleeping ones now.
> -	 * synchronize_srcu() in fsnotify_destroy_group() will wait for all
> -	 * processes sleeping in fanotify_handle_event() waiting for access
> -	 * response and thus also for all permission events to be freed.
> +	 * Destroy all non-permission events. For permission events just
> +	 * dequeue them and set the response. They will be freed once the
> +	 * response is consumed and fanotify_get_response() returns.
>  	 */
> +	mutex_lock(&group->notification_mutex);
> +	while (!fsnotify_notify_queue_is_empty(group)) {
> +		fsn_event = fsnotify_remove_first_event(group);
> +		if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
> +			fsnotify_destroy_event(group, fsn_event);
> +		else
> +			FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;

Why do we have to handle (i.e destroy) non-permission events here? Is this 
meant as some kind of optimization?

Also, we now set the response during both iterations. I assume the assignment during
the iteration of the access list is only a leftover and can be skipped.

Why dont we set the shutdown flag directly in release() as soon as we grep 
the notification mutex. I dont see any reason to do this seperated from
the removal of events from the notification list (or do I miss something?).
Similar situation in destroy_group(): We can set the flag in flush_notify instead
of destroy_group (we then do not even need the dedicated function stop_queueing() then).

fsnotify_remove_event() is not used any more now, so we should remove this function.

Regards,
Lino

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Jan Kara]

On Sat 10-09-16 02:33:34, Lino Sanfilippo wrote:
> > @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> >  	spin_unlock(&group->fanotify_data.access_lock);
> >  
> >  	/*
> > -	 * Since bypass_perm is set, newly queued events will not wait for
> > -	 * access response. Wake up the already sleeping ones now.
> > -	 * synchronize_srcu() in fsnotify_destroy_group() will wait for all
> > -	 * processes sleeping in fanotify_handle_event() waiting for access
> > -	 * response and thus also for all permission events to be freed.
> > +	 * Destroy all non-permission events. For permission events just
> > +	 * dequeue them and set the response. They will be freed once the
> > +	 * response is consumed and fanotify_get_response() returns.
> >  	 */
> > +	mutex_lock(&group->notification_mutex);
> > +	while (!fsnotify_notify_queue_is_empty(group)) {
> > +		fsn_event = fsnotify_remove_first_event(group);
> > +		if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
> > +			fsnotify_destroy_event(group, fsn_event);
> > +		else
> > +			FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
> 
> Why do we have to handle (i.e destroy) non-permission events here? Is this 
> meant as some kind of optimization?

Where else would they be destroyed? Normal events are destroyed when
userspace reads them but nobody is going to read these anymore. For
permission events things are more complex - they get destroyed by the
process creating these events since that process waits for the response.

> Also, we now set the response during both iterations. I assume the
> assignment during the iteration of the access list is only a leftover and
> can be skipped.

Both are currently needed because I've changed fanotify_get_response() to
check only the response value.

> Why dont we set the shutdown flag directly in release() as soon as we
> grep the notification mutex. I dont see any reason to do this seperated
> from the removal of events from the notification list (or do I miss
> something?).  Similar situation in destroy_group(): We can set the flag
> in flush_notify instead of destroy_group (we then do not even need the
> dedicated function stop_queueing() then).

We could do it like that but I didn't want fanotify to hook into generic
fsnotify internals and rather wanted to encapsulate the functionality in
a function. Since this is not really performance critical, extra round trip
on notification_mutex should be fine.

> fsnotify_remove_event() is not used any more now, so we should remove
> this function.

Yeah, will fix that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Miklos Szeredi]

On Mon, Sep 12, 2016 at 10:32 AM, Jan Kara <jack@suse.cz> wrote:
> On Sat 10-09-16 02:33:34, Lino Sanfilippo wrote:
>> > @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
>> >     spin_unlock(&group->fanotify_data.access_lock);
>> >
>> >     /*
>> > -    * Since bypass_perm is set, newly queued events will not wait for
>> > -    * access response. Wake up the already sleeping ones now.
>> > -    * synchronize_srcu() in fsnotify_destroy_group() will wait for all
>> > -    * processes sleeping in fanotify_handle_event() waiting for access
>> > -    * response and thus also for all permission events to be freed.
>> > +    * Destroy all non-permission events. For permission events just
>> > +    * dequeue them and set the response. They will be freed once the
>> > +    * response is consumed and fanotify_get_response() returns.
>> >      */
>> > +   mutex_lock(&group->notification_mutex);
>> > +   while (!fsnotify_notify_queue_is_empty(group)) {
>> > +           fsn_event = fsnotify_remove_first_event(group);
>> > +           if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
>> > +                   fsnotify_destroy_event(group, fsn_event);
>> > +           else
>> > +                   FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
>>
>> Why do we have to handle (i.e destroy) non-permission events here? Is this
>> meant as some kind of optimization?
>
> Where else would they be destroyed? Normal events are destroyed when
> userspace reads them but nobody is going to read these anymore. For
> permission events things are more complex - they get destroyed by the
> process creating these events since that process waits for the response.

I think he meant that normal events get destroyed anyway in
fsnotify_destroy_group().  Which is true, but then we'd need a proper
iterator for picking just permission events and it would be a lot more
complicated in the end.

Thanks,
Miklos

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Lino Sanfilippo]

On 12.09.2016 10:32, Jan Kara wrote:

> 
>> Why dont we set the shutdown flag directly in release() as soon as we
>> grep the notification mutex. I dont see any reason to do this seperated
>> from the removal of events from the notification list (or do I miss
>> something?).  Similar situation in destroy_group(): We can set the flag
>> in flush_notify instead of destroy_group (we then do not even need the
>> dedicated function stop_queueing() then).
> 
> We could do it like that but I didn't want fanotify to hook into generic
> fsnotify internals and rather wanted to encapsulate the functionality in
> a function. Since this is not really performance critical, extra round trip
> on notification_mutex should be fine.

I understood that flag as an extension for generic fsnotify groups, not only
for fanotify. Sure, there is nothing else that uses it right now, but
"being destroyed" is a state that is valid for all types of fsnotify groups, and it could
 be useful in future to know when a group is in this state. But maybe it depends
on the point of view if this should be treated as a fanotify or fsnotify extension. And
its up to you, of course.

Regards,
Lino



 

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Miklos Szeredi]

The backported patches test out OK.

I asked permission to publish the reproducer.

Thanks,
Miklos

Re: [PATCH 3/3] fanotify: Fix list corruption in fanotify_get_response()

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

On Tue, Sep 13, 2016 at 9:58 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> The backported patches test out OK.
>
> I asked permission to publish the reproducer.

Fujitsu provided the attached reproducer.  Entry point is "./run.sh"

Thanks,
Miklos

[-- Attachment #2: fanotify-bug-reproducer.tgz --]
[-- Type: application/x-gzip, Size: 2493 bytes --]