| Message ID | 20190108164611.11440-4-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: Make wait for permission event response interruptible | expand |
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.
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
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);
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(-)