| Message ID | 20210426184201.4177978-11-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | File system wide monitoring | expand |
On Mon, Apr 26, 2021 at 9:43 PM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > This patch introduces an optional info record that describes the > source (as in the region of the source-code where an event was > initiated). This record is not produced for other type of existing > notification, but it is optionally enabled for FAN_ERROR notifications. > I find this functionality controversial, because think that the fs provided s_last_error*, s_first_error* is more reliable and more powerful than this functionality. Let's leave it for a future extending proposal, should fanotify event reporting proposal pass muster, shall we? Or do you think that without this optional extension fanotify event reporting will not be valuable enough? Thanks, Amir.
Amir Goldstein <amir73il@gmail.com> writes: > On Mon, Apr 26, 2021 at 9:43 PM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: >> >> This patch introduces an optional info record that describes the >> source (as in the region of the source-code where an event was >> initiated). This record is not produced for other type of existing >> notification, but it is optionally enabled for FAN_ERROR notifications. >> > > I find this functionality controversial, because think that the fs provided > s_last_error*, s_first_error* is more reliable and more powerful than this > functionality. > > Let's leave it for a future extending proposal, should fanotify event reporting > proposal pass muster, shall we? > Or do you think that without this optional extension fanotify event reporting > will not be valuable enough? I think it is valuable enough without this bit, at least on a first moment. I understand it would be useful for ext4 to analyse information through this interface, but the main priority is to have a way to push out the information that an error occured, as you mentioned. Also, this might be more powerful if we stick to the ring buffer instead of single stlot, as it would allow more data to be collected than just first/last. > > Thanks, > Amir.
On Thu, Apr 29, 2021 at 11:40 AM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Amir Goldstein <amir73il@gmail.com> writes: > > > On Mon, Apr 26, 2021 at 9:43 PM Gabriel Krisman Bertazi > > <krisman@collabora.com> wrote: > >> > >> This patch introduces an optional info record that describes the > >> source (as in the region of the source-code where an event was > >> initiated). This record is not produced for other type of existing > >> notification, but it is optionally enabled for FAN_ERROR notifications. > >> > > > > I find this functionality controversial, because think that the fs provided > > s_last_error*, s_first_error* is more reliable and more powerful than this > > functionality. > > > > Let's leave it for a future extending proposal, should fanotify event reporting > > proposal pass muster, shall we? > > Or do you think that without this optional extension fanotify event reporting > > will not be valuable enough? > > I think it is valuable enough without this bit, at least on a first > moment. I understand it would be useful for ext4 to analyse information > through this interface, but the main priority is to have a way to push > out the information that an error occured, as you mentioned. Ack, if it's deemed cleaner we could look at sysfs on notification, but having the information in the same event provides some convenience factor, and avoids racing in the event that we're looking at an error after the first one. > > Also, this might be more powerful if we stick to the ring buffer instead > of single stlot, as it would allow more data to be collected than just > first/last. > > > > Thanks, > > Amir. > > -- > Gabriel Krisman Bertazi
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 4cb9dd31f084..0d1b4cb8b005 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -161,6 +161,11 @@ struct fanotify_fid_event { unsigned char _inline_fh_buf[FANOTIFY_INLINE_FH_LEN]; }; +struct fanotify_event_location { + int line; + const char *function; +}; + static inline struct fanotify_fid_event * FANOTIFY_FE(struct fanotify_event *event) { @@ -183,6 +188,7 @@ struct fanotify_error_event { struct fanotify_event fae; int error; __kernel_fsid_t fsid; + struct fanotify_event_location loc; int fs_data_size; /* Must be the last item in the structure */ diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 21162d347bd1..cb79a4a8e6fb 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -69,6 +69,16 @@ static size_t fanotify_error_info_len(struct fanotify_error_event *fee) return sizeof(struct fanotify_event_info_error); } +static size_t fanotify_location_info_len(const struct fanotify_event_location *loc) +{ + if (!loc->function) + return 0; + + /* Includes NULL byte at end of loc->function */ + return (sizeof(struct fanotify_event_info_location) + + strlen(loc->function) + 1); +} + static size_t fanotify_event_len(struct fanotify_event *event, unsigned int fid_mode) { @@ -260,6 +270,31 @@ static size_t copy_error_info_to_user(struct fanotify_error_event *fee, } +static size_t copy_location_info_to_user(struct fanotify_event_location *location, + char __user *buf, int count) +{ + size_t len = fanotify_location_info_len(location); + size_t tail = len - sizeof(struct fanotify_event_info_location); + struct fanotify_event_info_location info; + + if (!len) + return 0; + + info.hdr.info_type = FAN_EVENT_INFO_TYPE_LOCATION; + info.hdr.len = len; + info.line = location->line; + + if (copy_to_user(buf, &info, sizeof(info))) + return -EFAULT; + + buf += sizeof(info); + + if (copy_to_user(buf, location->function, tail)) + return -EFAULT; + + return info.hdr.len; +} + static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, int info_type, const char *name, size_t name_len, char __user *buf, size_t count) diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index cc9a1fa80e30..67217756dac9 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -125,6 +125,7 @@ struct fanotify_event_metadata { #define FAN_EVENT_INFO_TYPE_DFID_NAME 2 #define FAN_EVENT_INFO_TYPE_DFID 3 #define FAN_EVENT_INFO_TYPE_ERROR 4 +#define FAN_EVENT_INFO_TYPE_LOCATION 5 /* Variable length info record following event metadata */ struct fanotify_event_info_header { @@ -159,6 +160,12 @@ struct fanotify_event_info_error { __kernel_fsid_t fsid; }; +struct fanotify_event_info_location { + struct fanotify_event_info_header hdr; + int line; + char function[0]; +}; + struct fanotify_response { __s32 fd; __u32 response;
This patch introduces an optional info record that describes the source (as in the region of the source-code where an event was initiated). This record is not produced for other type of existing notification, but it is optionally enabled for FAN_ERROR notifications. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify.h | 6 +++++ fs/notify/fanotify/fanotify_user.c | 35 ++++++++++++++++++++++++++++++ include/uapi/linux/fanotify.h | 7 ++++++ 3 files changed, 48 insertions(+)