From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:06:39
Hi,
This is the 5th version of the FAN_FS_ERROR patches. This applies
the feedback from last version (thanks Amir, Jan). Biggest changes are
the split up of the FAN_FS_ERROR patch into something more reviewable,
and the removal of the event_info structure due to the perf regression
shown by unixbench.
This was tested with LTP for regressions, and also using the sample 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 (23):
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 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: Support passing argument to insert callback on add_event
fsnotify: Allow events reported with an empty inode
fsnotify: Support FS_ERROR event type
fanotify: Expose helper to estimate file handle encoding length
fanotify: Allow file handle encoding for unhashed events
fanotify: Encode invalid file handler 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/kernfs/file.c | 6 +-
fs/notify/fanotify/fanotify.c | 186 +++++++++---
fs/notify/fanotify/fanotify.h | 80 +++++-
fs/notify/fanotify/fanotify_user.c | 266 +++++++++++++++---
fs/notify/fsnotify.c | 14 +-
fs/notify/inotify/inotify_fsnotify.c | 2 +-
fs/notify/inotify/inotify_user.c | 6 +-
fs/notify/notification.c | 16 +-
include/linux/fanotify.h | 9 +-
include/linux/fsnotify.h | 20 +-
include/linux/fsnotify_backend.h | 76 ++++-
include/uapi/linux/fanotify.h | 8 +
samples/Kconfig | 9 +
samples/Makefile | 1 +
samples/fanotify/Makefile | 5 +
samples/fanotify/fs-monitor.c | 138 +++++++++
19 files changed, 803 insertions(+), 118 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-04 16:06:40
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-04 16:06:41
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-04 16:06:45
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-04 16:06:52
Split out the final bits of struct fsnotify_mark->flags for use by a
backend.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify_backend.h | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:06:54
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 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 | 23 ++++++++++++++++++
fs/notify/fanotify/fanotify_user.c | 38 ++++++++++++++++++++++++++++--
3 files changed, 67 insertions(+), 4 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:06:55
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-04 16:06:58
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-04 16:07:01
fsnotify_add_event is growing in number of parameters, which is 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>
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);
@@ -495,16 +495,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-04 16:07:05
FAN_FS_ERROR requires some initialization to happen from inside the
insert hook. This allows a user of fanotify_add_event to pass an
argument to be sent to the insert callback.
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 | 5 +++--
fs/notify/notification.c | 6 ++++--
include/linux/fsnotify_backend.h | 6 ++++--
3 files changed, 11 insertions(+), 6 deletions(-)
@@ -500,14 +500,16 @@ extern int fsnotify_insert_event(struct fsnotify_group *group,int(*merge)(structfsnotify_group*,structfsnotify_event*),void(*insert)(structfsnotify_group*,-structfsnotify_event*));+structfsnotify_event*,+constvoid*),+constvoid*insert_data);staticinlineintfsnotify_add_event(structfsnotify_group*group,structfsnotify_event*event,int(*merge)(structfsnotify_group*,structfsnotify_event*)){-returnfsnotify_insert_event(group,event,merge,NULL);+returnfsnotify_insert_event(group,event,merge,NULL,NULL);}/* Queue overflow event to a notification group */
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:07:15
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 directly against the super block instead of an
inode.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/kernfs/file.c | 6 +++---
fs/notify/fsnotify.c | 14 +++++++++-----
include/linux/fsnotify.h | 8 +++++---
include/linux/fsnotify_backend.h | 10 ++++++----
4 files changed, 23 insertions(+), 15 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:07:19
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 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)
---
include/linux/fsnotify.h | 12 ++++++++++++
include/linux/fsnotify_backend.h | 17 ++++++++++++++++-
2 files changed, 28 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-04 16:07:29
FAN_FS_ERROR will report a file handle, but it is a unhashed event.n
Allow passing a NULL hash to fanotify_encode_fh and avoid calculating
the hash if not needed.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:07:30
Instead of failing, encode an invalid file handler in fanotify_encode_fh
if no inode is provided. This bogus file handler will be reported by
FAN_FS_ERROR for non-inode errors.
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>
---
fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++---------------
fs/notify/fanotify/fanotify.h | 6 ++++--
2 files changed, 26 insertions(+), 19 deletions(-)
@@ -363,8 +357,9 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,if(fh_len<4||WARN_ON_ONCE(fh_len%4))gotoout_err;-/* No external buffer in a variable size allocated fh */-if(gfp&&fh_len>FANOTIFY_INLINE_FH_LEN){+fh->flags=0;+/* No external buffer in a variable size allocated fh or null fh */+if(inode&&gfp&&fh_len>FANOTIFY_INLINE_FH_LEN){/* Treat failure to allocate fh as failure to encode fh */err=-ENOMEM;ext_buf=kmalloc(fh_len,gfp);
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:07:38
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>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
changes since v5:
- Fix condition to include FANOTIFY_EVENT_FLAGS. (me)
---
fs/notify/fanotify/fanotify_user.c | 14 +++++++-------
include/linux/fanotify.h | 3 +++
2 files changed, 10 insertions(+), 7 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-04 16:07:45
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.
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-04 16:07:47
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>
---
fs/notify/fanotify/fanotify.c | 22 +++++++++++++++++
fs/notify/fanotify/fanotify.h | 13 ++++++++++
fs/notify/fanotify/fanotify_user.c | 39 +++++++++++++++++++++++++++++-
3 files changed, 73 insertions(+), 1 deletion(-)
@@ -219,6 +221,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-04 16:07:49
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 replace the event in the slot with a new
structure, 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 new slot.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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 | 50 +++++++++++++++++++++++-
fs/notify/fanotify/fanotify.h | 9 +++++
fs/notify/fanotify/fanotify_user.c | 63 +++++++++++++++++++++++++++++-
include/linux/fanotify.h | 6 ++-
4 files changed, 124 insertions(+), 4 deletions(-)
@@ -681,6 +681,42 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)returnfsid;}+staticintfanotify_merge_error_event(structfsnotify_group*group,+structfsnotify_event*event)+{+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee=FANOTIFY_EE(fae);++/*+*Whenerr_count>0,thereportingslotisfull.Justaccount+*theadditionalerrorandaborttheinsertion.+*/+if(fee->err_count){+fee->err_count++;+return1;+}++return0;+}++staticvoidfanotify_insert_error_event(structfsnotify_group*group,+structfsnotify_event*event,+constvoid*data)+{+conststructfs_error_report*report=(structfs_error_report*)data;+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee;++/* This might be an unexpected type of event (i.e. overflow). */+if(!fanotify_is_error_event(fae->mask))+return;++fee=FANOTIFY_EE(fae);+fee->fae.type=FANOTIFY_EVENT_TYPE_FS_ERROR;+fee->error=report->error;+fee->err_count=1;+}+/**Addaneventtohashtableforfastermerge.*/
@@ -223,6 +223,9 @@ 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. */};
@@ -1464,6 +1518,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-04 16:07:54
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we estimate a file handler 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 handler report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 27 +++++++++++++++++++++++++++
fs/notify/fanotify/fanotify.h | 12 ++++++++++++
fs/notify/fanotify/fanotify_user.c | 19 ++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
@@ -705,7 +705,9 @@ static void fanotify_insert_error_event(struct fsnotify_group *group,{conststructfs_error_report*report=(structfs_error_report*)data;structfanotify_event*fae=FANOTIFY_E(event);+structinode*inode=report->inode;structfanotify_error_event*fee;+intfh_len;/* This might be an unexpected type of event (i.e. overflow). */if(!fanotify_is_error_event(fae->mask))
@@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group,fee->fae.type=FANOTIFY_EVENT_TYPE_FS_ERROR;fee->error=report->error;fee->err_count=1;+fee->fsid=fee->sb_mark->fsn_mark.connector->fsid;++/*+*Errorreportingneedstohappeninatomiccontext.Ifthis+*inode'sfilehandlerlengthismorethanweinitially+*predicted,thereisnothingbetterwecandothanreportthe+*errorwithabadfilehandler.+*/+fh_len=fanotify_encode_fh_len(inode);+if(fh_len>fee->sb_mark->pred_fh_len){+pr_warn_ratelimited(+"FH overflows error event. Drop inode information.\n");+/*+*Updatethehandlersizepredictionforthenexterror+*eventallocation.Thisreducesthechanceofanother+*overflow.+*/+fee->sb_mark->pred_fh_len=fh_len;++/* For the current error, ignore the inode information. */+inode=NULL;+fh_len=fanotify_encode_fh_len(NULL);+}++fanotify_encode_fh(&fee->object_fh,inode,fh_len,NULL,0);}/*
@@ -227,6 +228,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_fhisfollowedbyavariablesizedbuffer,soitmust+*bethelastelementofthisstructure.+*/+structfanotify_fhobject_fh;};staticinlinestructfanotify_error_event*
@@ -172,8 +172,25 @@ static struct fanotify_error_event *fanotify_alloc_error_event({structfanotify_error_event*fee;+structsuper_block*sb;-fee=kzalloc(sizeof(*fee),GFP_KERNEL_ACCOUNT);+if(!sb_mark->pred_fh_len){+/*+*Thehandlersizeisinitiallypredictedtobethe+*samesizeastherootinodefilehandler.Itcanbe+*increasedlaterifalargerfilehandlerisfound.+*/+sb=container_of(sb_mark->fsn_mark.connector->obj,+structsuper_block,s_fsnotify_marks);+sb_mark->pred_fh_len=+fanotify_encode_fh_len(sb->s_root->d_inode);+}++/* Guarantee there is always space to report an invalid FID. */+if(sb_mark->pred_fh_len<FANOTIFY_NULL_FH_LEN)+sb_mark->pred_fh_len=FANOTIFY_NULL_FH_LEN;++fee=kzalloc(sizeof(*fee)+sb_mark->pred_fh_len,GFP_KERNEL_ACCOUNT);if(!fee)returnNULL;
From: Gabriel Krisman Bertazi <hidden> Date: 2021-08-04 16:07:57
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>
---
fs/notify/fanotify/fanotify_user.c | 35 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
2 files changed, 42 insertions(+)
@@ -395,6 +400,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)
@@ -553,6 +580,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-04 16:10:24
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-04 16:10:43
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 handler. A+filesystem specific handler can use that information to attempt a+recovery procedure. Errors that are not related to an inode are+reported with an empty file handler, with type FILEID_INVALID.
From: kernel test robot <hidden> Date: 2021-08-05 08:45:41
Hi Gabriel,
I love your patch! Yet something to improve:
[auto build test ERROR on ext3/fsnotify]
[also build test ERROR on ext4/dev ext3/for_next linus/master v5.14-rc4 next-20210804]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210805-001201
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: powerpc-ppc64_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e5294af07c565333b5efb87d6b431d7d601a1043
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210805-001201
git checkout e5294af07c565333b5efb87d6b431d7d601a1043
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All errors (new ones prefixed by >>):
In file included from include/linux/fsnotify.h:15,
from arch/powerpc/platforms/cell/spufs/inode.c:15:
include/linux/fsnotify_backend.h:367:27: error: 'FSNOTIFY_MARK_FLAG_ATTACHED' defined but not used [-Werror=unused-const-variable=]
367 | static const unsigned int FSNOTIFY_MARK_FLAG_##flag = \
| ^~~~~~~~~~~~~~~~~~~
include/linux/fsnotify_backend.h:379:1: note: in expansion of macro 'FSNOTIFY_MARK_FLAG'
379 | FSNOTIFY_MARK_FLAG(ATTACHED);
| ^~~~~~~~~~~~~~~~~~
include/linux/fsnotify_backend.h:367:27: error: 'FSNOTIFY_MARK_FLAG_ALIVE' defined but not used [-Werror=unused-const-variable=]
367 | static const unsigned int FSNOTIFY_MARK_FLAG_##flag = \
| ^~~~~~~~~~~~~~~~~~~
include/linux/fsnotify_backend.h:378:1: note: in expansion of macro 'FSNOTIFY_MARK_FLAG'
378 | FSNOTIFY_MARK_FLAG(ALIVE);
| ^~~~~~~~~~~~~~~~~~
quoted
include/linux/fsnotify_backend.h:367:27: error: 'FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY' defined but not used [-Werror=unused-const-variable=]
367 | static const unsigned int FSNOTIFY_MARK_FLAG_##flag = \
| ^~~~~~~~~~~~~~~~~~~
include/linux/fsnotify_backend.h:377:1: note: in expansion of macro 'FSNOTIFY_MARK_FLAG'
377 | FSNOTIFY_MARK_FLAG(IGNORED_SURV_MODIFY);
| ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY +367 include/linux/fsnotify_backend.h
365
366 #define FSNOTIFY_MARK_FLAG(flag) \
> 367 static const unsigned int FSNOTIFY_MARK_FLAG_##flag = \
368 (1 << FSN_MARK_FL_BIT_##flag)
369
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 09:14:11
On Wed 04-08-21 12:05:53, Gabriel Krisman Bertazi 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>
---
include/linux/fsnotify_backend.h | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
Static variable declaration in a header file makes me a bit uneasy. I know
it is const so a compiler should optimize this to a constant but still
there will likely be some side-effects (see the 0-day warning).
Honestly, given these are just three flags I'd just don't overengineer this
and have:
#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY \
(1 << FSN_MARK_FL_BIT_IGNORED_SURV_MODIFY)
...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 09:25:00
On Wed 04-08-21 12:05: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 mostly good, just one nit below:
quoted hunk
@@ -915,6 +916,38 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, return mask & ~oldmask; }+static struct fsnotify_mark *fanotify_alloc_mark(struct fsnotify_group *group,+ unsigned int type)+{+ struct fanotify_sb_mark *sb_mark;+ struct fsnotify_mark *mark;++ switch (type) {+ case FSNOTIFY_OBJ_TYPE_SB:+ sb_mark = kmem_cache_zalloc(fanotify_sb_mark_cache, GFP_KERNEL);+ if (!sb_mark)+ return NULL;+ mark = &sb_mark->fsn_mark;+ break;++ case FSNOTIFY_OBJ_TYPE_INODE:+ case FSNOTIFY_OBJ_TYPE_PARENT:+ case FSNOTIFY_OBJ_TYPE_VFSMOUNT:+ mark = kmem_cache_alloc(fanotify_mark_cache, GFP_KERNEL);+ break;
It is odd that sb marks are allocated with zalloc while other marks with
alloc. Why is that? It is errorprone to have this different among mark
types as somebody may mistakenly assume a mark is zeroed when it actually is
not. So please either use kmem_cache_alloc() for sb mark as well and zero
out by hand what you need, or do a cleanup patch that uses zalloc across
all of dnotify, inotify, fanotify (I can see kernel/audit_* users already
use zalloc) and drop zeroing from fsnotify_init_mark(). Thanks!
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 09:28:06
On Wed 04-08-21 12:05:57, Gabriel Krisman Bertazi wrote:
fsnotify_add_event is growing in number of parameters, which is most
^^ in most
cases...
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>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Otherwise looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
@@ -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);
@@ -495,16 +495,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: Jan Kara <jack@suse.cz> Date: 2021-08-05 09:56:21
On Wed 04-08-21 12:06:03, Gabriel Krisman Bertazi wrote:
Instead of failing, encode an invalid file handler in fanotify_encode_fh
if no inode is provided. This bogus file handler will be reported by
FAN_FS_ERROR for non-inode errors.
Also adjust the single caller that might rely on failure after passing
an empty inode.
It is not 'file handler' but rather 'file handle' - several times in the
changelog and in subject :).
I'd keep the fh->flags initialization here. Otherwise it will not be
initialized on some error returns.
quoted hunk
@@ -363,8 +357,9 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4)) goto out_err;- /* No external buffer in a variable size allocated fh */- if (gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {+ fh->flags = 0;+ /* No external buffer in a variable size allocated fh or null fh */+ if (inode && gfp && fh_len > FANOTIFY_INLINE_FH_LEN) { /* Treat failure to allocate fh as failure to encode fh */ err = -ENOMEM; ext_buf = kmalloc(fh_len, gfp);
@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF; }- dwords = fh_len >> 2;- type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);- err = -EINVAL;- if (!type || type == FILEID_INVALID || fh_len != dwords << 2)- goto out_err;-- fh->type = type;- fh->len = fh_len;+ if (inode) {+ dwords = fh_len >> 2;+ type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);+ err = -EINVAL;+ if (!type || type == FILEID_INVALID || fh_len != dwords << 2)+ goto out_err;+ fh->type = type;+ fh->len = fh_len;+ } else {+ /*+ * Invalid FHs are used on FAN_FS_ERROR for errors not+ * linked to any inode. Caller needs to guarantee the fh+ * has at least FANOTIFY_NULL_FH_LEN bytes of space.+ */+ fh->type = FILEID_INVALID;+ fh->len = FANOTIFY_NULL_FH_LEN;+ memset(buf, 0, FANOTIFY_NULL_FH_LEN);+ }
Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 10:24:56
On Wed 04-08-21 12:05:59, Gabriel Krisman Bertazi wrote:
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 directly against the super block instead of an
inode.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
There's a comment before fsnotify() declaration that states that 'either
@dir or @inode must be non-NULL' - in this patch it would be good time to
update that comment.
quoted hunk
@@ -459,12 +460,13 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) * 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 qstr *file_name, struct inode *inode, u32 cookie)+int fsnotify(__u32 mask, const void *data, int data_type,+ struct super_block *sb, struct inode *dir,+ const struct qstr *file_name, struct inode *inode,+ u32 cookie) {
Two notes as ideas for consideration:
1) We could derive 'sb' from 'data'. I.e., have a helper like
fsnotify_data_sb(data, data_type). For FSNOTIFY_EVENT_PATH and
FSNOTIFY_EVENT_INODE this is easy to provide, for FSNOTIFY_EVENT_ERROR we
would have to add sb pointer to the structure but I guess that's easy. That
way we'd avoid the mostly NULL 'sb' argument. What do you guys think?
2) AFAICS 'inode' can be always derived from 'data' as well. So maybe we
can drop it Amir?
That being said I can live with the code as is in this patch as well
(although with a bit of "ugh, irk ;)" so I want to discuss these ideas).
Honza
quoted hunk
const struct path *path = fsnotify_data_path(data, data_type); struct fsnotify_iter_info iter_info = {};- struct super_block *sb; struct mount *mnt = NULL; struct inode *parent = NULL; int ret = 0;
@@ -483,7 +485,9 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, */ parent = dir; }- sb = inode->i_sb;++ if (!sb)+ sb = inode->i_sb; /* * Optimization: srcu_read_lock() has a memory barrier which can
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 10:29:34
On Wed 04-08-21 12:06:04, Gabriel Krisman Bertazi 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>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. One style nit below, otherwise feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
quoted hunk
/*- * Events with data type inode do not carry enough information to report- * event->fd, so we do not allow setting a mask for inode events unless- * group supports reporting fid.- * inode events are not supported on a mount mark, because they do not- * carry enough information (i.e. path) to be filtered by mount point.- */+ * Events that do not carry enough information to report+ * event->fd require a group that supports reporting fid. Those+ * events are not supported on a mount mark, because they do not+ * carry enough information (i.e. path) to be filtered by mount+ * point.+ */
^ I think these should be one column more to the right to align
with the opening /*.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 10:34:01
On Wed 04-08-21 12:06:05, Gabriel Krisman Bertazi 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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 11:28:45
On Wed 04-08-21 12:06:08, 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 handler size and
if it is insuficient, we report an invalid FID and increase the
prediction for the next error slot allocation.
Hum, cannot we just preallocate MAX_HANDLE_SZ? It is wasteful but then I
don't expect error marks would be that frequent for this to really
matter and the code would be simpler.
Honza
quoted hunk
For errors that don't expose a file handler report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 27 +++++++++++++++++++++++++++
fs/notify/fanotify/fanotify.h | 12 ++++++++++++
fs/notify/fanotify/fanotify_user.c | 19 ++++++++++++++++++-
3 files changed, 57 insertions(+), 1 deletion(-)
@@ -705,7 +705,9 @@ static void fanotify_insert_error_event(struct fsnotify_group *group,{conststructfs_error_report*report=(structfs_error_report*)data;structfanotify_event*fae=FANOTIFY_E(event);+structinode*inode=report->inode;structfanotify_error_event*fee;+intfh_len;/* This might be an unexpected type of event (i.e. overflow). */if(!fanotify_is_error_event(fae->mask))
@@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group,fee->fae.type=FANOTIFY_EVENT_TYPE_FS_ERROR;fee->error=report->error;fee->err_count=1;+fee->fsid=fee->sb_mark->fsn_mark.connector->fsid;++/*+*Errorreportingneedstohappeninatomiccontext.Ifthis+*inode'sfilehandlerlengthismorethanweinitially+*predicted,thereisnothingbetterwecandothanreportthe+*errorwithabadfilehandler.+*/+fh_len=fanotify_encode_fh_len(inode);+if(fh_len>fee->sb_mark->pred_fh_len){+pr_warn_ratelimited(+"FH overflows error event. Drop inode information.\n");+/*+*Updatethehandlersizepredictionforthenexterror+*eventallocation.Thisreducesthechanceofanother+*overflow.+*/+fee->sb_mark->pred_fh_len=fh_len;++/* For the current error, ignore the inode information. */+inode=NULL;+fh_len=fanotify_encode_fh_len(NULL);+}++fanotify_encode_fh(&fee->object_fh,inode,fh_len,NULL,0);}/*
@@ -227,6 +228,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_fhisfollowedbyavariablesizedbuffer,soitmust+*bethelastelementofthisstructure.+*/+structfanotify_fhobject_fh;};staticinlinestructfanotify_error_event*
@@ -172,8 +172,25 @@ static struct fanotify_error_event *fanotify_alloc_error_event({structfanotify_error_event*fee;+structsuper_block*sb;-fee=kzalloc(sizeof(*fee),GFP_KERNEL_ACCOUNT);+if(!sb_mark->pred_fh_len){+/*+*Thehandlersizeisinitiallypredictedtobethe+*samesizeastherootinodefilehandler.Itcanbe+*increasedlaterifalargerfilehandlerisfound.+*/+sb=container_of(sb_mark->fsn_mark.connector->obj,+structsuper_block,s_fsnotify_marks);+sb_mark->pred_fh_len=+fanotify_encode_fh_len(sb->s_root->d_inode);+}++/* Guarantee there is always space to report an invalid FID. */+if(sb_mark->pred_fh_len<FANOTIFY_NULL_FH_LEN)+sb_mark->pred_fh_len=FANOTIFY_NULL_FH_LEN;++fee=kzalloc(sizeof(*fee)+sb_mark->pred_fh_len,GFP_KERNEL_ACCOUNT);if(!fee)returnNULL;
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 12:06:20
On Wed 04-08-21 12:06:08, 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 handler 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 handler report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
...
quoted hunk
@@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; fee->error = report->error; fee->err_count = 1;+ fee->fsid = fee->sb_mark->fsn_mark.connector->fsid;++ /*+ * Error reporting needs to happen in atomic context. If this+ * inode's file handler length is more than we initially+ * predicted, there is nothing better we can do than report the+ * error with a bad file handler.+ */+ fh_len = fanotify_encode_fh_len(inode);+ if (fh_len > fee->sb_mark->pred_fh_len) {+ pr_warn_ratelimited(+ "FH overflows error event. Drop inode information.\n");+ /*+ * Update the handler size prediction for the next error+ * event allocation. This reduces the chance of another+ * overflow.+ */+ fee->sb_mark->pred_fh_len = fh_len;++ /* For the current error, ignore the inode information. */+ inode = NULL;+ fh_len = fanotify_encode_fh_len(NULL);+ }++ fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0); }
I don't think calling fanotify_encode_fh() from under notification_lock is
a good practice. It calls into a filesystem and god knows which locks it may
need to take to get necessary info... See my reply to patch 18 how we could
handle that better.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 12:15:19
On Wed 04-08-21 12:06:07, Gabriel Krisman Bertazi 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.
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 replace the event in the slot with a new
structure, 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 new slot.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
The splitting of the patches really helped. Now I think I can grok much
more details than before :) Thanks! Some comments below.
@@ -681,6 +681,42 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)returnfsid;}+staticintfanotify_merge_error_event(structfsnotify_group*group,+structfsnotify_event*event)+{+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee=FANOTIFY_EE(fae);++/*+*Whenerr_count>0,thereportingslotisfull.Justaccount+*theadditionalerrorandaborttheinsertion.+*/+if(fee->err_count){+fee->err_count++;+return1;+}++return0;+}++staticvoidfanotify_insert_error_event(structfsnotify_group*group,+structfsnotify_event*event,+constvoid*data)+{+conststructfs_error_report*report=(structfs_error_report*)data;+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee;++/* This might be an unexpected type of event (i.e. overflow). */+if(!fanotify_is_error_event(fae->mask))+return;++fee=FANOTIFY_EE(fae);+fee->fae.type=FANOTIFY_EVENT_TYPE_FS_ERROR;+fee->error=report->error;+fee->err_count=1;+}+/**Addaneventtohashtableforfastermerge.*/
Hum, seeing this and how you had to extend fsnotify_add_event() to
accommodate this use, cannot we instead have something like:
if (fanotify_is_error_event(mask)) {
struct fanotify_sb_mark *sb_mark =
FANOTIFY_SB_MARK(fsnotify_iter_sb_mark(iter_info));
struct fanotify_error_event *event = &sb_mark->fee_slot;
bool queue = false;
spin_lock(&group->notification_lock);
/* Not yet queued? */
if (!event->err_count) {
fee->error = report->error;
queue = true;
}
event->err_count++;
spin_unlock(&group->notification_lock);
if (queue) {
... fill in other error info in 'event' such as fhandle
fsnotify_add_event(group, &event->fae.fse, NULL);
}
}
It would be IMHO simpler to follow what's going on and we don't have to
touch fsnotify_add_event(). I do recognize that due to races it may happen
that some racing fsnotify(FAN_FS_ERROR) call returns before the event is
actually visible in the event queue. It don't think it really matters but
if we wanted to be more careful, we would need to preformat fhandle into a
local buffer and only copy it into the event under notification_lock when
we see the event is unused.
quoted hunk
+/*+ * Replace a mark's error event with a new structure in preparation for+ * it to be dequeued. This is a bit annoying since we need to drop the+ * lock, so another thread might just steal the event from us.+ */+static int fanotify_replace_fs_error_event(struct fsnotify_group *group,+ struct fanotify_event *fae)+{+ struct fanotify_error_event *new, *fee = FANOTIFY_EE(fae);+ struct fanotify_sb_mark *sb_mark = fee->sb_mark;+ struct fsnotify_event *fse;++ pr_debug("%s: event=%p\n", __func__, fae);++ assert_spin_locked(&group->notification_lock);++ spin_unlock(&group->notification_lock);+ new = fanotify_alloc_error_event(sb_mark);+ spin_lock(&group->notification_lock);++ if (!new)+ return -ENOMEM;++ /*+ * Since we temporarily dropped the notification_lock, the event+ * might have been taken from under us and reported by another+ * reader. If that is the case, don't play games, just retry.+ */+ fse = fsnotify_peek_first_event(group);+ if (fse != &fae->fse) {+ kfree(new);+ return -EAGAIN;+ }++ sb_mark->fee_slot = new;++ return 0;+}+ /* * Get an fanotify notification event if one exists and is small * enough to fit in "count". Return an error pointer if the count
@@ -212,9 +252,21 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group, goto out; }+ if (fanotify_is_error_event(event->mask)) {+ /*+ * Replace the error event ahead of dequeueing so we+ * don't need to handle a incorrectly dequeued event.+ */+ ret = fanotify_replace_fs_error_event(group, event);+ if (ret) {+ event = ERR_PTR(ret);+ goto out;+ }+ }+
The replacing, retry, and all is hairy. Cannot we just keep the same event
attached to the sb mark and copy-out to on-stack buffer under
notification_lock in get_one_event()? The event is big (due to fhandle) but
fanotify_read() is not called from a deep call chain so we should have
enough space on stack for that.
quoted hunk
/*- * Held the notification_lock the whole time, so this is the- * same event we peeked above.+ * Even though we might have temporarily dropped the lock, this+ * is guaranteed to be the same event we peeked above. */ fsnotify_remove_first_event(group); if (fanotify_is_perm_event(event->mask))
@@ -596,6 +648,8 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, event = get_one_event(group, count); if (IS_ERR(event)) { ret = PTR_ERR(event);+ if (ret == -EAGAIN)+ continue; break; }
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-05 13:50:56
On Thu, Aug 5, 2021 at 3:15 PM Jan Kara [off-list ref] wrote:
On Wed 04-08-21 12:06:07, Gabriel Krisman Bertazi wrote:
quoted
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 replace the event in the slot with a new
structure, 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 new slot.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
The splitting of the patches really helped. Now I think I can grok much
more details than before :) Thanks! Some comments below.
@@ -681,6 +681,42 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)returnfsid;}+staticintfanotify_merge_error_event(structfsnotify_group*group,+structfsnotify_event*event)+{+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee=FANOTIFY_EE(fae);++/*+*Whenerr_count>0,thereportingslotisfull.Justaccount+*theadditionalerrorandaborttheinsertion.+*/+if(fee->err_count){+fee->err_count++;+return1;+}++return0;+}++staticvoidfanotify_insert_error_event(structfsnotify_group*group,+structfsnotify_event*event,+constvoid*data)+{+conststructfs_error_report*report=(structfs_error_report*)data;+structfanotify_event*fae=FANOTIFY_E(event);+structfanotify_error_event*fee;++/* This might be an unexpected type of event (i.e. overflow). */+if(!fanotify_is_error_event(fae->mask))+return;++fee=FANOTIFY_EE(fae);+fee->fae.type=FANOTIFY_EVENT_TYPE_FS_ERROR;+fee->error=report->error;+fee->err_count=1;+}+/**Addaneventtohashtableforfastermerge.*/
Hum, seeing this and how you had to extend fsnotify_add_event() to
accommodate this use, cannot we instead have something like:
if (fanotify_is_error_event(mask)) {
struct fanotify_sb_mark *sb_mark =
FANOTIFY_SB_MARK(fsnotify_iter_sb_mark(iter_info));
struct fanotify_error_event *event = &sb_mark->fee_slot;
bool queue = false;
spin_lock(&group->notification_lock);
/* Not yet queued? */
if (!event->err_count) {
fee->error = report->error;
queue = true;
}
event->err_count++;
spin_unlock(&group->notification_lock);
if (queue) {
... fill in other error info in 'event' such as fhandle
fsnotify_add_event(group, &event->fae.fse, NULL);
}
}
It would be IMHO simpler to follow what's going on and we don't have to
touch fsnotify_add_event(). I do recognize that due to races it may happen
that some racing fsnotify(FAN_FS_ERROR) call returns before the event is
actually visible in the event queue. It don't think it really matters but
if we wanted to be more careful, we would need to preformat fhandle into a
local buffer and only copy it into the event under notification_lock when
we see the event is unused.
quoted
+/*+ * Replace a mark's error event with a new structure in preparation for+ * it to be dequeued. This is a bit annoying since we need to drop the+ * lock, so another thread might just steal the event from us.+ */+static int fanotify_replace_fs_error_event(struct fsnotify_group *group,+ struct fanotify_event *fae)+{+ struct fanotify_error_event *new, *fee = FANOTIFY_EE(fae);+ struct fanotify_sb_mark *sb_mark = fee->sb_mark;+ struct fsnotify_event *fse;++ pr_debug("%s: event=%p\n", __func__, fae);++ assert_spin_locked(&group->notification_lock);++ spin_unlock(&group->notification_lock);+ new = fanotify_alloc_error_event(sb_mark);+ spin_lock(&group->notification_lock);++ if (!new)+ return -ENOMEM;++ /*+ * Since we temporarily dropped the notification_lock, the event+ * might have been taken from under us and reported by another+ * reader. If that is the case, don't play games, just retry.+ */+ fse = fsnotify_peek_first_event(group);+ if (fse != &fae->fse) {+ kfree(new);+ return -EAGAIN;+ }++ sb_mark->fee_slot = new;++ return 0;+}+ /* * Get an fanotify notification event if one exists and is small * enough to fit in "count". Return an error pointer if the count
@@ -212,9 +252,21 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group, goto out; }+ if (fanotify_is_error_event(event->mask)) {+ /*+ * Replace the error event ahead of dequeueing so we+ * don't need to handle a incorrectly dequeued event.+ */+ ret = fanotify_replace_fs_error_event(group, event);+ if (ret) {+ event = ERR_PTR(ret);+ goto out;+ }+ }+
The replacing, retry, and all is hairy. Cannot we just keep the same event
attached to the sb mark and copy-out to on-stack buffer under
notification_lock in get_one_event()? The event is big (due to fhandle) but
fanotify_read() is not called from a deep call chain so we should have
enough space on stack for that.
For the record, this was one of the first implementations from Gabriel.
When I proposed the double buffer implementation it was either that
or go back to copy to stack.
Given the complications, I agree that going back to copy to stack
is preferred.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-08-05 14:14:40
On Thu, Aug 5, 2021 at 1:24 PM Jan Kara [off-list ref] wrote:
On Wed 04-08-21 12:05:59, Gabriel Krisman Bertazi wrote:
quoted
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 directly against the super block instead of an
inode.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
There's a comment before fsnotify() declaration that states that 'either
@dir or @inode must be non-NULL' - in this patch it would be good time to
update that comment.
quoted
@@ -459,12 +460,13 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) * 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 qstr *file_name, struct inode *inode, u32 cookie)+int fsnotify(__u32 mask, const void *data, int data_type,+ struct super_block *sb, struct inode *dir,+ const struct qstr *file_name, struct inode *inode,+ u32 cookie) {
Two notes as ideas for consideration:
1) We could derive 'sb' from 'data'. I.e., have a helper like
fsnotify_data_sb(data, data_type). For FSNOTIFY_EVENT_PATH and
FSNOTIFY_EVENT_INODE this is easy to provide, for FSNOTIFY_EVENT_ERROR we
would have to add sb pointer to the structure but I guess that's easy. That
way we'd avoid the mostly NULL 'sb' argument. What do you guys think?
I think that's a great and simple idea that escaped me.
2) AFAICS 'inode' can be always derived from 'data' as well. So maybe we
can drop it Amir?
If only we could. The reason that we pass the allegedly redundant inode
argument is because there are two different distinguished inode
arguments:
1. The inode event happened on, which can be referenced from data
2. Inode that may be marked, which is passed in the inode argument
Particularly, dirent events carry the inode of the child as data, but
intentionally pass NULL inode arguments, because mark on inode
itself should not be getting e.g. FAN_DELETE event, but
audit_mark_handle_event() uses the child inode data.
If we wanted to, we could pass report_mask arg to fsnotify()
instead of inode arg and then fsnotify() will build iter_info
accordingly, but that sounds very complicated and doesn't gain
much.
There could be a simpler solution that I am missing...
Thanks,
Amir.
From: Jan Kara <jack@suse.cz> Date: 2021-08-05 15:55:32
On Thu 05-08-21 17:14:26, Amir Goldstein wrote:
quoted
2) AFAICS 'inode' can be always derived from 'data' as well. So maybe we
can drop it Amir?
If only we could. The reason that we pass the allegedly redundant inode
argument is because there are two different distinguished inode
arguments:
1. The inode event happened on, which can be referenced from data
2. Inode that may be marked, which is passed in the inode argument
Particularly, dirent events carry the inode of the child as data, but
intentionally pass NULL inode arguments, because mark on inode
itself should not be getting e.g. FAN_DELETE event, but
audit_mark_handle_event() uses the child inode data.
I see, thanks for explanation. I forgot that NULL 'inode' argument from
fsnotify_name() is actually needed for this to work.
If we wanted to, we could pass report_mask arg to fsnotify()
instead of inode arg and then fsnotify() will build iter_info
accordingly, but that sounds very complicated and doesn't gain
much.
Yeah. I'll think a bit more if we could simplify this but now I don't see
anything obvious.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR