From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:32
Hi,
This is the 6th version of the FAN_FS_ERROR patches. This applies
the feedback from last version (thanks Amir, Jan).
There are important changes in this version. some of which brings us
back to previous versions of this series. I did my best to avoid
problems that were mentioned during earlier revisions, and I think I
covered everything. But I apologize if this requires reviewers to repeat
some comments.
First of all, despite initializing the error event from inside the
insert callback and abusing the merge logic for err_count update, this
version reverts to a simple insertion code, and configures the event
before sending it to be queued by fsnotify. This makes the submission
code less trivial, but addresses the potential problem of encoding the
FH while holding the group->notification_lock.
This version also drops the slot replacement code when dequeueing, and
reverts back to the copy-to-stack mechanism. This simplifies the code a
lot.
The way we report superblock errors also changed. Now, the handle is
omitted and we return the handle_bytes as 0.
Finally, we no longer play games with predicting the file handle size
beforehand. Now, the code just allocates space for the largest handle
possible, and assume that is enough.
On another note, this also restores the mark reference owned by the
error event while it is queued. As Amir explained, this is required to
prevent the mark from going away while the event is queued.
This was tested with LTP for regressions and also using the sample code
on the last patch, with a corrupted image. I wrote a new ltp test for
this feature which is being reviewed and is available at:
https://gitlab.collabora.com/krisman/ltp -b fan-fs-error
In addition, I wrote a man-page that can be pulled from:
https://gitlab.collabora.com/krisman/man-pages.git -b fan-fs-error
And is being reviewed at the list.
I also pushed this full series to:
https://gitlab.collabora.com/krisman/linux -b fanotify-notifications-single-slot
Thank you
Original cover letter
---------------------
Hi,
This series follow up on my previous proposal [1] to support file system
wide monitoring. As suggested by Amir, this proposal drops the ring
buffer in favor of a single slot associated with each mark. This
simplifies a bit the implementation, as you can see in the code.
As a reminder, This proposal is limited to an interface for
administrators to monitor the health of a file system, instead of a
generic inteface for file errors. Therefore, this doesn't solve the
problem of writeback errors or the need to watch a specific subtree.
In comparison to the previous RFC, this implementation also drops the
per-fs data and location, and leave those as future extensions.
* Implementation
The feature is implemented on top of fanotify, as a new type of fanotify
mark, FAN_ERROR, which a file system monitoring tool can register to
receive error notifications. When an error occurs a new notification is
generated, in addition followed by this info field:
- FS generic data: A file system agnostic structure that has a generic
error code and identifies the filesystem. Basically, it let's
userspace know something happened on a monitored filesystem. Since
only the first error is recorded since the last read, this also
includes a counter of errors that happened since the last read.
* Testing
This was tested by watching notifications flowing from an intentionally
corrupted filesystem in different places. In addition, other events
were watched in an attempt to detect regressions.
Is there a specific testsuite for fanotify I should be running?
* Patches
This patchset is divided as follows: Patch 1 through 5 are refactoring
to fsnotify/fanotify in preparation for FS_ERROR/FAN_ERROR; patch 6 and
7 implement the FS_ERROR API for filesystems to report error; patch 8
add support for FAN_ERROR in fanotify; Patch 9 is an example
implementation for ext4; patch 10 and 11 provide a sample userspace code
and documentation.
I also pushed the full series to:
https://gitlab.collabora.com/krisman/linux -b fanotify-notifications-single-slot
[1] https://lwn.net/Articles/854545/
[2] https://lwn.net/Articles/856916/
Cc: Darrick J. Wong <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dave Chinner <david@fromorbit.com>
Cc: jack@suse.com
To: amir73il@gmail.com
Cc: dhowells@redhat.com
Cc: khazhy@google.com
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-ext4@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-api@vger.kernel.org
Gabriel Krisman Bertazi (21):
fsnotify: Don't insert unmergeable events in hashtable
fanotify: Fold event size calculation to its own function
fanotify: Split fsid check from other fid mode checks
fsnotify: Reserve mark flag bits for backends
fanotify: Split superblock marks out to a new cache
inotify: Don't force FS_IN_IGNORED
fsnotify: Add helper to detect overflow_event
fsnotify: Add wrapper around fsnotify_add_event
fsnotify: Allow events reported with an empty inode
fsnotify: Support FS_ERROR event type
fanotify: Allow file handle encoding for unhashed events
fanotify: Encode invalid file handle when no inode is provided
fanotify: Require fid_mode for any non-fd event
fanotify: Reserve UAPI bits for FAN_FS_ERROR
fanotify: Preallocate per superblock mark error event
fanotify: Handle FAN_FS_ERROR events
fanotify: Report fid info for file related file system errors
fanotify: Emit generic error info type for error event
ext4: Send notifications on error
samples: Add fs error monitoring example
docs: Document the FAN_FS_ERROR event
.../admin-guide/filesystem-monitoring.rst | 70 +++++
Documentation/admin-guide/index.rst | 1 +
fs/ext4/super.c | 8 +
fs/notify/fanotify/fanotify.c | 139 +++++++++-
fs/notify/fanotify/fanotify.h | 69 ++++-
fs/notify/fanotify/fanotify_user.c | 256 ++++++++++++++----
fs/notify/fsnotify.c | 19 +-
fs/notify/inotify/inotify_fsnotify.c | 2 +-
fs/notify/inotify/inotify_user.c | 6 +-
fs/notify/notification.c | 12 +-
include/linux/fanotify.h | 9 +-
include/linux/fsnotify.h | 13 +
include/linux/fsnotify_backend.h | 64 ++++-
include/uapi/linux/fanotify.h | 8 +
samples/Kconfig | 9 +
samples/Makefile | 1 +
samples/fanotify/Makefile | 5 +
samples/fanotify/fs-monitor.c | 138 ++++++++++
18 files changed, 740 insertions(+), 89 deletions(-)
create mode 100644 Documentation/admin-guide/filesystem-monitoring.rst
create mode 100644 samples/fanotify/Makefile
create mode 100644 samples/fanotify/fs-monitor.c
--
2.32.0
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:38
Some events, like the overflow event, are not mergeable, so they are not
hashed. But, when failing inside fsnotify_add_event for lack of space,
fsnotify_add_event() still calls the insert hook, which adds the
overflow event to the merge list. Add a check to prevent any kind of
unmergeable event to be inserted in the hashtable.
Fixes: 94e00d28a680 ("fsnotify: use hash table for faster events merge")
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v2:
- Do check for hashed events inside the insert hook (Amir)
---
fs/notify/fanotify/fanotify.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:39
Every time this function is invoked, it is immediately added to
FAN_EVENT_METADATA_LEN, since there is no need to just calculate the
length of info records. This minor clean up folds the rest of the
calculation into the function, which now operates in terms of events,
returning the size of the entire event, including metadata.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v1:
- rebased on top of hashing patches
---
fs/notify/fanotify/fanotify_user.c | 33 +++++++++++++++++-------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -117,17 +117,24 @@ static int fanotify_fid_info_len(int fh_len, int name_len)returnroundup(FANOTIFY_INFO_HDR_LEN+info_len,FANOTIFY_EVENT_ALIGN);}-staticintfanotify_event_info_len(unsignedintfid_mode,-structfanotify_event*event)+staticsize_tfanotify_event_len(structfanotify_event*event,+unsignedintfid_mode){-structfanotify_info*info=fanotify_event_info(event);-intdir_fh_len=fanotify_event_dir_fh_len(event);-intfh_len=fanotify_event_object_fh_len(event);-intinfo_len=0;+size_tevent_len=FAN_EVENT_METADATA_LEN;+structfanotify_info*info;+intdir_fh_len;+intfh_len;intdot_len=0;+if(!fid_mode)+returnevent_len;++info=fanotify_event_info(event);+dir_fh_len=fanotify_event_dir_fh_len(event);+fh_len=fanotify_event_object_fh_len(event);+if(dir_fh_len){-info_len+=fanotify_fid_info_len(dir_fh_len,info->name_len);+event_len+=fanotify_fid_info_len(dir_fh_len,info->name_len);}elseif((fid_mode&FAN_REPORT_NAME)&&(event->mask&FAN_ONDIR)){/**WithgroupflagFAN_REPORT_NAME,ifnamewasnotrecordedin
@@ -137,9 +144,9 @@ static int fanotify_event_info_len(unsigned int fid_mode,}if(fh_len)-info_len+=fanotify_fid_info_len(fh_len,dot_len);+event_len+=fanotify_fid_info_len(fh_len,dot_len);-returninfo_len;+returnevent_len;}/*
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:41
FAN_FS_ERROR will require fsid, but not necessarily require the
filesystem to expose a file handle. Split those checks into different
functions, so they can be used separately when setting up an event.
While there, update a comment about tmpfs having 0 fsid, which is no
longer true.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v2:
- FAN_ERROR -> FAN_FS_ERROR (Amir)
- Update comment (Amir)
Changes since v1:
(Amir)
- Sort hunks to simplify diff.
Changes since RFC:
(Amir)
- Rename fanotify_check_path_fsid -> fanotify_test_fsid.
- Use dentry directly instead of path.
---
fs/notify/fanotify/fanotify_user.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
@@ -1220,6 +1219,12 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)root_fsid.val[1]!=fsid->val[1])return-EXDEV;+return0;+}++/* Check if filesystem can encode a unique fid */+staticintfanotify_test_fid(structdentry*dentry)+{/**Weneedtomakesurethatthefilesystemsupportsatleast*encodingafilehandlesousercanusename_to_handle_at()to
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:46
Split out the final bits of struct fsnotify_mark->flags for use by a
backend.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Changes since v1:
- turn consts into defines (jan)
---
include/linux/fsnotify_backend.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:50
FAN_FS_ERROR will require an error structure to be stored per mark.
But, since FAN_FS_ERROR doesn't apply to inode/mount marks, it should
suffice to only expose this information for superblock marks. Therefore,
wrap this kind of marks into a container and plumb it for the future.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- turn the flag bits into defines (jan)
- don't use zalloc for consistency (jan)
Changes since v2:
- Move mark initialization to fanotify_alloc_mark (Amir)
Changes since v1:
- Only extend superblock marks (Amir)
---
fs/notify/fanotify/fanotify.c | 10 ++++++--
fs/notify/fanotify/fanotify.h | 20 ++++++++++++++++
fs/notify/fanotify/fanotify_user.c | 38 ++++++++++++++++++++++++++++--
3 files changed, 64 insertions(+), 4 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:53
According to Amir:
"FS_IN_IGNORED is completely internal to inotify and there is no need
to set it in i_fsnotify_mask at all, so if we remove the bit from the
output of inotify_arg_to_mask() no functionality will change and we will
be able to overload the event bit for FS_ERROR."
This is done in preparation to overload FS_ERROR with the notification
mechanism in fanotify.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/inotify/inotify_user.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:40:57
Similarly to fanotify_is_perm_event and friends, provide a helper
predicate to say whether a mask is of an overflow event.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.h | 3 ++-
include/linux/fsnotify_backend.h | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:01
fsnotify_add_event is growing in number of parameters, which in most
case are just passed a NULL pointer. So, split out a new
fsnotify_insert_event function to clean things up for users who don't
need an insert hook.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 4 ++--
fs/notify/inotify/inotify_fsnotify.c | 2 +-
fs/notify/notification.c | 12 ++++++------
include/linux/fsnotify_backend.h | 23 ++++++++++++++++-------
4 files changed, 25 insertions(+), 16 deletions(-)
@@ -116,7 +116,7 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,if(len)strcpy(event->name,name->name);-ret=fsnotify_add_event(group,fsn_event,inotify_merge,NULL);+ret=fsnotify_add_event(group,fsn_event,inotify_merge);if(ret){/* Our event wasn't used in the end. Free it. */fsnotify_destroy_event(group,fsn_event);
@@ -494,16 +494,25 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);externvoidfsnotify_destroy_event(structfsnotify_group*group,structfsnotify_event*event);/* attach the event to the group notification queue */-externintfsnotify_add_event(structfsnotify_group*group,-structfsnotify_event*event,-int(*merge)(structfsnotify_group*,-structfsnotify_event*),-void(*insert)(structfsnotify_group*,-structfsnotify_event*));+externintfsnotify_insert_event(structfsnotify_group*group,+structfsnotify_event*event,+int(*merge)(structfsnotify_group*,+structfsnotify_event*),+void(*insert)(structfsnotify_group*,+structfsnotify_event*));++staticinlineintfsnotify_add_event(structfsnotify_group*group,+structfsnotify_event*event,+int(*merge)(structfsnotify_group*,+structfsnotify_event*))+{+returnfsnotify_insert_event(group,event,merge,NULL);+}+/* Queue overflow event to a notification group */staticinlinevoidfsnotify_queue_overflow(structfsnotify_group*group){-fsnotify_add_event(group,group->overflow_event,NULL,NULL);+fsnotify_add_event(group,group->overflow_event,NULL);}staticinlineboolfsnotify_is_overflow_event(u32mask)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:05
Some file system events (i.e. FS_ERROR) might not be associated with an
inode. For these, it makes sense to associate them directly with the
super block of the file system they apply to. This patch allows the
event to be reported with a NULL inode, by recovering the superblock
directly from the data field, if needed.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
---
fs/notify/fsnotify.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:08
Expose a new type of fsnotify event for filesystems to report errors for
userspace monitoring tools. fanotify will send this type of
notification for FAN_FS_ERROR events. This also introduce a helper for
generating the new event.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!
Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)
Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)
---
fs/notify/fsnotify.c | 3 +++
include/linux/fsnotify.h | 13 +++++++++++++
include/linux/fsnotify_backend.h | 18 +++++++++++++++++-
3 files changed, 33 insertions(+), 1 deletion(-)
@@ -42,6 +42,12 @@#define FS_UNMOUNT 0x00002000 /* inode on umount fs */#define FS_Q_OVERFLOW 0x00004000 /* Event queued overflowed */+#define FS_ERROR 0x00008000 /* Filesystem Error (fanotify) */++/*+*FS_IN_IGNOREDoverloadsFS_ERROR.Itisonlyusedinternallybyinotify+*whichdoesnotsupportFS_ERROR.+*/#define FS_IN_IGNORED 0x00008000 /* last inotify event here */#define FS_OPEN_PERM 0x00010000 /* open event in an permission hook */
@@ -95,7 +101,8 @@#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \FS_EVENTS_POSS_ON_CHILD|\FS_DELETE_SELF|FS_MOVE_SELF|FS_DN_RENAME|\-FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED)+FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED|\+FS_ERROR)/* Extra flags that may be reported with event or control handling of events */#define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:12
FAN_FS_ERROR will report a file handle, but it is an unhashed event.
Allow passing a NULL hash to fanotify_encode_fh and avoid calculating
the hash if not needed.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/notify/fanotify/fanotify.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:19
Instead of failing, encode an invalid file handle in fanotify_encode_fh
if no inode is provided. This bogus file handle will be reported by
FAN_FS_ERROR for non-inode errors.
When being reported to userspace, the length information is actually
reset and the handle cleaned up, such that userspace don't have the
visibility of the internal kernel representation of this null handle.
Also adjust the single caller that might rely on failure after passing
an empty inode.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic
---
fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++-----
fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
2 files changed, 41 insertions(+), 21 deletions(-)
@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,return-EFAULT;handle.handle_type=fh->type;-handle.handle_bytes=fh_len;++/* FILEID_INVALID handle type is reported without its f_handle. */+if(fh->type!=FILEID_INVALID)+handle.handle_bytes=fh_len;if(copy_to_user(buf,&handle,sizeof(handle)))return-EFAULT;
@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,if(WARN_ON_ONCE(len<fh_len))return-EFAULT;-/*-*Foraninlinefhandinlinefilename,copythroughstacktoexclude-*thecopyfromusercopyhardeningprotections.-*/-fh_buf=fanotify_fh_buf(fh);-if(fh_len<=FANOTIFY_INLINE_FH_LEN){-memcpy(bounce,fh_buf,fh_len);-fh_buf=bounce;+if(fh->type!=FILEID_INVALID){+/*+*Foraninlinefhandinlinefilename,copythrough+*stacktoexcludethecopyfromusercopyhardening+*protections.+*/+fh_buf=fanotify_fh_buf(fh);+if(fh_len<=FANOTIFY_INLINE_FH_LEN){+memcpy(bounce,fh_buf,fh_len);+fh_buf=bounce;+}+if(copy_to_user(buf,fh_buf,fh_len))+return-EFAULT;+buf+=fh_len;+len-=fh_len;}-if(copy_to_user(buf,fh_buf,fh_len))-return-EFAULT;--buf+=fh_len;-len-=fh_len;if(name_len){/* Copy the filename with terminating null */
@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,}/* Pad with 0's */-WARN_ON_ONCE(len<0||len>=FANOTIFY_EVENT_ALIGN);+WARN_ON_ONCE(len<0);if(len>0&&clear_user(buf,len))return-EFAULT;
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:23
Like inode events, FAN_FS_ERROR will require fid mode. Therefore,
convert the verification during fanotify_mark(2) to require fid for any
non-fd event. This means fid_mode will not only be required for inode
events, but for any event that doesn't provide a descriptor.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
changes since v5:
- Fix condition to include FANOTIFY_EVENT_FLAGS. (me)
- Fix comment identation (jan)
---
fs/notify/fanotify/fanotify_user.c | 12 ++++++------
include/linux/fanotify.h | 3 +++
2 files changed, 9 insertions(+), 6 deletions(-)
@@ -81,6 +81,9 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */*/#define FANOTIFY_DIRENT_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE)+/* Events that can be reported with event->fd */+#define FANOTIFY_FD_EVENTS (FANOTIFY_PATH_EVENTS | FANOTIFY_PERM_EVENTS)+/* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \FAN_ATTRIB|FAN_MOVE_SELF|FAN_DELETE_SELF)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:25
FAN_FS_ERROR allows reporting of event type FS_ERROR to userspace, which
a mechanism to report file system wide problems via fanotify. This
commit preallocate userspace visible bits to match the FS_ERROR event.
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 1 +
include/uapi/linux/fanotify.h | 1 +
2 files changed, 2 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:29
Error reporting needs to be done in an atomic context. This patch
introduces a single error slot for superblock marks that report the
FAN_FS_ERROR event, to be used during event submission.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)
---
fs/notify/fanotify/fanotify.c | 12 ++++++++++++
fs/notify/fanotify/fanotify.h | 13 +++++++++++++
fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
3 files changed, 54 insertions(+), 2 deletions(-)
@@ -216,6 +218,17 @@ FANOTIFY_NE(struct fanotify_event *event)returncontainer_of(event,structfanotify_name_event,fae);}+structfanotify_error_event{+structfanotify_eventfae;+structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */+};++staticinlinestructfanotify_error_event*+FANOTIFY_EE(structfanotify_event*event)+{+returncontainer_of(event,structfanotify_error_event,fae);+}+staticinline__kernel_fsid_t*fanotify_event_fsid(structfanotify_event*event){if(event->type==FANOTIFY_EVENT_TYPE_FID)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:34
Wire up FAN_FS_ERROR in the fanotify_mark syscall. The event can only
be requested for the entire filesystem, thus it requires the
FAN_MARK_FILESYSTEM.
FAN_FS_ERROR has to be handled slightly differently from other events
because it needs to be submitted in an atomic context, using
preallocated memory. This patch implements the submission path by only
storing the first error event that happened in the slot (userspace
resets the slot by reading the event).
Extra error events happening when the slot is occupied are merged to the
original report, and the only information keep for these extra errors is
an accumulator counting the number of events, which is part of the
record reported back to userspace.
Reporting only the first event should be fine, since when a FS error
happens, a cascade of error usually follows, but the most meaningful
information is (usually) on the first erro.
The event dequeueing is also a bit special to avoid losing events. Since
event merging only happens while the event is queued, there is a window
between when an error event is dequeued (notification_lock is dropped)
until it is reset (.free_event()) where the slot is full, but no merges
can happen.
The proposed solution is to copy the event to the stack prior to
dropping the lock. This way, if a new event arrives in the time between
the event was dequeued and the time it resets, the new errors will still
be logged and merged in the recently freed slot.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Copy to stack instead of replacing the fee slot(jan)
- prepare error slot outside of the notification lock(jan)
Changes since v4:
- Split parts to earlier patches (amir)
- Simplify fanotify entry replacement
- Update handle size prediction on overflow
Changes since v3:
- Convert WARN_ON to pr_warn (amir)
- Remove unecessary READ/WRITE_ONCE (amir)
- Alloc with GFP_KERNEL_ACCOUNT(amir)
- Simplify flags on mark allocation (amir)
- Avoid atomic set of error_count (amir)
- Simplify rules when merging error_event (amir)
- Allocate new error_event on get_one_event (amir)
- Report superblock error with invalid FH (amir,jan)
Changes since v2:
- Support and equire FID mode (amir)
- Goto error path instead of early return (amir)
- Simplify get_one_event (me)
- Base merging on error_count
- drop fanotify_queue_error_event
Changes since v1:
- Pass dentry to fanotify_check_fsid (Amir)
- FANOTIFY_EVENT_TYPE_ERROR -> FANOTIFY_EVENT_TYPE_FS_ERROR
- Merge previous patch into it
- Use a single slot
- Move fanotify_mark.error_event definition to this commit
- Rename FAN_ERROR -> FAN_FS_ERROR
- Restrict FAN_FS_ERROR to FAN_MARK_FILESYSTEM
---
fs/notify/fanotify/fanotify.c | 57 +++++++++++++++++++++++++++++-
fs/notify/fanotify/fanotify.h | 21 +++++++++++
fs/notify/fanotify/fanotify_user.c | 39 ++++++++++++++++++--
include/linux/fanotify.h | 6 +++-
4 files changed, 119 insertions(+), 4 deletions(-)
@@ -1031,6 +1057,10 @@ static int fanotify_add_mark(struct fsnotify_group *group,fanotify_init_event(&fee->fae,0,FS_ERROR);fee->sb_mark=sb_mark;sb_mark->fee_slot=fee;++/* Mark the error slot ready to receive events. */+fanotify_reset_error_slot(fee);+}}
@@ -1459,6 +1489,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,fsid=&__fsid;}+if(mask&FAN_FS_ERROR&&mark_type!=FAN_MARK_FILESYSTEM){+ret=-EINVAL;+gotopath_put_and_out;+}+/* inode held in place by reference to path; group by fget on fd */if(mark_type==FAN_MARK_INODE)inode=path.dentry->d_inode;
@@ -88,9 +88,13 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \FAN_ATTRIB|FAN_MOVE_SELF|FAN_DELETE_SELF)+/* Events that can only be reported with data type FSNOTIFY_EVENT_ERROR */+#define FANOTIFY_ERROR_EVENTS (FAN_FS_ERROR)+/* Events that user can request to be notified on */#define FANOTIFY_EVENTS (FANOTIFY_PATH_EVENTS | \-FANOTIFY_INODE_EVENTS)+FANOTIFY_INODE_EVENTS|\+FANOTIFY_ERROR_EVENTS)/* Events that require a permission response from user */#define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:37
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we estimate a file handle size and
if it is insuficient, we report an invalid FID and increase the
prediction for the next error slot allocation.
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Use preallocated MAX_HANDLE_SZ FH buffer
- Report superblock errors with a zerolength INVALID FID (jan, amir)
---
fs/notify/fanotify/fanotify.c | 15 +++++++++++++++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 7 +++++++
3 files changed, 33 insertions(+)
@@ -223,6 +223,13 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */++__kernel_fsid_tfsid;/* FSID this error refers to. */++/* object_fh must be followed by the inline handle buffer. */+structfanotify_fhobject_fh;+/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */+unsignedchar_inline_fh_buf[MAX_HANDLE_SZ];};staticinlinestructfanotify_error_event*
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:43
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
@@ -220,6 +220,7 @@ FANOTIFY_NE(struct fanotify_event *event)structfanotify_error_event{structfanotify_eventfae;+s32error;/* Error reported by the Filesystem. */u32err_count;/* Suppressed errors count */structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */
@@ -342,6 +348,28 @@ static int process_access_response(struct fsnotify_group *group,return-ENOENT;}+staticsize_tcopy_error_info_to_user(structfanotify_event*event,+char__user*buf,intcount)+{+structfanotify_event_info_errorinfo;+structfanotify_error_event*fee=FANOTIFY_EE(event);++info.hdr.info_type=FAN_EVENT_INFO_TYPE_ERROR;+info.hdr.pad=0;+info.hdr.len=FANOTIFY_INFO_ERROR_LEN;++if(WARN_ON(count<info.hdr.len))+return-EFAULT;++info.error=fee->error;+info.error_count=fee->err_count;++if(copy_to_user(buf,&info,sizeof(info)))+return-EFAULT;++returninfo.hdr.len;+}+staticintcopy_info_to_user(__kernel_fsid_t*fsid,structfanotify_fh*fh,intinfo_type,constchar*name,size_tname_len,char__user*buf,size_tcount)
@@ -505,6 +533,14 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,if(f)fd_install(fd,f);+if(fanotify_is_error_event(event->mask)){+ret=copy_error_info_to_user(event,buf,count);+if(ret<0)+gotoout_close_fd;+buf+=ret;+count-=ret;+}+/* Event info records order is: dir fid + name, child fid */if(fanotify_event_dir_fh_len(event)){info_type=info->name_len?FAN_EVENT_INFO_TYPE_DFID_NAME:
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:48
Send a FS_ERROR message via fsnotify to a userspace monitoring tool
whenever a ext4 error condition is triggered. This follows the existing
error conditions in ext4, so it is hooked to the ext4_error* functions.
It also follows the current dmesg reporting in the format. The
filesystem message is composed mostly by the string that would be
otherwise printed in dmesg.
A new ext4 specific record format is exposed in the uapi, such that a
monitoring tool knows what to expect when listening errors of an ext4
filesystem.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
fs/ext4/super.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-12 21:41:56
Document the FAN_FS_ERROR event for user administrators and user space
developers.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes Since v4:
- Update documentation about reporting non-file error.
Changes Since v3:
- Move FAN_FS_ERROR notification into a subsection of the file.
Changes Since v2:
- NTR
Changes since v1:
- Drop references to location record
- Explain that the inode field is optional
- Explain we are reporting only the first error
---
.../admin-guide/filesystem-monitoring.rst | 70 +++++++++++++++++++
Documentation/admin-guide/index.rst | 1 +
2 files changed, 71 insertions(+)
create mode 100644 Documentation/admin-guide/filesystem-monitoring.rst
@@ -0,0 +1,70 @@+.. SPDX-License-Identifier: GPL-2.0++====================================+File system Monitoring with fanotify+====================================++File system Error Reporting+===========================++fanotify supports the FAN_FS_ERROR mark for file system-wide error+reporting. It is meant to be used by file system health monitoring+daemons who listen on that interface and take actions (notify sysadmin,+start recovery) when a file system problem is detected by the kernel.++By design, A FAN_FS_ERROR notification exposes sufficient information for a+monitoring tool to know a problem in the file system has happened. It+doesn't necessarily provide a user space application with semantics to+verify an IO operation was successfully executed. That is outside of+scope of this feature. Instead, it is only meant as a framework for+early file system problem detection and reporting recovery tools.++When a file system operation fails, it is common for dozens of kernel+errors to cascade after the initial failure, hiding the original failure+log, which is usually the most useful debug data to troubleshoot the+problem. For this reason, FAN_FS_ERROR only reports the first error that+occurred since the last notification, and it simply counts addition+errors. This ensures that the most important piece of error information+is never lost.++FAN_FS_ERROR requires the fanotify group to be setup with the+FAN_REPORT_FID flag.++At the time of this writing, the only file system that emits FAN_FS_ERROR+notifications is Ext4.++A user space example code is provided at ``samples/fanotify/fs-monitor.c``.++A FAN_FS_ERROR Notification has the following format::++ [ Notification Metadata (Mandatory) ]+ [ Generic Error Record (Mandatory) ]+ [ FID record (Mandatory) ]++Generic error record+--------------------++The generic error record provides enough information for a file system+agnostic tool to learn about a problem in the file system, without+providing any additional details about the problem. This record is+identified by ``struct fanotify_event_info_header.info_type`` being set+to FAN_EVENT_INFO_TYPE_ERROR.++ struct fanotify_event_info_error {+ struct fanotify_event_info_header hdr;+ __s32 error;+ __u32 error_count;+ };++The `error` field identifies the type of error. `error_count` count+tracks the number of errors that occurred and were suppressed to+preserve the original error, since the last notification.++FID record+----------++The FID record can be used to uniquely identify the inode that triggered+the error through the combination of fsid and file handle. A file system+specific application can use that information to attempt a recovery+procedure. Errors that are not related to an inode are reported with an+empty file handle, with type FILEID_INVALID.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 07:28:40
On Fri, Aug 13, 2021 at 12:40 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Split out the final bits of struct fsnotify_mark->flags for use by a
backend.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Changes since v1:
- turn consts into defines (jan)
---
include/linux/fsnotify_backend.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 07:49:12
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Expose a new type of fsnotify event for filesystems to report errors for
userspace monitoring tools. fanotify will send this type of
notification for FAN_FS_ERROR events. This also introduce a helper for
generating the new event.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
---
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!
Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)
Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)
---
fs/notify/fsnotify.c | 3 +++
include/linux/fsnotify.h | 13 +++++++++++++
include/linux/fsnotify_backend.h | 18 +++++++++++++++++-
3 files changed, 33 insertions(+), 1 deletion(-)
@@ -42,6 +42,12 @@#define FS_UNMOUNT 0x00002000 /* inode on umount fs */#define FS_Q_OVERFLOW 0x00004000 /* Event queued overflowed */+#define FS_ERROR 0x00008000 /* Filesystem Error (fanotify) */++/*+*FS_IN_IGNOREDoverloadsFS_ERROR.Itisonlyusedinternallybyinotify+*whichdoesnotsupportFS_ERROR.+*/#define FS_IN_IGNORED 0x00008000 /* last inotify event here */#define FS_OPEN_PERM 0x00010000 /* open event in an permission hook */
@@ -95,7 +101,8 @@#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \FS_EVENTS_POSS_ON_CHILD|\FS_DELETE_SELF|FS_MOVE_SELF|FS_DN_RENAME|\-FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED)+FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED|\+FS_ERROR)/* Extra flags that may be reported with event or control handling of events */#define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 07:59:09
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Some file system events (i.e. FS_ERROR) might not be associated with an
inode. For these, it makes sense to associate them directly with the
super block of the file system they apply to. This patch allows the
event to be reported with a NULL inode, by recovering the superblock
directly from the data field, if needed.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
--
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
---
fs/notify/fsnotify.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
Irrelevant comment. sb must always be available from @data.
quoted hunk
+ * event. if both are non-NULL event may be reported to
+ * both.
* @cookie: inotify rename cookie
*/
int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
const struct path *path = fsnotify_data_path(data, data_type);
+ const struct super_block *sb = fsnotify_data_sb(data, data_type);
All the games with @data @inode and @dir args are irrelevant to this.
sb should always be available from @data and it does not matter
if fsnotify_data_inode() is the same as @inode, @dir or neither.
All those inodes are anyway on the same sb.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:00:07
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
FAN_FS_ERROR will report a file handle, but it is an unhashed event.
Allow passing a NULL hash to fanotify_encode_fh and avoid calculating
the hash if not needed.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Jan Kara <jack@suse.cz>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:28:02
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Instead of failing, encode an invalid file handle in fanotify_encode_fh
if no inode is provided. This bogus file handle will be reported by
FAN_FS_ERROR for non-inode errors.
When being reported to userspace, the length information is actually
reset and the handle cleaned up, such that userspace don't have the
visibility of the internal kernel representation of this null handle.
Also adjust the single caller that might rely on failure after passing
an empty inode.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic
---
fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++-----
fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
2 files changed, 41 insertions(+), 21 deletions(-)
@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,return-EFAULT;handle.handle_type=fh->type;-handle.handle_bytes=fh_len;++/* FILEID_INVALID handle type is reported without its f_handle. */+if(fh->type!=FILEID_INVALID)+handle.handle_bytes=fh_len;
I know I suggested those exact lines, but looking at the patch,
I think it would be better to do:
+ if (fh->type != FILEID_INVALID)
+ fh_len = 0;
handle.handle_bytes = fh_len;
quoted hunk
if (copy_to_user(buf, &handle, sizeof(handle)))
return -EFAULT;
@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, if (WARN_ON_ONCE(len < fh_len)) return -EFAULT;- /*- * For an inline fh and inline file name, copy through stack to exclude- * the copy from usercopy hardening protections.- */- fh_buf = fanotify_fh_buf(fh);- if (fh_len <= FANOTIFY_INLINE_FH_LEN) {- memcpy(bounce, fh_buf, fh_len);- fh_buf = bounce;+ if (fh->type != FILEID_INVALID) {
... and here: if (fh_len) {
quoted hunk
+ /*
+ * For an inline fh and inline file name, copy through
+ * stack to exclude the copy from usercopy hardening
+ * protections.
+ */
+ fh_buf = fanotify_fh_buf(fh);
+ if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+ memcpy(bounce, fh_buf, fh_len);
+ fh_buf = bounce;
+ }
+ if (copy_to_user(buf, fh_buf, fh_len))
+ return -EFAULT;
+ buf += fh_len;
+ len -= fh_len;
}
- if (copy_to_user(buf, fh_buf, fh_len))
- return -EFAULT;
-
- buf += fh_len;
- len -= fh_len;
if (name_len) {
/* Copy the filename with terminating null */
@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, } /* Pad with 0's */- WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);+ WARN_ON_ONCE(len < 0);
According to my calculations, FAN_FS_ERROR event with NULL_FH is expected
to get here with len == 4, so you can change this to:
WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN);
But first, I would like to get Jan's feedback on this concept of keeping
unneeded 4 bytes zero padding in reported event in case of NULL_FH
in order to keep the FID reporting code simpler.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:29:11
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Like inode events, FAN_FS_ERROR will require fid mode. Therefore,
convert the verification during fanotify_mark(2) to require fid for any
non-fd event. This means fid_mode will not only be required for inode
events, but for any event that doesn't provide a descriptor.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -81,6 +81,9 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */*/#define FANOTIFY_DIRENT_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE)+/* Events that can be reported with event->fd */+#define FANOTIFY_FD_EVENTS (FANOTIFY_PATH_EVENTS | FANOTIFY_PERM_EVENTS)+/* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \FAN_ATTRIB|FAN_MOVE_SELF|FAN_DELETE_SELF)--
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:29:42
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
FAN_FS_ERROR allows reporting of event type FS_ERROR to userspace, which
a mechanism to report file system wide problems via fanotify. This
commit preallocate userspace visible bits to match the FS_ERROR event.
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:40:43
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Error reporting needs to be done in an atomic context. This patch
introduces a single error slot for superblock marks that report the
FAN_FS_ERROR event, to be used during event submission.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)
---
fs/notify/fanotify/fanotify.c | 12 ++++++++++++
fs/notify/fanotify/fanotify.h | 13 +++++++++++++
fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
3 files changed, 54 insertions(+), 2 deletions(-)
@@ -216,6 +218,17 @@ FANOTIFY_NE(struct fanotify_event *event)returncontainer_of(event,structfanotify_name_event,fae);}+structfanotify_error_event{+structfanotify_eventfae;+structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */+};++staticinlinestructfanotify_error_event*+FANOTIFY_EE(structfanotify_event*event)+{+returncontainer_of(event,structfanotify_error_event,fae);+}+staticinline__kernel_fsid_t*fanotify_event_fsid(structfanotify_event*event){if(event->type==FANOTIFY_EVENT_TYPE_FID)
@@ -999,6 +1001,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,{structfsnotify_mark*fsn_mark;__u32added;+intret=0;mutex_lock(&group->mark_mutex);fsn_mark=fsnotify_find_mark(connp,group);
@@ -1009,13 +1012,37 @@ static int fanotify_add_mark(struct fsnotify_group *group,returnPTR_ERR(fsn_mark);}}++/*+*Erroreventsareallocatedpersuper-blockmarkonlyif+*strictlyneeded(i.e.FAN_FS_ERRORwasrequested).+*/+if(type==FSNOTIFY_OBJ_TYPE_SB&&!(flags&FAN_MARK_IGNORED_MASK)&&+(mask&FAN_FS_ERROR)){+structfanotify_sb_mark*sb_mark=FANOTIFY_SB_MARK(fsn_mark);++if(!sb_mark->fee_slot){+structfanotify_error_event*fee=+kzalloc(sizeof(*fee),GFP_KERNEL_ACCOUNT);+if(!fee){+ret=-ENOMEM;+gotoout;+}+fanotify_init_event(&fee->fae,0,FS_ERROR);+fee->sb_mark=sb_mark;
I think Jan wanted to avoid zalloc()?
Please use kmalloc() and init the rest of the fee-> members.
We do not need to fill the entire fh buf with zeroes.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 08:48:15
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -220,6 +220,7 @@ FANOTIFY_NE(struct fanotify_event *event)structfanotify_error_event{structfanotify_eventfae;+s32error;/* Error reported by the Filesystem. */u32err_count;/* Suppressed errors count */structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */
@@ -342,6 +348,28 @@ static int process_access_response(struct fsnotify_group *group,return-ENOENT;}+staticsize_tcopy_error_info_to_user(structfanotify_event*event,+char__user*buf,intcount)+{+structfanotify_event_info_errorinfo;+structfanotify_error_event*fee=FANOTIFY_EE(event);++info.hdr.info_type=FAN_EVENT_INFO_TYPE_ERROR;+info.hdr.pad=0;+info.hdr.len=FANOTIFY_INFO_ERROR_LEN;++if(WARN_ON(count<info.hdr.len))+return-EFAULT;++info.error=fee->error;+info.error_count=fee->err_count;++if(copy_to_user(buf,&info,sizeof(info)))+return-EFAULT;++returninfo.hdr.len;+}+staticintcopy_info_to_user(__kernel_fsid_t*fsid,structfanotify_fh*fh,intinfo_type,constchar*name,size_tname_len,char__user*buf,size_tcount)
@@ -505,6 +533,14 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,if(f)fd_install(fd,f);+if(fanotify_is_error_event(event->mask)){+ret=copy_error_info_to_user(event,buf,count);+if(ret<0)+gotoout_close_fd;+buf+=ret;+count-=ret;+}+/* Event info records order is: dir fid + name, child fid */if(fanotify_event_dir_fh_len(event)){info_type=info->name_len?FAN_EVENT_INFO_TYPE_DFID_NAME:
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 09:00:50
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we estimate a file handle size and
if it is insuficient, we report an invalid FID and increase the
prediction for the next error slot allocation.
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Use preallocated MAX_HANDLE_SZ FH buffer
- Report superblock errors with a zerolength INVALID FID (jan, amir)
---
fs/notify/fanotify/fanotify.c | 15 +++++++++++++++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 7 +++++++
3 files changed, 33 insertions(+)
@@ -223,6 +223,13 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */++__kernel_fsid_tfsid;/* FSID this error refers to. */++/* object_fh must be followed by the inline handle buffer. */+structfanotify_fhobject_fh;+/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */+unsignedchar_inline_fh_buf[MAX_HANDLE_SZ];};staticinlinestructfanotify_error_event*
I would go with:
size_t len = offsetof(struct fanotify_error_event, _inline_fh_buf)
+ fee->object_fh.len);
memcpy(error_on_stack, fee, len);
But maybe it's just me, so I don't insist.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 09:03:39
On Fri, Aug 13, 2021 at 12:00 PM Amir Goldstein [off-list ref] wrote:
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we estimate a file handle size and
if it is insuficient, we report an invalid FID and increase the
prediction for the next error slot allocation.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-13 09:36:06
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Wire up FAN_FS_ERROR in the fanotify_mark syscall. The event can only
be requested for the entire filesystem, thus it requires the
FAN_MARK_FILESYSTEM.
Please split the Wire-up to fanotify_mark syscall into a separate patch applied
after patches that implement the report of event info records.
quoted hunk
FAN_FS_ERROR has to be handled slightly differently from other events
because it needs to be submitted in an atomic context, using
preallocated memory. This patch implements the submission path by only
storing the first error event that happened in the slot (userspace
resets the slot by reading the event).
Extra error events happening when the slot is occupied are merged to the
original report, and the only information keep for these extra errors is
an accumulator counting the number of events, which is part of the
record reported back to userspace.
Reporting only the first event should be fine, since when a FS error
happens, a cascade of error usually follows, but the most meaningful
information is (usually) on the first erro.
The event dequeueing is also a bit special to avoid losing events. Since
event merging only happens while the event is queued, there is a window
between when an error event is dequeued (notification_lock is dropped)
until it is reset (.free_event()) where the slot is full, but no merges
can happen.
The proposed solution is to copy the event to the stack prior to
dropping the lock. This way, if a new event arrives in the time between
the event was dequeued and the time it resets, the new errors will still
be logged and merged in the recently freed slot.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Copy to stack instead of replacing the fee slot(jan)
- prepare error slot outside of the notification lock(jan)
Changes since v4:
- Split parts to earlier patches (amir)
- Simplify fanotify entry replacement
- Update handle size prediction on overflow
Changes since v3:
- Convert WARN_ON to pr_warn (amir)
- Remove unecessary READ/WRITE_ONCE (amir)
- Alloc with GFP_KERNEL_ACCOUNT(amir)
- Simplify flags on mark allocation (amir)
- Avoid atomic set of error_count (amir)
- Simplify rules when merging error_event (amir)
- Allocate new error_event on get_one_event (amir)
- Report superblock error with invalid FH (amir,jan)
Changes since v2:
- Support and equire FID mode (amir)
- Goto error path instead of early return (amir)
- Simplify get_one_event (me)
- Base merging on error_count
- drop fanotify_queue_error_event
Changes since v1:
- Pass dentry to fanotify_check_fsid (Amir)
- FANOTIFY_EVENT_TYPE_ERROR -> FANOTIFY_EVENT_TYPE_FS_ERROR
- Merge previous patch into it
- Use a single slot
- Move fanotify_mark.error_event definition to this commit
- Rename FAN_ERROR -> FAN_FS_ERROR
- Restrict FAN_FS_ERROR to FAN_MARK_FILESYSTEM
---
fs/notify/fanotify/fanotify.c | 57 +++++++++++++++++++++++++++++-
fs/notify/fanotify/fanotify.h | 21 +++++++++++
fs/notify/fanotify/fanotify_user.c | 39 ++++++++++++++++++--
include/linux/fanotify.h | 6 +++-
4 files changed, 119 insertions(+), 4 deletions(-)
Please add commentary to explain why logic is before merge()/insert().
+ spin_unlock(&group->notification_lock);
+
+ fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
+
+ if (fsnotify_insert_event(group, &fee->fae.fse,
+ NULL, fanotify_insert_error_event)) {
+ /*
+ * Even if an error occurred, an overflow event is
+ * queued. Just reset the error count and succeed.
+ */
+ spin_lock(&group->notification_lock);
+ fanotify_reset_error_slot(fee);
+ spin_unlock(&group->notification_lock);
This feels racy.
I think that fanotify_reset_error_slot() should WARN about
trying to reset a queued error event and here we need to
check that fee was not queued while we dropped the lock.
And I am not convinced about correctness of incrementing
err_count while the lock is dropped.
Need to see the commentary.
quoted hunk
+ }
+
+ return 0;
+}
+
/*
* Add an event to hash table for faster merge.
*/
@@ -857,10 +909,13 @@ static void fanotify_free_name_event(struct fanotify_event *event) static void fanotify_free_error_event(struct fanotify_event *event) {+ struct fanotify_error_event *fee = FANOTIFY_EE(event);+ /* * The actual event is tied to a mark, and is released on mark * removal */+ fsnotify_put_mark(&fee->sb_mark->fsn_mark); } static void fanotify_free_event(struct fsnotify_event *fsn_event)
@@ -1031,6 +1057,10 @@ static int fanotify_add_mark(struct fsnotify_group *group,fanotify_init_event(&fee->fae,0,FS_ERROR);fee->sb_mark=sb_mark;sb_mark->fee_slot=fee;++/* Mark the error slot ready to receive events. */+fanotify_reset_error_slot(fee);+}}
@@ -1459,6 +1489,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,fsid=&__fsid;}+if(mask&FAN_FS_ERROR&&mark_type!=FAN_MARK_FILESYSTEM){+ret=-EINVAL;+gotopath_put_and_out;+}+
Split to Wire-up patch please.
quoted hunk
/* inode held in place by reference to path; group by fget on fd */
if (mark_type == FAN_MARK_INODE)
inode = path.dentry->d_inode;
@@ -88,9 +88,13 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \FAN_ATTRIB|FAN_MOVE_SELF|FAN_DELETE_SELF)+/* Events that can only be reported with data type FSNOTIFY_EVENT_ERROR */+#define FANOTIFY_ERROR_EVENTS (FAN_FS_ERROR)+/* Events that user can request to be notified on */#define FANOTIFY_EVENTS (FANOTIFY_PATH_EVENTS | \-FANOTIFY_INODE_EVENTS)+FANOTIFY_INODE_EVENTS|\+FANOTIFY_ERROR_EVENTS)
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 13:17:50
On Fri 13-08-21 10:28:27, Amir Goldstein wrote:
On Fri, Aug 13, 2021 at 12:40 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Split out the final bits of struct fsnotify_mark->flags for use by a
backend.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Changes since v1:
- turn consts into defines (jan)
---
include/linux/fsnotify_backend.h | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
@@ -398,9 +412,7 @@ struct fsnotify_mark {structfsnotify_mark_connector*connector;/* Events types to ignore [mark->lock, group->mark_mutex] */__u32ignored_mask;-#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY 0x01-#define FSNOTIFY_MARK_FLAG_ALIVE 0x02-#define FSNOTIFY_MARK_FLAG_ATTACHED 0x04+/* Upper bits [31:PRIVATE_FLAGS] are reserved for backend usage */
I don't understand what [31:PRIVATE_FLAGS] means
I think it should be [FSN_MARK_PRIVATE_FLAGS:31] (identifying a range of
bits). I'd maybe write just "Bits starting from FSN_MARK_PRIVATE_FLAGS are
reserved for backend usage". With this fixed feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 13:20:17
On Thu 12-08-21 17:39:54, Gabriel Krisman Bertazi wrote:
FAN_FS_ERROR will require an error structure to be stored per mark.
But, since FAN_FS_ERROR doesn't apply to inode/mount marks, it should
suffice to only expose this information for superblock marks. Therefore,
wrap this kind of marks into a container and plumb it for the future.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
---
Changes since v5:
- turn the flag bits into defines (jan)
- don't use zalloc for consistency (jan)
Changes since v2:
- Move mark initialization to fanotify_alloc_mark (Amir)
Changes since v1:
- Only extend superblock marks (Amir)
---
fs/notify/fanotify/fanotify.c | 10 ++++++--
fs/notify/fanotify/fanotify.h | 20 ++++++++++++++++
fs/notify/fanotify/fanotify_user.c | 38 ++++++++++++++++++++++++++++--
3 files changed, 64 insertions(+), 4 deletions(-)
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 13:25:16
On Thu 12-08-21 17:39:59, Gabriel Krisman Bertazi wrote:
Expose a new type of fsnotify event for filesystems to report errors for
userspace monitoring tools. fanotify will send this type of
notification for FAN_FS_ERROR events. This also introduce a helper for
generating the new event.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
---
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!
Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)
Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)
---
fs/notify/fsnotify.c | 3 +++
include/linux/fsnotify.h | 13 +++++++++++++
include/linux/fsnotify_backend.h | 18 +++++++++++++++++-
3 files changed, 33 insertions(+), 1 deletion(-)
@@ -42,6 +42,12 @@#define FS_UNMOUNT 0x00002000 /* inode on umount fs */#define FS_Q_OVERFLOW 0x00004000 /* Event queued overflowed */+#define FS_ERROR 0x00008000 /* Filesystem Error (fanotify) */++/*+*FS_IN_IGNOREDoverloadsFS_ERROR.Itisonlyusedinternallybyinotify+*whichdoesnotsupportFS_ERROR.+*/#define FS_IN_IGNORED 0x00008000 /* last inotify event here */#define FS_OPEN_PERM 0x00010000 /* open event in an permission hook */
@@ -95,7 +101,8 @@#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \FS_EVENTS_POSS_ON_CHILD|\FS_DELETE_SELF|FS_MOVE_SELF|FS_DN_RENAME|\-FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED)+FS_UNMOUNT|FS_Q_OVERFLOW|FS_IN_IGNORED|\+FS_ERROR)/* Extra flags that may be reported with event or control handling of events */#define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 14:07:00
On Fri 13-08-21 11:27:48, Amir Goldstein wrote:
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Instead of failing, encode an invalid file handle in fanotify_encode_fh
if no inode is provided. This bogus file handle will be reported by
FAN_FS_ERROR for non-inode errors.
When being reported to userspace, the length information is actually
reset and the handle cleaned up, such that userspace don't have the
visibility of the internal kernel representation of this null handle.
Also adjust the single caller that might rely on failure after passing
an empty inode.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic
---
fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++-----
fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
2 files changed, 41 insertions(+), 21 deletions(-)
@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,return-EFAULT;handle.handle_type=fh->type;-handle.handle_bytes=fh_len;++/* FILEID_INVALID handle type is reported without its f_handle. */+if(fh->type!=FILEID_INVALID)+handle.handle_bytes=fh_len;
I know I suggested those exact lines, but looking at the patch,
I think it would be better to do:
+ if (fh->type != FILEID_INVALID)
+ fh_len = 0;
handle.handle_bytes = fh_len;
quoted
if (copy_to_user(buf, &handle, sizeof(handle)))
return -EFAULT;
@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, if (WARN_ON_ONCE(len < fh_len)) return -EFAULT;- /*- * For an inline fh and inline file name, copy through stack to exclude- * the copy from usercopy hardening protections.- */- fh_buf = fanotify_fh_buf(fh);- if (fh_len <= FANOTIFY_INLINE_FH_LEN) {- memcpy(bounce, fh_buf, fh_len);- fh_buf = bounce;+ if (fh->type != FILEID_INVALID) {
... and here: if (fh_len) {
quoted
+ /*
+ * For an inline fh and inline file name, copy through
+ * stack to exclude the copy from usercopy hardening
+ * protections.
+ */
+ fh_buf = fanotify_fh_buf(fh);
+ if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+ memcpy(bounce, fh_buf, fh_len);
+ fh_buf = bounce;
+ }
+ if (copy_to_user(buf, fh_buf, fh_len))
+ return -EFAULT;
+ buf += fh_len;
+ len -= fh_len;
}
- if (copy_to_user(buf, fh_buf, fh_len))
- return -EFAULT;
-
- buf += fh_len;
- len -= fh_len;
if (name_len) {
/* Copy the filename with terminating null */
@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, } /* Pad with 0's */- WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);+ WARN_ON_ONCE(len < 0);
According to my calculations, FAN_FS_ERROR event with NULL_FH is expected
to get here with len == 4, so you can change this to:
WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN);
But first, I would like to get Jan's feedback on this concept of keeping
unneeded 4 bytes zero padding in reported event in case of NULL_FH
in order to keep the FID reporting code simpler.
Dunno, it still seems like quite some complications (simple ones but
non-trivial amount of them) for what is rather a corner case. What if we
*internally* propagated the information that there's no inode info with
FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(),
fanotify_encode_fh(), or fanotify_alloc_name_event(). In
copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's
all. No useless padding, no specialcasing of copying etc. Am I missing
something?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-16 15:55:50
On Mon, Aug 16, 2021 at 5:07 PM Jan Kara [off-list ref] wrote:
On Fri 13-08-21 11:27:48, Amir Goldstein wrote:
quoted
On Fri, Aug 13, 2021 at 12:41 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Instead of failing, encode an invalid file handle in fanotify_encode_fh
if no inode is provided. This bogus file handle will be reported by
FAN_FS_ERROR for non-inode errors.
When being reported to userspace, the length information is actually
reset and the handle cleaned up, such that userspace don't have the
visibility of the internal kernel representation of this null handle.
Also adjust the single caller that might rely on failure after passing
an empty inode.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic
---
fs/notify/fanotify/fanotify.c | 27 ++++++++++++++++++-----
fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
2 files changed, 41 insertions(+), 21 deletions(-)
@@ -360,7 +360,10 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,return-EFAULT;handle.handle_type=fh->type;-handle.handle_bytes=fh_len;++/* FILEID_INVALID handle type is reported without its f_handle. */+if(fh->type!=FILEID_INVALID)+handle.handle_bytes=fh_len;
I know I suggested those exact lines, but looking at the patch,
I think it would be better to do:
+ if (fh->type != FILEID_INVALID)
+ fh_len = 0;
handle.handle_bytes = fh_len;
quoted
if (copy_to_user(buf, &handle, sizeof(handle)))
return -EFAULT;
@@ -369,20 +372,22 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, if (WARN_ON_ONCE(len < fh_len)) return -EFAULT;- /*- * For an inline fh and inline file name, copy through stack to exclude- * the copy from usercopy hardening protections.- */- fh_buf = fanotify_fh_buf(fh);- if (fh_len <= FANOTIFY_INLINE_FH_LEN) {- memcpy(bounce, fh_buf, fh_len);- fh_buf = bounce;+ if (fh->type != FILEID_INVALID) {
... and here: if (fh_len) {
quoted
+ /*
+ * For an inline fh and inline file name, copy through
+ * stack to exclude the copy from usercopy hardening
+ * protections.
+ */
+ fh_buf = fanotify_fh_buf(fh);
+ if (fh_len <= FANOTIFY_INLINE_FH_LEN) {
+ memcpy(bounce, fh_buf, fh_len);
+ fh_buf = bounce;
+ }
+ if (copy_to_user(buf, fh_buf, fh_len))
+ return -EFAULT;
+ buf += fh_len;
+ len -= fh_len;
}
- if (copy_to_user(buf, fh_buf, fh_len))
- return -EFAULT;
-
- buf += fh_len;
- len -= fh_len;
if (name_len) {
/* Copy the filename with terminating null */
@@ -398,7 +403,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, } /* Pad with 0's */- WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);+ WARN_ON_ONCE(len < 0);
According to my calculations, FAN_FS_ERROR event with NULL_FH is expected
to get here with len == 4, so you can change this to:
WARN_ON_ONCE(len < 0 || len > FANOTIFY_EVENT_ALIGN);
But first, I would like to get Jan's feedback on this concept of keeping
unneeded 4 bytes zero padding in reported event in case of NULL_FH
in order to keep the FID reporting code simpler.
Dunno, it still seems like quite some complications (simple ones but
non-trivial amount of them) for what is rather a corner case. What if we
*internally* propagated the information that there's no inode info with
FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(),
fanotify_encode_fh(), or fanotify_alloc_name_event(). In
copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's
all. No useless padding, no specialcasing of copying etc. Am I missing
something?
I am perfectly fine with encoding "no inode" with FILEID_ROOT internally.
It's already the value used by fanotify_encode_fh() in upstream.
However, if we use zero len internally, we need to pass fh_type to
fanotify_fid_info_len() and special case FILEID_ROOT in order to
take FANOTIFY_FID_INFO_HDR_LEN into account.
And special case fanotify_event_object_fh_len() in
fanotify_event_info_len() and in copy_info_records_to_user().
Or maybe I am missing something....
Thanks,
Amir.
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 15:58:03
On Thu 12-08-21 17:40:04, Gabriel Krisman Bertazi wrote:
quoted hunk
Error reporting needs to be done in an atomic context. This patch
introduces a single error slot for superblock marks that report the
FAN_FS_ERROR event, to be used during event submission.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes v5:
- Restore mark references. (jan)
- Tie fee slot to the mark lifetime.(jan)
- Don't reallocate event(jan)
---
fs/notify/fanotify/fanotify.c | 12 ++++++++++++
fs/notify/fanotify/fanotify.h | 13 +++++++++++++
fs/notify/fanotify/fanotify_user.c | 31 ++++++++++++++++++++++++++++--
3 files changed, 54 insertions(+), 2 deletions(-)
I was pondering about the lifetime rules some more. This is also related to
patch 16/21 but I'll comment here. When we hold mark ref from queued event,
we introduce a subtle race into group destruction logic. There we first
evict all marks, wait for them to be destroyed by worker thread after SRCU
period expires, and then we remove queued events. When we hold mark
reference from an event we break this as mark will exist until the event is
dequeued and then group can get freed before we actually free the mark and
so mark freeing can hit use-after-free issues.
So we'll have to do this a bit differently. I have two options:
1) Instead of preallocating events explicitely like this, we could setup a
mempool to allocate error events from for each notification group. We would
resize the mempool when adding error mark so that it has as many reserved
events as error marks. Upside is error events will be much less special -
no special lifetime rules. We'd just need to setup & resize the mempool. We
would also have to provide proper merge function for error events (to merge
events from the same sb). Also there will be limitation of number of error
marks per group because mempools use kmalloc() for an array tracking
reserved events. But we could certainly manage 512, likely 1024 error marks
per notification group.
2) We would keep attaching event to mark as currently. As far as I have
checked the event doesn't actually need a back-ref to sb_mark. It is
really only used for mark reference taking (and then to get to sb from
fanotify_handle_error_event() but we can certainly get to sb by easier
means there). So I would just remove that. What we still need to know in
fanotify_free_error_event() though is whether the sb_mark is still alive or
not. If it is alive, we leave the event alone, otherwise we need to free it.
So we need a mark_alive flag in the error event and then do in ->freeing_mark
callback something like:
if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);
### /* Maybe we could use mark->lock for this? */
spin_lock(&group->notification_lock);
if (fa_mark->fee_slot) {
if (list_empty(&fa_mark->fee_slot->fae.fse.list)) {
kfree(fa_mark->fee_slot);
fa_mark->fee_slot = NULL;
} else {
fa_mark->fee_slot->mark_alive = 0;
}
}
spin_unlock(&group->notification_lock);
}
And then when queueing and dequeueing event we would have to carefully
check what is the mark & event state under appropriate lock (because
->handle_event() callbacks can see marks on the way to be destroyed as they
are protected just by SRCU).
quoted hunk
@@ -1009,13 +1012,37 @@ static int fanotify_add_mark(struct fsnotify_group *group, return PTR_ERR(fsn_mark); } }++ /*+ * Error events are allocated per super-block mark only if+ * strictly needed (i.e. FAN_FS_ERROR was requested).+ */+ if (type == FSNOTIFY_OBJ_TYPE_SB && !(flags & FAN_MARK_IGNORED_MASK) &&+ (mask & FAN_FS_ERROR)) {+ struct fanotify_sb_mark *sb_mark = FANOTIFY_SB_MARK(fsn_mark);++ if (!sb_mark->fee_slot) {+ struct fanotify_error_event *fee =+ kzalloc(sizeof(*fee), GFP_KERNEL_ACCOUNT);
Careful here. The 'sb_mark' can be already attached to sb and events can
walk it. So we should make sure these readers don't see half initialized
'fee' due to CPU reordering stores. So this needs to be protected by the
same lock that we use when generating error event.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 16:11:51
On Mon 16-08-21 18:54:58, Amir Goldstein wrote:
On Mon, Aug 16, 2021 at 5:07 PM Jan Kara [off-list ref] wrote:
quoted
Dunno, it still seems like quite some complications (simple ones but
non-trivial amount of them) for what is rather a corner case. What if we
*internally* propagated the information that there's no inode info with
FILEID_ROOT fh? That means: No changes to fanotify_encode_fh_len(),
fanotify_encode_fh(), or fanotify_alloc_name_event(). In
copy_info_to_user() we just mangle FILEID_ROOT to FILEID_INVALID and that's
all. No useless padding, no specialcasing of copying etc. Am I missing
something?
I am perfectly fine with encoding "no inode" with FILEID_ROOT internally.
It's already the value used by fanotify_encode_fh() in upstream.
However, if we use zero len internally, we need to pass fh_type to
fanotify_fid_info_len() and special case FILEID_ROOT in order to
take FANOTIFY_FID_INFO_HDR_LEN into account.
And special case fanotify_event_object_fh_len() in
fanotify_event_info_len() and in copy_info_records_to_user().
Right, this will need some tweaking. I would actually leave
fanotify_fid_info_len() alone, just have in fanotify_event_info_len()
something like:
- if (fh_len)
+ if (fh_len || fanotify_event_needs_fsid(event))
and similarly in copy_info_records_to_user():
- if (fanotify_event_object_fh_len(event)) {
+ if (fanotify_event_object_fh_len(event) ||
+ fanotify_event_needs_fsid(event)) {
And that should be all that's needed as far as I'm reading the code.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 16:18:35
On Thu 12-08-21 17:40:06, Gabriel Krisman Bertazi wrote:
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we estimate a file handle size and
if it is insuficient, we report an invalid FID and increase the
prediction for the next error slot allocation.
This needs updating. The code now uses MAX_HANDLE_SZ...
quoted hunk
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Use preallocated MAX_HANDLE_SZ FH buffer
- Report superblock errors with a zerolength INVALID FID (jan, amir)
---
fs/notify/fanotify/fanotify.c | 15 +++++++++++++++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 7 +++++++
3 files changed, 33 insertions(+)
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 16:23:05
On Thu 12-08-21 17:40:07, Gabriel Krisman Bertazi wrote:
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
@@ -220,6 +220,7 @@ FANOTIFY_NE(struct fanotify_event *event)structfanotify_error_event{structfanotify_eventfae;+s32error;/* Error reported by the Filesystem. */u32err_count;/* Suppressed errors count */structfanotify_sb_mark*sb_mark;/* Back reference to the mark. */
@@ -342,6 +348,28 @@ static int process_access_response(struct fsnotify_group *group,return-ENOENT;}+staticsize_tcopy_error_info_to_user(structfanotify_event*event,+char__user*buf,intcount)+{+structfanotify_event_info_errorinfo;+structfanotify_error_event*fee=FANOTIFY_EE(event);++info.hdr.info_type=FAN_EVENT_INFO_TYPE_ERROR;+info.hdr.pad=0;+info.hdr.len=FANOTIFY_INFO_ERROR_LEN;++if(WARN_ON(count<info.hdr.len))+return-EFAULT;++info.error=fee->error;+info.error_count=fee->err_count;++if(copy_to_user(buf,&info,sizeof(info)))+return-EFAULT;++returninfo.hdr.len;+}+staticintcopy_info_to_user(__kernel_fsid_t*fsid,structfanotify_fh*fh,intinfo_type,constchar*name,size_tname_len,char__user*buf,size_tcount)
@@ -505,6 +533,14 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,if(f)fd_install(fd,f);+if(fanotify_is_error_event(event->mask)){+ret=copy_error_info_to_user(event,buf,count);+if(ret<0)+gotoout_close_fd;+buf+=ret;+count-=ret;+}+/* Event info records order is: dir fid + name, child fid */if(fanotify_event_dir_fh_len(event)){info_type=info->name_len?FAN_EVENT_INFO_TYPE_DFID_NAME:
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 16:26:21
On Thu 12-08-21 17:40:08, Gabriel Krisman Bertazi wrote:
Send a FS_ERROR message via fsnotify to a userspace monitoring tool
whenever a ext4 error condition is triggered. This follows the existing
error conditions in ext4, so it is hooked to the ext4_error* functions.
It also follows the current dmesg reporting in the format. The
filesystem message is composed mostly by the string that would be
otherwise printed in dmesg.
A new ext4 specific record format is exposed in the uapi, such that a
monitoring tool knows what to expect when listening errors of an ext4
filesystem.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
fs/ext4/super.c | 8 ++++++++
1 file changed, 8 insertions(+)
Does it make sense to report root inode here? ext4_std_error() gets
generally used for filesystem-wide errors.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-16 16:40:55
On Thu 12-08-21 17:40:10, Gabriel Krisman Bertazi wrote:
Document the FAN_FS_ERROR event for user administrators and user space
developers.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -0,0 +1,70 @@+.. SPDX-License-Identifier: GPL-2.0++====================================+File system Monitoring with fanotify+====================================++File system Error Reporting+===========================++fanotify supports the FAN_FS_ERROR mark for file system-wide error
^ Capital 'F'. ^^^ I'd rather write "event type".
+reporting. It is meant to be used by file system health monitoring
+daemons who listen on that interface and take actions (notify sysadmin,
^^^ which ^^^^^^^^^^^^^^^^^ for these events
+start recovery) when a file system problem is detected by the kernel.
+
+By design, A FAN_FS_ERROR notification exposes sufficient information for a
+monitoring tool to know a problem in the file system has happened. It
+doesn't necessarily provide a user space application with semantics to
+verify an IO operation was successfully executed. That is outside of
+scope of this feature. Instead, it is only meant as a framework for
+early file system problem detection and reporting recovery tools.
+
+When a file system operation fails, it is common for dozens of kernel
+errors to cascade after the initial failure, hiding the original failure
+log, which is usually the most useful debug data to troubleshoot the
+problem. For this reason, FAN_FS_ERROR only reports the first error that
+occurred since the last notification, and it simply counts addition
^^^ additional
+errors. This ensures that the most important piece of error information
+is never lost.
+
+FAN_FS_ERROR requires the fanotify group to be setup with the
+FAN_REPORT_FID flag.
+
+At the time of this writing, the only file system that emits FAN_FS_ERROR
+notifications is Ext4.
+
+A user space example code is provided at ``samples/fanotify/fs-monitor.c``.
+
+A FAN_FS_ERROR Notification has the following format::
+
+ [ Notification Metadata (Mandatory) ]
+ [ Generic Error Record (Mandatory) ]
+ [ FID record (Mandatory) ]
+
+Generic error record
+--------------------
+
+The generic error record provides enough information for a file system
+agnostic tool to learn about a problem in the file system, without
+providing any additional details about the problem. This record is
+identified by ``struct fanotify_event_info_header.info_type`` being set
+to FAN_EVENT_INFO_TYPE_ERROR.
+
+ struct fanotify_event_info_error {
+ struct fanotify_event_info_header hdr;
+ __s32 error;
+ __u32 error_count;
+ };
+
+The `error` field identifies the type of error. `error_count` count
+tracks the number of errors that occurred and were suppressed to
+preserve the original error, since the last notification.
So is 'error' expected to be errno? Or is that some fs-specific error
identifier? Will it be positive (i.e. real errno) or negative (as errno is
usually passed in the kernel)? I think it should be specified here.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-08-16 21:41:09
On Thu, Aug 12, 2021 at 05:40:07PM -0400, Gabriel Krisman Bertazi wrote:
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
My apologies for not having time to review this patchset since it was
redesigned to use fanotify. Someday it would be helpful to be able to
export more detailed error reports from XFS, but as I'm not ready to
move forward and write that today, I'll try to avoid derailling this at
the last minute.
Eventually, XFS might want to be able to report errors in file data,
file metadata, allocation group metadata, and whole-filesystem metadata.
Userspace can already gather reports from XFS about corruptions reported
by the online fsck code (see xfs_health.c).
I /think/ we could subclass the file error structure that you've
provided like so:
struct fanotify_event_info_xfs_filesystem_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* quotas, realtime bitmap, etc. */
};
struct fanotify_event_info_xfs_perag_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* agf, agi, agfl, bno btree, ino btree, etc. */
__u32 agno; /* allocation group number */
};
struct fanotify_event_info_xfs_file_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* extent map, dir, attr, etc. */
__u64 offset; /* file data offset, if applicable */
__u64 length; /* file data length, if applicable */
};
(A real XFS implementation might have one structure with the type code
providing for a tagged union or something; I split it into three
separate structs here to avoid confusing things.)
I have three questions at this point:
1) What's the maximum size of a fanotify event structure? None of these
structures exceed 36 bytes, which I hope will fit in whatever size
constraints?
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
--D
From: Jan Kara <jack@suse.cz> Date: 2021-08-17 09:05:46
On Mon 16-08-21 14:41:03, Darrick J. Wong wrote:
On Thu, Aug 12, 2021 at 05:40:07PM -0400, Gabriel Krisman Bertazi wrote:
quoted
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
My apologies for not having time to review this patchset since it was
redesigned to use fanotify. Someday it would be helpful to be able to
export more detailed error reports from XFS, but as I'm not ready to
move forward and write that today, I'll try to avoid derailling this at
the last minute.
I think we are not quite there and tweaking the passed structure is easy
enough so no worries. Eventually, passing some filesystem-specific blob
together with the event was the plan AFAIR. You're right now is a good
moment to think how exactly we want that passed.
Eventually, XFS might want to be able to report errors in file data,
file metadata, allocation group metadata, and whole-filesystem metadata.
Userspace can already gather reports from XFS about corruptions reported
by the online fsck code (see xfs_health.c).
Yes, although note that the current plan is that we currently have only one
error event queue, others are just added to error_count until the event is
fetched by userspace (on the grounds that the first error is usually the
most meaningful, the others are usually just cascading problems). But I'm
not sure if this scheme would be suitable for online fsck usecase since we
may discard even valid independent errors this way.
I /think/ we could subclass the file error structure that you've
provided like so:
struct fanotify_event_info_xfs_filesystem_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* quotas, realtime bitmap, etc. */
};
struct fanotify_event_info_xfs_perag_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* agf, agi, agfl, bno btree, ino btree, etc. */
__u32 agno; /* allocation group number */
};
struct fanotify_event_info_xfs_file_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* extent map, dir, attr, etc. */
__u64 offset; /* file data offset, if applicable */
__u64 length; /* file data length, if applicable */
};
(A real XFS implementation might have one structure with the type code
providing for a tagged union or something; I split it into three
separate structs here to avoid confusing things.)
The structure of fanotify event as passed to userspace generally is:
struct fanotify_event_metadata {
__u32 event_len;
__u8 vers;
__u8 reserved;
__u16 metadata_len;
__aligned_u64 mask;
__s32 fd;
__s32 pid;
};
If event_len is > sizeof(struct fanotify_event_metadata), userspace is
expected to look for struct fanotify_event_info_header after struct
fanotify_event_metadata. struct fanotify_event_info_header looks like:
struct fanotify_event_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
Again if the end of this info (defined by 'len') is smaller than
'event_len', there is next header with next payload of data. So for example
error event will have:
struct fanotify_event_metadata
struct fanotify_event_info_error
struct fanotify_event_info_fid
Now either we could add fs specific blob into fanotify_event_info_error
(but then it would be good to add 'magic' to fanotify_event_info_error now
and define that if 'len' is larger, fs-specific blob follows after fixed
data) or we can add another info type FAN_EVENT_INFO_TYPE_ERROR_FS_DATA
(i.e., attach another structure into the event) which would contain the
'magic' and then blob of data. I don't have strong preference.
I have three questions at this point:
1) What's the maximum size of a fanotify event structure? None of these
structures exceed 36 bytes, which I hope will fit in whatever size
constraints?
Whole event must fit into 4G, each event info needs to fit in 64k. At least
these are the limits of the interface. Practically, it would be difficult
and inefficient to manipulate such huge events...
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
That is the expected behavior :). But I guess separate info type for
fs-specific blob might be more foolproof in this sense - when parsing
events, you are expected to just skip info_types you don't understand
(based on 'len' and 'type' in the common header) and generally different
events have different sets of infos attached to them so you mostly have to
implement this logic to be able to process events.
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
If we go via the 'separate info type' route, then the magic can go into
that structure and there's no great use for 'magic' in
fanotify_event_info_error.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-17 10:08:30
On Tue, Aug 17, 2021 at 12:05 PM Jan Kara [off-list ref] wrote:
On Mon 16-08-21 14:41:03, Darrick J. Wong wrote:
quoted
On Thu, Aug 12, 2021 at 05:40:07PM -0400, Gabriel Krisman Bertazi wrote:
quoted
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
My apologies for not having time to review this patchset since it was
redesigned to use fanotify. Someday it would be helpful to be able to
export more detailed error reports from XFS, but as I'm not ready to
move forward and write that today, I'll try to avoid derailling this at
the last minute.
I think we are not quite there and tweaking the passed structure is easy
enough so no worries. Eventually, passing some filesystem-specific blob
together with the event was the plan AFAIR. You're right now is a good
moment to think how exactly we want that passed.
quoted
Eventually, XFS might want to be able to report errors in file data,
file metadata, allocation group metadata, and whole-filesystem metadata.
Userspace can already gather reports from XFS about corruptions reported
by the online fsck code (see xfs_health.c).
Yes, although note that the current plan is that we currently have only one
error event queue, others are just added to error_count until the event is
fetched by userspace (on the grounds that the first error is usually the
most meaningful, the others are usually just cascading problems). But I'm
not sure if this scheme would be suitable for online fsck usecase since we
may discard even valid independent errors this way.
quoted
I /think/ we could subclass the file error structure that you've
provided like so:
struct fanotify_event_info_xfs_filesystem_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* quotas, realtime bitmap, etc. */
};
struct fanotify_event_info_xfs_perag_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* agf, agi, agfl, bno btree, ino btree, etc. */
__u32 agno; /* allocation group number */
};
struct fanotify_event_info_xfs_file_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* extent map, dir, attr, etc. */
__u64 offset; /* file data offset, if applicable */
__u64 length; /* file data length, if applicable */
};
(A real XFS implementation might have one structure with the type code
providing for a tagged union or something; I split it into three
separate structs here to avoid confusing things.)
The structure of fanotify event as passed to userspace generally is:
struct fanotify_event_metadata {
__u32 event_len;
__u8 vers;
__u8 reserved;
__u16 metadata_len;
__aligned_u64 mask;
__s32 fd;
__s32 pid;
};
If event_len is > sizeof(struct fanotify_event_metadata), userspace is
expected to look for struct fanotify_event_info_header after struct
fanotify_event_metadata. struct fanotify_event_info_header looks like:
struct fanotify_event_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
Again if the end of this info (defined by 'len') is smaller than
'event_len', there is next header with next payload of data. So for example
error event will have:
struct fanotify_event_metadata
struct fanotify_event_info_error
struct fanotify_event_info_fid
Now either we could add fs specific blob into fanotify_event_info_error
(but then it would be good to add 'magic' to fanotify_event_info_error now
and define that if 'len' is larger, fs-specific blob follows after fixed
data) or we can add another info type FAN_EVENT_INFO_TYPE_ERROR_FS_DATA
(i.e., attach another structure into the event) which would contain the
'magic' and then blob of data. I don't have strong preference.
quoted
I have three questions at this point:
1) What's the maximum size of a fanotify event structure? None of these
structures exceed 36 bytes, which I hope will fit in whatever size
constraints?
Whole event must fit into 4G, each event info needs to fit in 64k. At least
these are the limits of the interface. Practically, it would be difficult
and inefficient to manipulate such huge events...
Just keep in mind that the current scheme pre-allocates the single event slot
on fanotify_mark() time and (I think) we agreed to pre-allocate
sizeof(fsnotify_error_event) + MAX_HDNALE_SZ.
If filesystems would want to store some variable length fs specific info,
a future implementation will have to take that into account.
quoted
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
That is the expected behavior :). But I guess separate info type for
fs-specific blob might be more foolproof in this sense - when parsing
events, you are expected to just skip info_types you don't understand
(based on 'len' and 'type' in the common header) and generally different
events have different sets of infos attached to them so you mostly have to
implement this logic to be able to process events.
quoted
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
If we go via the 'separate info type' route, then the magic can go into
that structure and there's no great use for 'magic' in
fanotify_event_info_error.
My 0.02$:
With current patch set, filesystem reports error using:
fsnotify_sb_error(sb, inode, error)
The optional @inode argument is encoded to a filesystem opaque
blob using exportfs_encode_inode_fh(), recorded in the event
as a blob and reported to userspace as a blob.
If filesystem would like to report a different type of opaque blob
(e.g. xfs_perag_info), the interface should be extended to:
fsnotify_sb_error(sb, inode, error, info, info_len)
and the 'separate info type' route seems like the best and most natural
way to deal with the case of information that is only emitted from
a specific filesystem with a specific feature enabled (online fsck).
IOW, there is no need for fanotify_event_info_xfs_perag_error
in fanotify UAPI if you ask me.
Regarding 'magic' in fanotify_event_info_error, I also don't see the
need for that, because the event already has fsid which can be
used to identify the filesystem in question.
Keep in mind that the value of handle_type inside struct file_handle
inside struct fanotify_event_info_fid is not a universal classifier.
Specifically, the type 0x81 means "XFS_FILEID_INO64_GEN"
only in the context of XFS and it can mean something else in the
context of another type of filesystem.
If we add a new info record fanotify_event_info_fs_private
it could even be an alias to fanotify_event_info_fid with the only
difference that the handle[0] member is not expected to be
struct file_handle, but some other fs private struct.
Thanks,
Amir.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-08-18 00:10:40
On Tue, Aug 17, 2021 at 11:05:38AM +0200, Jan Kara wrote:
On Mon 16-08-21 14:41:03, Darrick J. Wong wrote:
quoted
On Thu, Aug 12, 2021 at 05:40:07PM -0400, Gabriel Krisman Bertazi wrote:
quoted
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
My apologies for not having time to review this patchset since it was
redesigned to use fanotify. Someday it would be helpful to be able to
export more detailed error reports from XFS, but as I'm not ready to
move forward and write that today, I'll try to avoid derailling this at
the last minute.
I think we are not quite there and tweaking the passed structure is easy
enough so no worries. Eventually, passing some filesystem-specific blob
together with the event was the plan AFAIR. You're right now is a good
moment to think how exactly we want that passed.
quoted
Eventually, XFS might want to be able to report errors in file data,
file metadata, allocation group metadata, and whole-filesystem metadata.
Userspace can already gather reports from XFS about corruptions reported
by the online fsck code (see xfs_health.c).
Yes, although note that the current plan is that we currently have only one
error event queue, others are just added to error_count until the event is
fetched by userspace (on the grounds that the first error is usually the
most meaningful, the others are usually just cascading problems). But I'm
not sure if this scheme would be suitable for online fsck usecase since we
may discard even valid independent errors this way.
<nod> The use-cases might split here -- we probably don't want online
fsck to be generating fs error events since the only tool that can do
anything about the broken metadata is the online fsck tool itself.
However, for random errors found by regular reader/writer threads, I
have a patchset in djwong-dev that adds recording of those errors;
that's the place where I think I'd want to add the ability to send
notification blobs to userspace.
Hmm. For handling accumulated errors, can we still access the
fanotify_event_info_* object once we've handed it to fanotify? If the
user hasn't picked up the event yet, it might be acceptable to set more
bits in the type mask and bump the error count. In other words, every
time userspace actually reads the event, it'll get the latest error
state. I /think/ that's where the design of this patchset is going,
right?
quoted
I /think/ we could subclass the file error structure that you've
provided like so:
struct fanotify_event_info_xfs_filesystem_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* quotas, realtime bitmap, etc. */
};
struct fanotify_event_info_xfs_perag_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* agf, agi, agfl, bno btree, ino btree, etc. */
__u32 agno; /* allocation group number */
};
struct fanotify_event_info_xfs_file_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* extent map, dir, attr, etc. */
__u64 offset; /* file data offset, if applicable */
__u64 length; /* file data length, if applicable */
};
(A real XFS implementation might have one structure with the type code
providing for a tagged union or something; I split it into three
separate structs here to avoid confusing things.)
The structure of fanotify event as passed to userspace generally is:
struct fanotify_event_metadata {
__u32 event_len;
__u8 vers;
__u8 reserved;
__u16 metadata_len;
__aligned_u64 mask;
__s32 fd;
__s32 pid;
};
If event_len is > sizeof(struct fanotify_event_metadata), userspace is
expected to look for struct fanotify_event_info_header after struct
fanotify_event_metadata. struct fanotify_event_info_header looks like:
struct fanotify_event_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
Again if the end of this info (defined by 'len') is smaller than
'event_len', there is next header with next payload of data. So for example
error event will have:
struct fanotify_event_metadata
struct fanotify_event_info_error
struct fanotify_event_info_fid
Now either we could add fs specific blob into fanotify_event_info_error
(but then it would be good to add 'magic' to fanotify_event_info_error now
and define that if 'len' is larger, fs-specific blob follows after fixed
data) or we can add another info type FAN_EVENT_INFO_TYPE_ERROR_FS_DATA
(i.e., attach another structure into the event) which would contain the
'magic' and then blob of data. I don't have strong preference.
I have a slight preference for the second. It doesn't make much sense
to have a magic value in fanotify_event_info_error to decode a totally
separate structure.
quoted
I have three questions at this point:
1) What's the maximum size of a fanotify event structure? None of these
structures exceed 36 bytes, which I hope will fit in whatever size
constraints?
Whole event must fit into 4G, each event info needs to fit in 64k. At least
these are the limits of the interface. Practically, it would be difficult
and inefficient to manipulate such huge events...
Ok. I doubt we'll ever get close to a 4k page for a single fs object.
quoted
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
That is the expected behavior :). But I guess separate info type for
fs-specific blob might be more foolproof in this sense - when parsing
events, you are expected to just skip info_types you don't understand
(based on 'len' and 'type' in the common header) and generally different
events have different sets of infos attached to them so you mostly have to
implement this logic to be able to process events.
Ok, good to hear this. :)
quoted
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
If we go via the 'separate info type' route, then the magic can go into
that structure and there's no great use for 'magic' in
fanotify_event_info_error.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-08-18 00:16:35
On Tue, Aug 17, 2021 at 01:08:06PM +0300, Amir Goldstein wrote:
On Tue, Aug 17, 2021 at 12:05 PM Jan Kara [off-list ref] wrote:
quoted
On Mon 16-08-21 14:41:03, Darrick J. Wong wrote:
quoted
On Thu, Aug 12, 2021 at 05:40:07PM -0400, Gabriel Krisman Bertazi wrote:
quoted
The Error info type is a record sent to users on FAN_FS_ERROR events
documenting the type of error. It also carries an error count,
documenting how many errors were observed since the last reporting.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v5:
- Move error code here
---
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fanotify/fanotify.h | 1 +
fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 45 insertions(+)
My apologies for not having time to review this patchset since it was
redesigned to use fanotify. Someday it would be helpful to be able to
export more detailed error reports from XFS, but as I'm not ready to
move forward and write that today, I'll try to avoid derailling this at
the last minute.
I think we are not quite there and tweaking the passed structure is easy
enough so no worries. Eventually, passing some filesystem-specific blob
together with the event was the plan AFAIR. You're right now is a good
moment to think how exactly we want that passed.
quoted
Eventually, XFS might want to be able to report errors in file data,
file metadata, allocation group metadata, and whole-filesystem metadata.
Userspace can already gather reports from XFS about corruptions reported
by the online fsck code (see xfs_health.c).
Yes, although note that the current plan is that we currently have only one
error event queue, others are just added to error_count until the event is
fetched by userspace (on the grounds that the first error is usually the
most meaningful, the others are usually just cascading problems). But I'm
not sure if this scheme would be suitable for online fsck usecase since we
may discard even valid independent errors this way.
quoted
I /think/ we could subclass the file error structure that you've
provided like so:
struct fanotify_event_info_xfs_filesystem_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* quotas, realtime bitmap, etc. */
};
struct fanotify_event_info_xfs_perag_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* agf, agi, agfl, bno btree, ino btree, etc. */
__u32 agno; /* allocation group number */
};
struct fanotify_event_info_xfs_file_error {
struct fanotify_event_info_error base;
__u32 magic; /* 0x58465342 to identify xfs */
__u32 type; /* extent map, dir, attr, etc. */
__u64 offset; /* file data offset, if applicable */
__u64 length; /* file data length, if applicable */
};
(A real XFS implementation might have one structure with the type code
providing for a tagged union or something; I split it into three
separate structs here to avoid confusing things.)
The structure of fanotify event as passed to userspace generally is:
struct fanotify_event_metadata {
__u32 event_len;
__u8 vers;
__u8 reserved;
__u16 metadata_len;
__aligned_u64 mask;
__s32 fd;
__s32 pid;
};
If event_len is > sizeof(struct fanotify_event_metadata), userspace is
expected to look for struct fanotify_event_info_header after struct
fanotify_event_metadata. struct fanotify_event_info_header looks like:
struct fanotify_event_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
Again if the end of this info (defined by 'len') is smaller than
'event_len', there is next header with next payload of data. So for example
error event will have:
struct fanotify_event_metadata
struct fanotify_event_info_error
struct fanotify_event_info_fid
Now either we could add fs specific blob into fanotify_event_info_error
(but then it would be good to add 'magic' to fanotify_event_info_error now
and define that if 'len' is larger, fs-specific blob follows after fixed
data) or we can add another info type FAN_EVENT_INFO_TYPE_ERROR_FS_DATA
(i.e., attach another structure into the event) which would contain the
'magic' and then blob of data. I don't have strong preference.
quoted
I have three questions at this point:
1) What's the maximum size of a fanotify event structure? None of these
structures exceed 36 bytes, which I hope will fit in whatever size
constraints?
Whole event must fit into 4G, each event info needs to fit in 64k. At least
these are the limits of the interface. Practically, it would be difficult
and inefficient to manipulate such huge events...
Just keep in mind that the current scheme pre-allocates the single event slot
on fanotify_mark() time and (I think) we agreed to pre-allocate
sizeof(fsnotify_error_event) + MAX_HDNALE_SZ.
If filesystems would want to store some variable length fs specific info,
a future implementation will have to take that into account.
<nod> I /think/ for the fs and AG metadata we could preallocate these,
so long as fsnotify doesn't free them out from under us. For inodes...
there are many more of those, so they'd have to be allocated
dynamically.
quoted
quoted
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
That is the expected behavior :). But I guess separate info type for
fs-specific blob might be more foolproof in this sense - when parsing
events, you are expected to just skip info_types you don't understand
(based on 'len' and 'type' in the common header) and generally different
events have different sets of infos attached to them so you mostly have to
implement this logic to be able to process events.
quoted
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
If we go via the 'separate info type' route, then the magic can go into
that structure and there's no great use for 'magic' in
fanotify_event_info_error.
My 0.02$:
With current patch set, filesystem reports error using:
fsnotify_sb_error(sb, inode, error)
The optional @inode argument is encoded to a filesystem opaque
blob using exportfs_encode_inode_fh(), recorded in the event
as a blob and reported to userspace as a blob.
If filesystem would like to report a different type of opaque blob
(e.g. xfs_perag_info), the interface should be extended to:
fsnotify_sb_error(sb, inode, error, info, info_len)
and the 'separate info type' route seems like the best and most natural
way to deal with the case of information that is only emitted from
a specific filesystem with a specific feature enabled (online fsck).
<nod> This seems reasonable to me.
IOW, there is no need for fanotify_event_info_xfs_perag_error
in fanotify UAPI if you ask me.
Regarding 'magic' in fanotify_event_info_error, I also don't see the
need for that, because the event already has fsid which can be
used to identify the filesystem in question.
Keep in mind that the value of handle_type inside struct file_handle
inside struct fanotify_event_info_fid is not a universal classifier.
Specifically, the type 0x81 means "XFS_FILEID_INO64_GEN"
only in the context of XFS and it can mean something else in the
context of another type of filesystem.
Can you pass the handle into the kernel to open a fd to file mentioned
in the report? I don't think userspace is supposed to know what's
inside a file handle, and it would be helpful if it didn't matter here
either. :)
If we add a new info record fanotify_event_info_fs_private
it could even be an alias to fanotify_event_info_fid with the only
difference that the handle[0] member is not expected to be
struct file_handle, but some other fs private struct.
I ... think I prefer it being a separate info blob.
--D
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-18 03:24:41
[...]
quoted
Just keep in mind that the current scheme pre-allocates the single event slot
on fanotify_mark() time and (I think) we agreed to pre-allocate
sizeof(fsnotify_error_event) + MAX_HDNALE_SZ.
If filesystems would want to store some variable length fs specific info,
a future implementation will have to take that into account.
<nod> I /think/ for the fs and AG metadata we could preallocate these,
so long as fsnotify doesn't free them out from under us.
fs won't get notified when the event is freed, so fsnotify must
take ownership on the data structure.
I was thinking more along the lines of limiting maximum size for fs
specific info and pre-allocating that size for the event.
For inodes...
there are many more of those, so they'd have to be allocated
dynamically.
The current scheme is that the size of the queue for error events
is one and the single slot is pre-allocated.
The reason for pre-allocate is that the assumption is that fsnotify_error()
could be called from contexts where memory allocation would be
inconvenient.
Therefore, we can store the encoded file handle of the first erroneous
inode, but we do not store any more events until user read this
one event.
Hmm. For handling accumulated errors, can we still access the
fanotify_event_info_* object once we've handed it to fanotify? If the
user hasn't picked up the event yet, it might be acceptable to set more
bits in the type mask and bump the error count. In other words, every
time userspace actually reads the event, it'll get the latest error
state. I /think/ that's where the design of this patchset is going,
right?
Sort of.
fsnotify does have a concept of "merging" new event with an event
already in queue.
With most fsnotify events, merge only happens if the info related
to the new event (e.g. sb,inode) is the same as that off the queued
event and the "merge" is only in the event mask
(e.g. FS_OPEN|FS_CLOSE).
However, the current scheme for "merge" of an FS_ERROR event is only
bumping err_count, even if the new reported error or inode do not
match the error/inode in the queued event.
If we define error event subtypes (e.g. FS_ERROR_WRITEBACK,
FS_ERROR_METADATA), then the error event could contain
a field for subtype mask and user could read the subtype mask
along with the accumulated error count, but this cannot be
done by providing the filesystem access to modify an internal
fsnotify event, so those have to be generic UAPI defined subtypes.
If you think that would be useful, then we may want to consider
reserving the subtype mask field in fanotify_event_info_error in
advance.
quoted
quoted
quoted
2) If a program written for today's notification events sees a
fanotify_event_info_header from future-XFS with a header length that is
larger than FANOTIFY_INFO_ERROR_LEN, will it be able to react
appropriately? Which is to say, ignore it on the grounds that the
length is unexpectedly large?
That is the expected behavior :). But I guess separate info type for
fs-specific blob might be more foolproof in this sense - when parsing
events, you are expected to just skip info_types you don't understand
(based on 'len' and 'type' in the common header) and generally different
events have different sets of infos attached to them so you mostly have to
implement this logic to be able to process events.
quoted
It /looks/ like this is the case; really I'm just fishing around here
to make sure nothing in the design of /this/ patchset would make it Very
Difficult(tm) to add more information later.
3) Once we let filesystem implementations create their own extended
error notifications, should we have a "u32 magic" to aid in decoding?
Or even add it to fanotify_event_info_error now?
If we go via the 'separate info type' route, then the magic can go into
that structure and there's no great use for 'magic' in
fanotify_event_info_error.
My 0.02$:
With current patch set, filesystem reports error using:
fsnotify_sb_error(sb, inode, error)
The optional @inode argument is encoded to a filesystem opaque
blob using exportfs_encode_inode_fh(), recorded in the event
as a blob and reported to userspace as a blob.
If filesystem would like to report a different type of opaque blob
(e.g. xfs_perag_info), the interface should be extended to:
fsnotify_sb_error(sb, inode, error, info, info_len)
and the 'separate info type' route seems like the best and most natural
way to deal with the case of information that is only emitted from
a specific filesystem with a specific feature enabled (online fsck).
<nod> This seems reasonable to me.
quoted
IOW, there is no need for fanotify_event_info_xfs_perag_error
in fanotify UAPI if you ask me.
Regarding 'magic' in fanotify_event_info_error, I also don't see the
need for that, because the event already has fsid which can be
used to identify the filesystem in question.
Keep in mind that the value of handle_type inside struct file_handle
inside struct fanotify_event_info_fid is not a universal classifier.
Specifically, the type 0x81 means "XFS_FILEID_INO64_GEN"
only in the context of XFS and it can mean something else in the
context of another type of filesystem.
Can you pass the handle into the kernel to open a fd to file mentioned
in the report? I don't think userspace is supposed to know what's
inside a file handle, and it would be helpful if it didn't matter here
either. :)
User gets a file handle and can do whatever users can do with file
handles... that is, open_by_handle_at() (if filesystem and inode are
still alive and healthy) and for less privileged users, compare with
result of name_to_handle_at() of another object.
Obviously, filesystem specialized tools could parse the file handle
to extract more information.
quoted
If we add a new info record fanotify_event_info_fs_private
it could even be an alias to fanotify_event_info_fid with the only
difference that the handle[0] member is not expected to be
struct file_handle, but some other fs private struct.
I ... think I prefer it being a separate info blob.
Yes. That is what I meant.
Separate info record INFO_TYPE_ERROR_FS_DATA, whose info record
format is quite the same as that of INFO_TYPE_FID, but the blob is a
different type of blob.
Thanks,
Amir.
From: Jan Kara <jack@suse.cz> Date: 2021-08-18 09:58:27
On Wed 18-08-21 06:24:26, Amir Goldstein wrote:
[...]
quoted
quoted
Just keep in mind that the current scheme pre-allocates the single event slot
on fanotify_mark() time and (I think) we agreed to pre-allocate
sizeof(fsnotify_error_event) + MAX_HDNALE_SZ.
If filesystems would want to store some variable length fs specific info,
a future implementation will have to take that into account.
<nod> I /think/ for the fs and AG metadata we could preallocate these,
so long as fsnotify doesn't free them out from under us.
fs won't get notified when the event is freed, so fsnotify must
take ownership on the data structure.
I was thinking more along the lines of limiting maximum size for fs
specific info and pre-allocating that size for the event.
Agreed. If there's a sensible upperbound than preallocating this inside
fsnotify is likely the least problematic solution.
quoted
For inodes...
there are many more of those, so they'd have to be allocated
dynamically.
The current scheme is that the size of the queue for error events
is one and the single slot is pre-allocated.
The reason for pre-allocate is that the assumption is that fsnotify_error()
could be called from contexts where memory allocation would be
inconvenient.
Therefore, we can store the encoded file handle of the first erroneous
inode, but we do not store any more events until user read this
one event.
Right. OTOH I can imagine allowing GFP_NOFS allocations in the error
context. At least for ext4 it would be workable (after all ext4 manages to
lock & modify superblock in its error handlers, GFP_NOFS allocation isn't
harder). But then if events are dynamically allocated there's still the
inconvenient question what are you going to do if you need to report fs
error and you hit ENOMEM. Just not sending the notification may have nasty
consequences and in the world of containerization and virtualization
tightly packed machines where ENOMEM happens aren't that unlikely. It is
just difficult to make assumptions about filesystems overall so we decided
to be better safe and preallocate the event.
Or, we could leave the allocation troubles for the filesystem and
fsnotify_sb_error() would be passed already allocated event (this way
attaching of fs-specific blobs to the event is handled as well) which it
would just queue. Plus we'd need to provide some helper to fill in generic
part of the event...
The disadvantage is that if there are filesystems / callsites needing
preallocated events, it would be painful for them. OTOH current two users -
ext4 & xfs - can handle allocation in the error path AFAIU.
Thinking about this some more, maybe we could have event preallocated (like
a "rescue event"). Normally we would dynamically allocate (or get passed
from fs) the event and only if the allocation fails, we would queue the
rescue event to indicate to listeners that something bad happened, there
was error but we could not fully report it.
But then, even if we'd go for dynamic event allocation by default, we need
to efficiently merge events since some fs failures (e.g. resulting in
journal abort in ext4) lead to basically all operations with the filesystem
to fail and that could easily swamp the notification system with useless
events. Current system with preallocated event nicely handles this
situation, it is questionable how to extend it for online fsck usecase
where we need to queue more than one event (but even there probably needs
to be some sensible upper-bound). I'll think about it...
quoted
Hmm. For handling accumulated errors, can we still access the
fanotify_event_info_* object once we've handed it to fanotify? If the
user hasn't picked up the event yet, it might be acceptable to set more
bits in the type mask and bump the error count. In other words, every
time userspace actually reads the event, it'll get the latest error
state. I /think/ that's where the design of this patchset is going,
right?
Sort of.
fsnotify does have a concept of "merging" new event with an event
already in queue.
With most fsnotify events, merge only happens if the info related
to the new event (e.g. sb,inode) is the same as that off the queued
event and the "merge" is only in the event mask
(e.g. FS_OPEN|FS_CLOSE).
However, the current scheme for "merge" of an FS_ERROR event is only
bumping err_count, even if the new reported error or inode do not
match the error/inode in the queued event.
If we define error event subtypes (e.g. FS_ERROR_WRITEBACK,
FS_ERROR_METADATA), then the error event could contain
a field for subtype mask and user could read the subtype mask
along with the accumulated error count, but this cannot be
done by providing the filesystem access to modify an internal
fsnotify event, so those have to be generic UAPI defined subtypes.
If you think that would be useful, then we may want to consider
reserving the subtype mask field in fanotify_event_info_error in
advance.
It depends on what exactly Darrick has in mind but I suspect we'd need a
fs-specific merge helper that would look at fs-specific blobs in the event
and decide whether events can be merged or not, possibly also handling the
merge by updating the blob. From the POV of fsnotify that would probably
mean merge callback in the event itself. But I guess this needs more
details from Darrick and maybe we don't need to decide this at this moment
since nobody is close to the point of having code needing to pass fs-blobs
with events.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-18 13:03:00
On Thu 12-08-21 17:40:09, Gabriel Krisman Bertazi wrote:
Introduce an example of a FAN_FS_ERROR fanotify user to track filesystem
errors.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Shouldn't we get these from uapi headers? But I guess the problem is that
you want this sample to work before glibc picks up the new headers? Is this
meant as a sample code for userspace to copy from or more as a testcase?
The ordering of additional infos is undefined. Your code must not rely on
the fact that FAN_EVENT_INFO_TYPE_ERROR comes first and
FAN_EVENT_INFO_TYPE_FID second. Also you should ignore (maybe just print
type and len in this sample code) when you see unexpected info types as
later additions to the API may add additional info records
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-08-19 03:58:52
On Wed, Aug 18, 2021 at 11:58:18AM +0200, Jan Kara wrote:
On Wed 18-08-21 06:24:26, Amir Goldstein wrote:
quoted
[...]
quoted
quoted
Just keep in mind that the current scheme pre-allocates the single event slot
on fanotify_mark() time and (I think) we agreed to pre-allocate
sizeof(fsnotify_error_event) + MAX_HDNALE_SZ.
If filesystems would want to store some variable length fs specific info,
a future implementation will have to take that into account.
<nod> I /think/ for the fs and AG metadata we could preallocate these,
so long as fsnotify doesn't free them out from under us.
fs won't get notified when the event is freed, so fsnotify must
take ownership on the data structure.
I was thinking more along the lines of limiting maximum size for fs
specific info and pre-allocating that size for the event.
Agreed. If there's a sensible upperbound than preallocating this inside
fsnotify is likely the least problematic solution.
quoted
quoted
For inodes...
there are many more of those, so they'd have to be allocated
dynamically.
The current scheme is that the size of the queue for error events
is one and the single slot is pre-allocated.
The reason for pre-allocate is that the assumption is that fsnotify_error()
could be called from contexts where memory allocation would be
inconvenient.
Therefore, we can store the encoded file handle of the first erroneous
inode, but we do not store any more events until user read this
one event.
Right. OTOH I can imagine allowing GFP_NOFS allocations in the error
context. At least for ext4 it would be workable (after all ext4 manages to
lock & modify superblock in its error handlers, GFP_NOFS allocation isn't
harder). But then if events are dynamically allocated there's still the
inconvenient question what are you going to do if you need to report fs
error and you hit ENOMEM. Just not sending the notification may have nasty
consequences and in the world of containerization and virtualization
tightly packed machines where ENOMEM happens aren't that unlikely. It is
just difficult to make assumptions about filesystems overall so we decided
to be better safe and preallocate the event.
Or, we could leave the allocation troubles for the filesystem and
fsnotify_sb_error() would be passed already allocated event (this way
attaching of fs-specific blobs to the event is handled as well) which it
would just queue. Plus we'd need to provide some helper to fill in generic
part of the event...
The disadvantage is that if there are filesystems / callsites needing
preallocated events, it would be painful for them. OTOH current two users -
ext4 & xfs - can handle allocation in the error path AFAIU.
Thinking about this some more, maybe we could have event preallocated (like
a "rescue event"). Normally we would dynamically allocate (or get passed
from fs) the event and only if the allocation fails, we would queue the
rescue event to indicate to listeners that something bad happened, there
was error but we could not fully report it.
Yes.
But then, even if we'd go for dynamic event allocation by default, we need
to efficiently merge events since some fs failures (e.g. resulting in
journal abort in ext4) lead to basically all operations with the filesystem
to fail and that could easily swamp the notification system with useless
events.
Hm. Going out on a limb, I would guess that the majority of fs error
flood events happen if the storage fails catastrophically. Assuming
that a catastrophic failure will quickly take the filesystem offline, I
would say that for XFS we should probably send one last "and then we
died" event and stop reporting after that.
Current system with preallocated event nicely handles this
situation, it is questionable how to extend it for online fsck usecase
where we need to queue more than one event (but even there probably needs
to be some sensible upper-bound). I'll think about it...
At least for XFS, I was figuring that xfs_scrub errors wouldn't be
reported via fsnotify since the repair tool is already running anyway.
quoted
quoted
Hmm. For handling accumulated errors, can we still access the
fanotify_event_info_* object once we've handed it to fanotify? If the
user hasn't picked up the event yet, it might be acceptable to set more
bits in the type mask and bump the error count. In other words, every
time userspace actually reads the event, it'll get the latest error
state. I /think/ that's where the design of this patchset is going,
right?
Sort of.
fsnotify does have a concept of "merging" new event with an event
already in queue.
With most fsnotify events, merge only happens if the info related
to the new event (e.g. sb,inode) is the same as that off the queued
event and the "merge" is only in the event mask
(e.g. FS_OPEN|FS_CLOSE).
However, the current scheme for "merge" of an FS_ERROR event is only
bumping err_count, even if the new reported error or inode do not
match the error/inode in the queued event.
If we define error event subtypes (e.g. FS_ERROR_WRITEBACK,
FS_ERROR_METADATA), then the error event could contain
a field for subtype mask and user could read the subtype mask
along with the accumulated error count, but this cannot be
done by providing the filesystem access to modify an internal
fsnotify event, so those have to be generic UAPI defined subtypes.
If you think that would be useful, then we may want to consider
reserving the subtype mask field in fanotify_event_info_error in
advance.
It depends on what exactly Darrick has in mind but I suspect we'd need a
fs-specific merge helper that would look at fs-specific blobs in the event
and decide whether events can be merged or not, possibly also handling the
merge by updating the blob.
Yes. If the filesystem itself were allowed to manage the lifespan of
the fsnotify error event object then this would be trivial -- we'll own
the object, keep it updated as needed, and fsnotify can copy the
contents to userspace whenever convenient.
(This might be a naïve view of fsnotify...)
From the POV of fsnotify that would probably
mean merge callback in the event itself. But I guess this needs more
details from Darrick and maybe we don't need to decide this at this moment
since nobody is close to the point of having code needing to pass fs-blobs
with events.
<nod> We ... probably don't need to decide this now.
--D