[3/4] fanotify: Track permission event state
diff mbox series

Message ID 20190108164611.11440-4-jack@suse.cz
State New
Headers show
Series
  • fanotify: Make wait for permission event response interruptible
Related show

Commit Message

Jan Kara Jan. 8, 2019, 4:46 p.m. UTC
Track whether permission event got already reported to userspace and
whether userspace already answered to the permission event in high bits
of its ->response field. Also protect stores to ->response field by
group->notification_lock. This will allow aborting wait for reply to
permission event from userspace.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify.c      | 16 ++++++++++------
 fs/notify/fanotify/fanotify.h      | 10 +++++++++-
 fs/notify/fanotify/fanotify_user.c | 17 ++++++++++++-----
 3 files changed, 31 insertions(+), 12 deletions(-)

Comments

Amir Goldstein Jan. 9, 2019, 7:22 a.m. UTC | #1
On Tue, Jan 8, 2019 at 6:46 PM Jan Kara <jack@suse.cz> wrote:
>
> Track whether permission event got already reported to userspace and
> whether userspace already answered to the permission event in high bits
> of its ->response field. Also protect stores to ->response field by
> group->notification_lock. This will allow aborting wait for reply to
> permission event from userspace.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify.c      | 16 ++++++++++------
>  fs/notify/fanotify/fanotify.h      | 10 +++++++++-
>  fs/notify/fanotify/fanotify_user.c | 17 ++++++++++++-----
>  3 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 3723f3d18d20..cca13adc3a4c 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -62,13 +62,19 @@ static int fanotify_get_response(struct fsnotify_group *group,
>                                  struct fsnotify_iter_info *iter_info)
>  {
>         int ret;
> +       unsigned int response;
>
> +       BUILD_BUG_ON(FAN_EVENT_STATE_MASK & (FAN_AUDIT | FAN_ALLOW | FAN_DENY));
>         pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>
> -       wait_event(group->fanotify_data.access_waitq, event->response);
> +       wait_event(group->fanotify_data.access_waitq,
> +                  (event->response & FAN_EVENT_STATE_MASK) ==
> +                                                       FAN_EVENT_ANSWERED);
> +
> +       response = event->response & ~FAN_EVENT_STATE_MASK;
>
>         /* userspace responded, convert to something usable */
> -       switch (event->response & ~FAN_AUDIT) {
> +       switch (response & ~FAN_AUDIT) {
>         case FAN_ALLOW:
>                 ret = 0;
>                 break;
> @@ -78,10 +84,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
>         }
>
>         /* Check if the response should be audited */
> -       if (event->response & FAN_AUDIT)
> -               audit_fanotify(event->response & ~FAN_AUDIT);
> -
> -       event->response = 0;
> +       if (response & FAN_AUDIT)
> +               audit_fanotify(response & ~FAN_AUDIT);
>
>         pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
>                  group, event, ret);
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index ea05b8a401e7..954d997745c3 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -22,6 +22,12 @@ struct fanotify_event_info {
>         struct pid *pid;
>  };
>
> +/* State of permission event we store inside response field */
> +#define FAN_EVENT_STATE_MASK 0xc0000000
> +
> +#define FAN_EVENT_REPORTED 0x40000000  /* Event reported to userspace */
> +#define FAN_EVENT_ANSWERED 0x80000000  /* Event answered by userspace */
> +
>  /*
>   * Structure for permission fanotify events. It gets allocated and freed in
>   * fanotify_handle_event() since we wait there for user response. When the
> @@ -31,7 +37,9 @@ struct fanotify_event_info {
>   */
>  struct fanotify_perm_event_info {
>         struct fanotify_event_info fae;
> -       int response;   /* userspace answer to question */
> +       unsigned int response;  /* userspace answer to the event. We also use
> +                                * high bits of this field for recording state
> +                                * of the event. */

What's wrong with:

        short response;   /* userspace answer to question */
        short state;   /* the state of this event */

Will make for a much simpler code/patch IMO.

Thanks,
Amir.
Jan Kara Jan. 9, 2019, 9:11 a.m. UTC | #2
On Wed 09-01-19 09:22:19, Amir Goldstein wrote:
> > @@ -31,7 +37,9 @@ struct fanotify_event_info {
> >   */
> >  struct fanotify_perm_event_info {
> >         struct fanotify_event_info fae;
> > -       int response;   /* userspace answer to question */
> > +       unsigned int response;  /* userspace answer to the event. We also use
> > +                                * high bits of this field for recording state
> > +                                * of the event. */
> 
> What's wrong with:
> 
>         short response;   /* userspace answer to question */
>         short state;   /* the state of this event */
> 
> Will make for a much simpler code/patch IMO.

I guess you're right. I'm somewhat cautious about types shorter than int as
I do read 'state' locklessly and for shorter-than-int types compiler can do
fancy things like read-modify-write cycle of a full word when storing some
value in either response or state. But I think this particular case should
be fine.

								Honza

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 3723f3d18d20..cca13adc3a4c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -62,13 +62,19 @@  static int fanotify_get_response(struct fsnotify_group *group,
 				 struct fsnotify_iter_info *iter_info)
 {
 	int ret;
+	unsigned int response;
 
+	BUILD_BUG_ON(FAN_EVENT_STATE_MASK & (FAN_AUDIT | FAN_ALLOW | FAN_DENY));
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	wait_event(group->fanotify_data.access_waitq, event->response);
+	wait_event(group->fanotify_data.access_waitq,
+		   (event->response & FAN_EVENT_STATE_MASK) ==
+							FAN_EVENT_ANSWERED);
+
+	response = event->response & ~FAN_EVENT_STATE_MASK;
 
 	/* userspace responded, convert to something usable */
-	switch (event->response & ~FAN_AUDIT) {
+	switch (response & ~FAN_AUDIT) {
 	case FAN_ALLOW:
 		ret = 0;
 		break;
@@ -78,10 +84,8 @@  static int fanotify_get_response(struct fsnotify_group *group,
 	}
 
 	/* Check if the response should be audited */
-	if (event->response & FAN_AUDIT)
-		audit_fanotify(event->response & ~FAN_AUDIT);
-
-	event->response = 0;
+	if (response & FAN_AUDIT)
+		audit_fanotify(response & ~FAN_AUDIT);
 
 	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
 		 group, event, ret);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index ea05b8a401e7..954d997745c3 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -22,6 +22,12 @@  struct fanotify_event_info {
 	struct pid *pid;
 };
 
+/* State of permission event we store inside response field */
+#define FAN_EVENT_STATE_MASK 0xc0000000
+
+#define FAN_EVENT_REPORTED 0x40000000	/* Event reported to userspace */
+#define FAN_EVENT_ANSWERED 0x80000000	/* Event answered by userspace */
+
 /*
  * Structure for permission fanotify events. It gets allocated and freed in
  * fanotify_handle_event() since we wait there for user response. When the
@@ -31,7 +37,9 @@  struct fanotify_event_info {
  */
 struct fanotify_perm_event_info {
 	struct fanotify_event_info fae;
-	int response;	/* userspace answer to question */
+	unsigned int response;	/* userspace answer to the event. We also use
+				 * high bits of this field for recording state
+				 * of the event. */
 	int fd;		/* fd we passed to userspace for this event */
 };
 
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 2b2c8b8a17bd..611c2ff50d64 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -50,7 +50,8 @@  struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 /*
  * Get an fsnotify notification event if one exists and is small
  * enough to fit in "count". Return an error pointer if the count
- * is not large enough.
+ * is not large enough. When permission event is dequeued, its state is
+ * updated accordingly.
  */
 static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
@@ -66,6 +67,8 @@  static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 		goto out;
 	}
 	event = fsnotify_remove_first_event(group);
+	if (fanotify_is_perm_event(event->mask))
+		FANOTIFY_PE(event)->response = FAN_EVENT_REPORTED;
 out:
 	spin_unlock(&group->notification_lock);
 	return event;
@@ -178,7 +181,7 @@  static int process_access_response(struct fsnotify_group *group,
 			continue;
 
 		list_del_init(&event->fae.fse.list);
-		event->response = response;
+		event->response = response | FAN_EVENT_ANSWERED;
 		spin_unlock(&group->notification_lock);
 		wake_up(&group->fanotify_data.access_waitq);
 		return 0;
@@ -301,7 +304,10 @@  static ssize_t fanotify_read(struct file *file, char __user *buf,
 			fsnotify_destroy_event(group, kevent);
 		} else {
 			if (ret <= 0) {
-				FANOTIFY_PE(kevent)->response = FAN_DENY;
+				spin_lock(&group->notification_lock);
+				FANOTIFY_PE(kevent)->response =
+						FAN_DENY | FAN_EVENT_ANSWERED;
+				spin_unlock(&group->notification_lock);
 				wake_up(&group->fanotify_data.access_waitq);
 			} else {
 				spin_lock(&group->notification_lock);
@@ -372,7 +378,7 @@  static int fanotify_release(struct inode *ignored, struct file *file)
 			 event);
 
 		list_del_init(&event->fae.fse.list);
-		event->response = FAN_ALLOW;
+		event->response = FAN_ALLOW | FAN_EVENT_ANSWERED;
 	}
 
 	/*
@@ -387,7 +393,8 @@  static int fanotify_release(struct inode *ignored, struct file *file)
 			fsnotify_destroy_event(group, fsn_event);
 			spin_lock(&group->notification_lock);
 		} else {
-			FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
+			FANOTIFY_PE(fsn_event)->response =
+					FAN_ALLOW | FAN_EVENT_ANSWERED;
 		}
 	}
 	spin_unlock(&group->notification_lock);