From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:37:09
Hi,
This attempts to get the ball rolling again for the FAN_FS_ERROR. This
version is slightly different from the previous approaches, since it uses
mempool for memory allocation, as suggested by Jan. It has the
advantage of simplifying a lot the enqueue/dequeue, which is now much
more similar to other event types, but it also means the guarantee that
an error event will be available is diminished.
The way we propagate superblock errors also changed. Now we use
FILEID_ROOT internally, and mangle it prior to copy_to_user.
I am no longer sure how to guarantee that at least one mempoll slot will
be available for each filesystem. Since we are now tying the poll to
the entire group, a stream of errors in a single file system might
prevent others from emitting an error. The possibility of this is
reduced since we merge errors to the same filesystem, but it is still
possible that they occur during the small window where the event is
dequeued and before it is freed, in which case another filesystem might
not be able to obtain a slot.
I'm also creating a poll of 32 entries initially to avoid spending too
much memory. This means that only 32 filesystems can be watched per
group with the FAN_FS_ERROR mark, before fanotify_mark starts returning
ENOMEM.
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
Amir Goldstein (3):
fsnotify: pass data_type to fsnotify_name()
fsnotify: pass dentry instead of inode data
fsnotify: clarify contract for create event hooks
Gabriel Krisman Bertazi (25):
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
inotify: Don't force FS_IN_IGNORED
fsnotify: Add helper to detect overflow_event
fsnotify: Add wrapper around fsnotify_add_event
fsnotify: Retrieve super block from the data field
fsnotify: Pass group argument to free_event
fanotify: Support null inode event in fanotify_dfid_inode
fanotify: Allow file handle encoding for unhashed events
fanotify: Encode empty file handle when no inode is provided
fanotify: Require fid_mode for any non-fd event
fsnotify: Support FS_ERROR event type
fanotify: Reserve UAPI bits for FAN_FS_ERROR
fanotify: Pre-allocate pool of error events
fanotify: Limit number of marks with FAN_FS_ERROR per group
fanotify: Support enqueueing of error events
fanotify: Support merging of error events
fanotify: Report FID entry even for zero-length file_handle
fanotify: Report fid info for file related file system errors
fanotify: Emit generic error info for error event
fanotify: Allow users to request FAN_FS_ERROR events
ext4: Send notifications on error
samples: Add fs error monitoring example
docs: Document the FAN_FS_ERROR event
.../admin-guide/filesystem-monitoring.rst | 76 ++++++++
Documentation/admin-guide/index.rst | 1 +
fs/ext4/super.c | 8 +
fs/notify/fanotify/fanotify.c | 122 +++++++++++-
fs/notify/fanotify/fanotify.h | 31 +++-
fs/notify/fanotify/fanotify_user.c | 173 ++++++++++++++----
fs/notify/fsnotify.c | 7 +-
fs/notify/group.c | 2 +-
fs/notify/inotify/inotify_fsnotify.c | 5 +-
fs/notify/inotify/inotify_user.c | 6 +-
fs/notify/notification.c | 14 +-
include/linux/fanotify.h | 9 +-
include/linux/fsnotify.h | 58 ++++--
include/linux/fsnotify_backend.h | 96 +++++++++-
include/uapi/linux/fanotify.h | 8 +
kernel/audit_fsnotify.c | 3 +-
kernel/audit_watch.c | 3 +-
samples/Kconfig | 9 +
samples/Makefile | 1 +
samples/fanotify/Makefile | 5 +
samples/fanotify/fs-monitor.c | 142 ++++++++++++++
21 files changed, 685 insertions(+), 94 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.33.0
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:37:13
From: Amir Goldstein <amir73il@gmail.com>
Align the arguments of fsnotify_name() to those of fsnotify().
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify.h | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:37:19
From: Amir Goldstein <amir73il@gmail.com>
Define a new data type to pass for event - FSNOTIFY_EVENT_DENTRY.
Use it to pass the dentry instead of it's ->d_inode where available.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify.h | 5 ++---
include/linux/fsnotify_backend.h | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)}}+staticinlinestructdentry*fsnotify_data_dentry(constvoid*data,intdata_type)+{+switch(data_type){+caseFSNOTIFY_EVENT_DENTRY:+/* Non const is needed for dget() */+return(structdentry*)data;+caseFSNOTIFY_EVENT_PATH:+return((conststructpath*)data)->dentry;+default:+returnNULL;+}+}+staticinlineconststructpath*fsnotify_data_path(constvoid*data,intdata_type){
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:37:27
From: Amir Goldstein <amir73il@gmail.com>
Clarify argument names and contract for fsnotify_create() and
fsnotify_mkdir() to reflect the anomaly of kernfs, which leaves dentries
negavite after mkdir/create.
Remove the WARN_ON(!inode) in audit code that were added by the Fixes
commit under the wrong assumption that dentries cannot be negative after
mkdir/create.
Fixes: aa93bdc5500c ("fsnotify: use helpers to access data by data_type")
Link: https://lore.kernel.org/linux-fsdevel/87mtp5yz0q.fsf@collabora.com/
Reported-by: Gabriel Krisman Bertazi <redacted>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify.h | 22 ++++++++++++++++------
kernel/audit_fsnotify.c | 3 +--
kernel/audit_watch.c | 3 +--
3 files changed, 18 insertions(+), 10 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:37:32
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-10-14 21:37:42
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 v6:
- Rebase on top of pidfd patches
Changes since v1:
- rebased on top of hashing patches
---
fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++-------------
1 file changed, 20 insertions(+), 15 deletions(-)
@@ -126,17 +126,24 @@ static int fanotify_fid_info_len(int fh_len, int name_len)FANOTIFY_EVENT_ALIGN);}-staticintfanotify_event_info_len(unsignedintinfo_mode,-structfanotify_event*event)+staticsize_tfanotify_event_len(unsignedintinfo_mode,+structfanotify_event*event){-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(!info_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((info_mode&FAN_REPORT_NAME)&&(event->mask&FAN_ONDIR)){/*
@@ -147,12 +154,12 @@ static int fanotify_event_info_len(unsigned int info_mode,}if(info_mode&FAN_REPORT_PIDFD)-info_len+=FANOTIFY_PIDFD_INFO_HDR_LEN;+event_len+=FANOTIFY_PIDFD_INFO_HDR_LEN;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-10-14 21:37:48
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(-)
@@ -1328,6 +1327,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-10-14 21:37:54
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-10-14 21:38:00
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-10-14 21:38:08
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);
@@ -498,16 +498,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-10-14 21:38:13
Some file system events (i.e. FS_ERROR) might not be associated with an
inode or directory. For these, we can retrieve the super block from the
data field. But, since the super_block is available in the data field
on every event type, simplify the code to always retrieve it from there,
through a new helper.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
--
Changes since v6:
- Always use data field for superblock retrieval
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
---
fs/notify/fsnotify.c | 7 +++----
include/linux/fsnotify_backend.h | 15 +++++++++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:38:22
For group-wide mempool backed events, like FS_ERROR, the free_event
callback will need to reference the group's mempool to free the memory.
Wire that argument into the current callers.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 3 ++-
fs/notify/group.c | 2 +-
fs/notify/inotify/inotify_fsnotify.c | 3 ++-
fs/notify/notification.c | 2 +-
include/linux/fsnotify_backend.h | 2 +-
5 files changed, 7 insertions(+), 5 deletions(-)
@@ -155,7 +155,7 @@ struct fsnotify_ops {conststructqstr*file_name,u32cookie);void(*free_group_priv)(structfsnotify_group*group);void(*freeing_mark)(structfsnotify_mark*mark,structfsnotify_group*group);-void(*free_event)(structfsnotify_event*event);+void(*free_event)(structfsnotify_group*group,structfsnotify_event*event);/* called on final put+free to free memory */void(*free_mark)(structfsnotify_mark*mark);};
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:38:26
FAN_FS_ERROR doesn't support DFID, but this function is still called for
every event. The problem is that it is not capable of handling null
inodes, which now can happen in case of superblock error events. For
this case, just returning dir will be enough.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:38:32
Allow passing a NULL hash to fanotify_encode_fh and avoid calculating
the hash if not needed.
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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-10-14 21:38:45
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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- Use FILEID_ROOT as the internal value (jan)
- Create an empty FH (jan)
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 | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:38:51
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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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(-)
@@ -84,6 +84,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-10-14 21:38:58
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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- Add fsnotify_data_error_report
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)
---
include/linux/fsnotify.h | 13 +++++++++++++
include/linux/fsnotify_backend.h | 32 +++++++++++++++++++++++++++++++-
2 files changed, 44 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-10-14 21:39:04
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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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-10-14 21:39:11
Error reporting needs to be done in an atomic context. This patch
introduces a group-wide mempool of error events, shared by all
marks in this group.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 3 +++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++-
include/linux/fsnotify_backend.h | 2 ++
4 files changed, 41 insertions(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:17
Since FAN_FS_ERROR memory must be pre-allocated, limit a single group
from watching too many file systems at once. The current scheme
guarantees 1 slot per filesystem, so limit the number of marks with
FAN_FS_ERROR per group.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
include/linux/fsnotify_backend.h | 1 +
2 files changed, 11 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:24
Once an error event is triggered, collect the data from the fs error
report and enqueue it in the notification group, similarly to what is
done for other events. FAN_FS_ERROR is no longer handled specially,
since the memory is now handled by a preallocated mempool.
For now, make the event unhashed. A future patch implements merging for
these kinds of events.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
fs/notify/fanotify/fanotify.h | 6 ++++++
2 files changed, 41 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:32
Error events (FAN_FS_ERROR) against the same file system can be merged
by simply iterating the error count. The hash is taken from the fsid,
without considering the FH. This means that only the first error object
is reported.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
fs/notify/fanotify/fanotify.h | 4 +++-
2 files changed, 39 insertions(+), 4 deletions(-)
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,returnfanotify_info_equal(info1,info2);}+staticboolfanotify_error_event_equal(structfanotify_error_event*fee1,+structfanotify_error_event*fee2)+{+/* Error events against the same file system are always merged. */+if(!fanotify_fsid_equal(&fee1->fsid,&fee2->fsid))+returnfalse;++returntrue;+}+staticboolfanotify_should_merge(structfanotify_event*old,structfanotify_event*new){
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:37
Non-inode errors will reported with an empty file_handle. In
preparation for that, allow some events to print the FID record even if
there isn't any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)FANOTIFY_EVENT_ALIGN);}+staticboolfanotify_event_allows_empty_fh(structfanotify_event*event)+{+switch(event->type){+caseFANOTIFY_EVENT_TYPE_FS_ERROR:+returntrue;+default:+returnfalse;+}+}+staticsize_tfanotify_event_len(unsignedintinfo_mode,structfanotify_event*event){
@@ -157,7 +167,7 @@ static size_t fanotify_event_len(unsigned int info_mode,if(info_mode&FAN_REPORT_PIDFD)event_len+=FANOTIFY_PIDFD_INFO_HDR_LEN;-if(fh_len)+if(fh_len||fanotify_event_allows_empty_fh(event))event_len+=fanotify_fid_info_len(fh_len,dot_len);returnevent_len;
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:44
Plumb the pieces to add a FID report to error records. Since all error
event memory must be pre-allocated, we pre-allocate the maximum file
handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- pass fsid from handle_events
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 | 8 ++++++++
2 files changed, 23 insertions(+)
@@ -202,6 +202,10 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */__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-10-14 21:39:51
The error info 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 v6:
- Rebase on top of pidfd patches
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 | 35 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 44 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:39:57
Wire up the FAN_FS_ERROR event in the fanotify_mark syscall, allowing
user space to request the monitoring of FAN_FS_ERROR events.
These events are limited to filesystem marks, so check it is the
case in the syscall handler.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 2 +-
fs/notify/fanotify/fanotify_user.c | 5 +++++
include/linux/fanotify.h | 6 +++++-
3 files changed, 11 insertions(+), 2 deletions(-)
@@ -1585,6 +1585,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;
@@ -91,9 +91,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-10-14 21:40:05
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>
---
Changes since v6:
- Report ext4_std_errors agains superblock (jan)
---
fs/ext4/super.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-14 21:40:16
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 v7:
- Update semantics
Changes Since v6:
- English fixes (jan)
- Proper document error field (jan)
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 | 76 +++++++++++++++++++
Documentation/admin-guide/index.rst | 1 +
2 files changed, 77 insertions(+)
create mode 100644 Documentation/admin-guide/filesystem-monitoring.rst
@@ -0,0 +1,76 @@+.. SPDX-License-Identifier: GPL-2.0++====================================+File system Monitoring with fanotify+====================================++File system Error Reporting+===========================++Fanotify supports the FAN_FS_ERROR event type for file system-wide error+reporting. It is meant to be used by file system health monitoring+daemons, which listen for these events and take actions (notify+sysadmin, start recovery) when a file system problem is detected.++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 out of+scope for 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 tries to report only the first+error that occurred for a process since the last notification, and it+simply counts additional errors. This ensures that the most important+pieces of information are 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 error in a file-system specific way.+Ext4, for instance, which is the only file system implementing this+interface at the time of this writing, exposes EXT4_ERR_ values in this+field. Please refer to the file system documentation for the meaning of+specific error codes.++`error_count` tracks the number of errors that occurred and were+suppressed to preserve the original error information, 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 of type FILEID_INVALID.
On Thu, Oct 14, 2021 at 06:36:44PM -0300, 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>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 05:39:54
On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Some file system events (i.e. FS_ERROR) might not be associated with an
inode or directory. For these, we can retrieve the super block from the
data field. But, since the super_block is available in the data field
on every event type, simplify the code to always retrieve it from there,
through a new helper.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
--
Changes since v6:
- Always use data field for superblock retrieval
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
---
fs/notify/fsnotify.c | 7 +++----
include/linux/fsnotify_backend.h | 15 +++++++++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 05:41:08
On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
For group-wide mempool backed events, like FS_ERROR, the free_event
callback will need to reference the group's mempool to free the memory.
Wire that argument into the current callers.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -155,7 +155,7 @@ struct fsnotify_ops {conststructqstr*file_name,u32cookie);void(*free_group_priv)(structfsnotify_group*group);void(*freeing_mark)(structfsnotify_mark*mark,structfsnotify_group*group);-void(*free_event)(structfsnotify_event*event);+void(*free_event)(structfsnotify_group*group,structfsnotify_event*event);/* called on final put+free to free memory */void(*free_mark)(structfsnotify_mark*mark);};--
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 05:49:14
On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
FAN_FS_ERROR doesn't support DFID, but this function is still called for
every event. The problem is that it is not capable of handling null
inodes, which now can happen in case of superblock error events. For
this case, just returning dir will be enough.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Please also fortify fsnotify_handle_inode_event() against calling
->handle_inode_event() with NULL inode.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 06:03:01
On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
---
Changes since v6:
- Use FILEID_ROOT as the internal value (jan)
- Create an empty FH (jan)
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 | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 06:16:00
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Since FAN_FS_ERROR memory must be pre-allocated, limit a single group
from watching too many file systems at once. The current scheme
guarantees 1 slot per filesystem, so limit the number of marks with
FAN_FS_ERROR per group.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
include/linux/fsnotify_backend.h | 1 +
2 files changed, 11 insertions(+)
Why not try to mempool_resize()?
Also, I did not read the rest of the patches yet, but don't we need two
slots per mark? one for alloc-pre-enqueue and one for free-post-dequeue?
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 06:20:02
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Error reporting needs to be done in an atomic context. This patch
introduces a group-wide mempool of error events, shared by all
marks in this group.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 3 +++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++-
include/linux/fsnotify_backend.h | 2 ++
4 files changed, 41 insertions(+), 1 deletion(-)
We can probably start with a more generous pool (128?)
It doesn't cost that much.
But anyway, I think this pool needs to auto-grow (up to a maximum size)
instead of having a rigid arbitrary limit.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 07:04:14
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Once an error event is triggered, collect the data from the fs error
report and enqueue it in the notification group, similarly to what is
done for other events. FAN_FS_ERROR is no longer handled specially,
since the memory is now handled by a preallocated mempool.
For now, make the event unhashed. A future patch implements merging for
these kinds of events.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
fs/notify/fanotify/fanotify.h | 6 ++++++
2 files changed, 41 insertions(+)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 07:09:49
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Error events (FAN_FS_ERROR) against the same file system can be merged
by simply iterating the error count. The hash is taken from the fsid,
without considering the FH. This means that only the first error object
is reported.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
fs/notify/fanotify/fanotify.h | 4 +++-
2 files changed, 39 insertions(+), 4 deletions(-)
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,returnfanotify_info_equal(info1,info2);}+staticboolfanotify_error_event_equal(structfanotify_error_event*fee1,+structfanotify_error_event*fee2)+{+/* Error events against the same file system are always merged. */+if(!fanotify_fsid_equal(&fee1->fsid,&fee2->fsid))+returnfalse;++returntrue;+}+staticboolfanotify_should_merge(structfanotify_event*old,structfanotify_event*new){
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 07:34:03
On Fri, Oct 15, 2021 at 9:19 AM Amir Goldstein [off-list ref] wrote:
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Error reporting needs to be done in an atomic context. This patch
introduces a group-wide mempool of error events, shared by all
marks in this group.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 3 +++
fs/notify/fanotify/fanotify.h | 11 +++++++++++
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++-
include/linux/fsnotify_backend.h | 2 ++
4 files changed, 41 insertions(+), 1 deletion(-)
We can probably start with a more generous pool (128?)
It doesn't cost that much.
But anyway, I think this pool needs to auto-grow (up to a maximum size)
instead of having a rigid arbitrary limit.
As long as the pool grows, I don't mind if it start at size 32,
but I just noticed that mempools cannot be accounted to memcg??
Then surely the maximum size need to be kept pretty low.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 07:56:51
On Fri, Oct 15, 2021 at 12:39 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 pre-allocate the maximum file
handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- pass fsid from handle_events
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 | 8 ++++++++
2 files changed, 23 insertions(+)
@@ -202,6 +202,10 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */__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];};
This struct duplicates most of struct fanotify_fid_event.
How about:
#define FANOTIFY_ERROR_FH_LEN \
(MAX_HANDLE_SZ - FANOTIFY_INLINE_FH_LEN)
struct fanotify_error_event {
u32 err_count; /* Suppressed errors count */
struct fanotify_event ffe;
/* Reserve space in ffe.object_fh.buf[] - access with
fanotify_fh_buf() */
unsigned char _fh_buf[FANOTIFY_ERROR_FH_LEN];
}
Or leaving out the struct padding and passing
FANOTIFY_ERROR_EVENT_SIZE as mempool object size?
#define FANOTIFY_ERROR_EVENT_SIZE \
(sizeof(struct fanotify_error_event) + FANOTIFY_ERROR_FH_LEN)
You do not have to make this change - it is a proposal that can have
supporters and objectors, so let's wait to see what you and other reviewers
have to say.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 08:11:24
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Non-inode errors will reported with an empty file_handle. In
preparation for that, allow some events to print the FID record even if
there isn't any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)FANOTIFY_EVENT_ALIGN);}+staticboolfanotify_event_allows_empty_fh(structfanotify_event*event)+{+switch(event->type){+caseFANOTIFY_EVENT_TYPE_FS_ERROR:+returntrue;+default:+returnfalse;+}+}+staticsize_tfanotify_event_len(unsignedintinfo_mode,structfanotify_event*event){
@@ -157,7 +167,7 @@ static size_t fanotify_event_len(unsigned int info_mode,if(info_mode&FAN_REPORT_PIDFD)event_len+=FANOTIFY_PIDFD_INFO_HDR_LEN;-if(fh_len)+if(fh_len||fanotify_event_allows_empty_fh(event))event_len+=fanotify_fid_info_len(fh_len,dot_len);returnevent_len;
@@ -375,6 +382,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,handle.handle_type=fh->type;handle.handle_bytes=fh_len;++/* Mangle handle_type for bad file_handle */+if(!fh_len)+handle.handle_type=FILEID_INVALID;+if(copy_to_user(buf,&handle,sizeof(handle)))return-EFAULT;
@@ -467,7 +479,8 @@ static int copy_info_records_to_user(struct fanotify_event *event,total_bytes+=ret;}-if(fanotify_event_object_fh_len(event)){+if(fanotify_event_object_fh_len(event)||+fanotify_event_allows_empty_fh(event)){constchar*dot=NULL;intdot_len=0;
I don't like this fanotify_event_allows_empty_fh() implementation so much.
How about this instead:
static inline struct fanotify_fh *fanotify_event_object_fh(
struct fanotify_event *event)
{
struct fanotify_fh *fh = NULL;
/* An error event encodes (a FILEID_INVAL) fh for an empty fh */
if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
return &FANOTIFY_EE(event)->object_fh;
else if (event->type == FANOTIFY_EVENT_TYPE_FID)
fh = &FANOTIFY_FE(event)->object_fh;
else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
fh = fanotify_info_file_fh(&FANOTIFY_NE(event)->info);
if (!fh && !fh->len)
return NULL;
return fh;
}
struct fanotify_fh *object_fh = fanotify_event_object_fh(event);
...
- if (fanotify_event_object_fh_len(event)) {
+ if (object_fh) {
const char *dot = NULL;
...
ret = copy_fid_info_to_user(fanotify_event_fsid(event),
- fanotify_event_object_fh(event),
+ object_fh,
info_type, dot, dot_len,
buf, count);
...
And similar change to fanotify_event_len()
This way, the logic of whether to report fh or not is encoded in
fanotify_event_object_fh() and fanotify_event_object_fh_len()
goes back to being a property of the the fh report.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 08:13:56
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
The error info 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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
---
Changes since v6:
- Rebase on top of pidfd patches
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 | 35 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 44 insertions(+)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 08:27:53
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Wire up the FAN_FS_ERROR event in the fanotify_mark syscall, allowing
user space to request the monitoring of FAN_FS_ERROR events.
These events are limited to filesystem marks, so check it is the
case in the syscall handler.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 2 +-
fs/notify/fanotify/fanotify_user.c | 5 +++++
include/linux/fanotify.h | 6 +++++-
3 files changed, 11 insertions(+), 2 deletions(-)
@@ -1585,6 +1585,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;+}+
Please move this up to the section where input args validity is checked
(i.e. before or after FANOTIFY_PERM_EVENTS check).
It is the correct context for this sort of check and ret is already
set to -EINVAL for the entire section.
Thanks,
Amir.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-15 08:38:22
On Fri, Oct 15, 2021 at 12:37 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Hi,
This attempts to get the ball rolling again for the FAN_FS_ERROR. This
version is slightly different from the previous approaches, since it uses
mempool for memory allocation, as suggested by Jan. It has the
advantage of simplifying a lot the enqueue/dequeue, which is now much
more similar to other event types, but it also means the guarantee that
an error event will be available is diminished.
Makes me very happy not having to worry about new enqueue/dequeue bugs :)
The way we propagate superblock errors also changed. Now we use
FILEID_ROOT internally, and mangle it prior to copy_to_user.
I am no longer sure how to guarantee that at least one mempoll slot will
be available for each filesystem. Since we are now tying the poll to
the entire group, a stream of errors in a single file system might
prevent others from emitting an error. The possibility of this is
reduced since we merge errors to the same filesystem, but it is still
possible that they occur during the small window where the event is
dequeued and before it is freed, in which case another filesystem might
not be able to obtain a slot.
Double buffering. Each mark/fs should have one slot reserved for equeue
and one reserved for copying the event to user.
I'm also creating a poll of 32 entries initially to avoid spending too
much memory. This means that only 32 filesystems can be watched per
group with the FAN_FS_ERROR mark, before fanotify_mark starts returning
ENOMEM.
I don't see a problem to grow the pool dynamically up to a reasonable
size, although it is a shame that the pool is not accounted to the group's
memcg (I think?).
Overall, the series looks very good to me, modulo to above comments
about the mempool size/resize and a few minor implementation details.
Good job!
Thanks,
Amir.
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:16:50
Hi!
On Thu 14-10-21 18:36:18, Gabriel Krisman Bertazi wrote:
This attempts to get the ball rolling again for the FAN_FS_ERROR. This
version is slightly different from the previous approaches, since it uses
mempool for memory allocation, as suggested by Jan. It has the
advantage of simplifying a lot the enqueue/dequeue, which is now much
more similar to other event types, but it also means the guarantee that
an error event will be available is diminished.
The way we propagate superblock errors also changed. Now we use
FILEID_ROOT internally, and mangle it prior to copy_to_user.
I am no longer sure how to guarantee that at least one mempoll slot will
be available for each filesystem. Since we are now tying the poll to
the entire group, a stream of errors in a single file system might
prevent others from emitting an error. The possibility of this is
reduced since we merge errors to the same filesystem, but it is still
possible that they occur during the small window where the event is
dequeued and before it is freed, in which case another filesystem might
not be able to obtain a slot.
Yes, but this happening would mean we hit this race on one fs, error on
another fs, and ENOMEM with GFP_NOFS allocation to top it. Not very likely
IMO. Also in that case we will queue overflow event in
fanotify_handle_event() so it will not be silent loss. The listening
application will learn that it missed some events.
I'm also creating a poll of 32 entries initially to avoid spending too
much memory. This means that only 32 filesystems can be watched per
group with the FAN_FS_ERROR mark, before fanotify_mark starts returning
ENOMEM.
We can consider auto-grow as Amir suggests but I also think you somewhat
misunderstand how mempools work. If you call mempool_alloc(), it will first
try to allocate memory with kmalloc() (using GFP_NOFS mask which you pass
to it). In 99.9% of cases this just succeeds. If kmalloc() fails, only
then mempool_alloc() will take one of the preallocated events and return
it. So even with mempool of size 32, we will not usually run out of events
when we have more than 32 filesystems. But it is true we cannot guarantee
reporting error to more than 32 filesystems under ENOMEM conditions. I'm
not sure if that matters...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:18:37
On Thu 14-10-21 18:36:19, Gabriel Krisman Bertazi wrote:
From: Amir Goldstein <amir73il@gmail.com>
Align the arguments of fsnotify_name() to those of fsnotify().
Signed-off-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
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:21:20
On Thu 14-10-21 18:36:21, Gabriel Krisman Bertazi wrote:
From: Amir Goldstein <amir73il@gmail.com>
Clarify argument names and contract for fsnotify_create() and
fsnotify_mkdir() to reflect the anomaly of kernfs, which leaves dentries
negavite after mkdir/create.
Remove the WARN_ON(!inode) in audit code that were added by the Fixes
commit under the wrong assumption that dentries cannot be negative after
mkdir/create.
Fixes: aa93bdc5500c ("fsnotify: use helpers to access data by data_type")
Link: https://lore.kernel.org/linux-fsdevel/87mtp5yz0q.fsf@collabora.com/
Reported-by: Gabriel Krisman Bertazi <redacted>
Signed-off-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
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:26:19
On Thu 14-10-21 18:36:28, Gabriel Krisman Bertazi wrote:
Some file system events (i.e. FS_ERROR) might not be associated with an
inode or directory. For these, we can retrieve the super block from the
data field. But, since the super_block is available in the data field
on every event type, simplify the code to always retrieve it from there,
through a new helper.
Suggested-by: Jan Kara <jack@suse.cz>
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 v6:
- Always use data field for superblock retrieval
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
---
fs/notify/fsnotify.c | 7 +++----
include/linux/fsnotify_backend.h | 15 +++++++++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:26:58
On Thu 14-10-21 18:36:29, Gabriel Krisman Bertazi wrote:
For group-wide mempool backed events, like FS_ERROR, the free_event
callback will need to reference the group's mempool to free the memory.
Wire that argument into the current callers.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
@@ -155,7 +155,7 @@ struct fsnotify_ops {conststructqstr*file_name,u32cookie);void(*free_group_priv)(structfsnotify_group*group);void(*freeing_mark)(structfsnotify_mark*mark,structfsnotify_group*group);-void(*free_event)(structfsnotify_event*event);+void(*free_event)(structfsnotify_group*group,structfsnotify_event*event);/* called on final put+free to free memory */void(*free_mark)(structfsnotify_mark*mark);};
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:30:36
On Thu 14-10-21 18:36:30, Gabriel Krisman Bertazi wrote:
FAN_FS_ERROR doesn't support DFID, but this function is still called for
every event. The problem is that it is not capable of handling null
inodes, which now can happen in case of superblock error events. For
this case, just returning dir will be enough.
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-10-15 09:32:50
On Thu 14-10-21 18:36:32, Gabriel Krisman Bertazi wrote:
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.
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 v6:
- Use FILEID_ROOT as the internal value (jan)
- Create an empty FH (jan)
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 | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:37:04
On Thu 14-10-21 18:36:35, Gabriel Krisman Bertazi wrote:
FAN_FS_ERROR allows reporting of event type FS_ERROR to userspace, which
^^
missing 'is'
quoted hunk
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>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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: Jan Kara <jack@suse.cz> Date: 2021-10-15 09:46:38
On Thu 14-10-21 18:36:36, Gabriel Krisman Bertazi wrote:
Error reporting needs to be done in an atomic context. This patch
So this is a requirement I was advocating to remove because although atomic
context is nice for filesystems, it is rather difficult for fanotify.
Instead I was advocating that we relax this and require filesystems to
report errors from a context where using GFP_NOFS allocations is fine. At
least for ext4 and xfs and other filesystems I know it isn't really that
much harder... If this proves to be a problem in some specific case, there
are also other options (like doing notification from a workqueue, events are
async anyway) for the filesystems to take.
introduces a group-wide mempool of error events, shared by all
marks in this group.
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 12:34:48
On Thu 14-10-21 18:36:38, Gabriel Krisman Bertazi wrote:
Once an error event is triggered, collect the data from the fs error
report and enqueue it in the notification group, similarly to what is
done for other events. FAN_FS_ERROR is no longer handled specially,
since the memory is now handled by a preallocated mempool.
For now, make the event unhashed. A future patch implements merging for
these kinds of events.
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-10-15 12:43:40
On Thu 14-10-21 18:36:39, Gabriel Krisman Bertazi wrote:
Error events (FAN_FS_ERROR) against the same file system can be merged
by simply iterating the error count. The hash is taken from the fsid,
without considering the FH. This means that only the first error object
is reported.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, if (++i > FANOTIFY_MAX_MERGE_EVENTS) break; if (fanotify_should_merge(old, new)) {- old->mask |= new->mask;+ fanotify_merge_event(old, new);
I guess no need for two functions (fanotify_merge_event(),
fanotify_merge_error_event()) for three lines of code? I'd just put those
three lines into fanotify_merge().
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 12:47:04
On Thu 14-10-21 18:36:42, Gabriel Krisman Bertazi wrote:
The error info 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 to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
Changes since v6:
- Rebase on top of pidfd patches
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 | 35 ++++++++++++++++++++++++++++++
include/uapi/linux/fanotify.h | 7 ++++++
4 files changed, 44 insertions(+)
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 12:49:10
On Thu 14-10-21 18:36:43, Gabriel Krisman Bertazi wrote:
Wire up the FAN_FS_ERROR event in the fanotify_mark syscall, allowing
user space to request the monitoring of FAN_FS_ERROR events.
These events are limited to filesystem marks, so check it is the
case in the syscall handler.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
No other comment besides what Amir has written...
Honza
@@ -1585,6 +1585,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;
@@ -91,9 +91,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: Jan Kara <jack@suse.cz> Date: 2021-10-15 13:13:48
On Fri 15-10-21 11:10:58, Amir Goldstein wrote:
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Non-inode errors will reported with an empty file_handle. In
preparation for that, allow some events to print the FID record even if
there isn't any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
@@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)FANOTIFY_EVENT_ALIGN);}+staticboolfanotify_event_allows_empty_fh(structfanotify_event*event)+{+switch(event->type){+caseFANOTIFY_EVENT_TYPE_FS_ERROR:+returntrue;+default:+returnfalse;+}+}+staticsize_tfanotify_event_len(unsignedintinfo_mode,structfanotify_event*event){
@@ -157,7 +167,7 @@ static size_t fanotify_event_len(unsigned int info_mode,if(info_mode&FAN_REPORT_PIDFD)event_len+=FANOTIFY_PIDFD_INFO_HDR_LEN;-if(fh_len)+if(fh_len||fanotify_event_allows_empty_fh(event))event_len+=fanotify_fid_info_len(fh_len,dot_len);returnevent_len;
@@ -375,6 +382,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,handle.handle_type=fh->type;handle.handle_bytes=fh_len;++/* Mangle handle_type for bad file_handle */+if(!fh_len)+handle.handle_type=FILEID_INVALID;+if(copy_to_user(buf,&handle,sizeof(handle)))return-EFAULT;
@@ -467,7 +479,8 @@ static int copy_info_records_to_user(struct fanotify_event *event,total_bytes+=ret;}-if(fanotify_event_object_fh_len(event)){+if(fanotify_event_object_fh_len(event)||+fanotify_event_allows_empty_fh(event)){constchar*dot=NULL;intdot_len=0;
I don't like this fanotify_event_allows_empty_fh() implementation so much.
How about this instead:
static inline struct fanotify_fh *fanotify_event_object_fh(
struct fanotify_event *event)
{
struct fanotify_fh *fh = NULL;
/* An error event encodes (a FILEID_INVAL) fh for an empty fh */
if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
return &FANOTIFY_EE(event)->object_fh;
else if (event->type == FANOTIFY_EVENT_TYPE_FID)
fh = &FANOTIFY_FE(event)->object_fh;
else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
fh = fanotify_info_file_fh(&FANOTIFY_NE(event)->info);
if (!fh && !fh->len)
return NULL;
return fh;
}
struct fanotify_fh *object_fh = fanotify_event_object_fh(event);
...
- if (fanotify_event_object_fh_len(event)) {
+ if (object_fh) {
const char *dot = NULL;
...
ret = copy_fid_info_to_user(fanotify_event_fsid(event),
- fanotify_event_object_fh(event),
+ object_fh,
info_type, dot, dot_len,
buf, count);
...
And similar change to fanotify_event_len()
This way, the logic of whether to report fh or not is encoded in
fanotify_event_object_fh() and fanotify_event_object_fh_len()
goes back to being a property of the the fh report.
I like this except that AFAICT this will be problematic for
fanotify_event_object_fh_len() because there we want to return 0 (length of
file handle buffer to copy) for error events - so "copy just header to
userspace" is going to be indistiguishable from "copy nothing".
But maybe we need helpers fanotify_event_has_object_fh() and
fanotify_event_has_dir_fh() (for symmetry) instead of directly checking
fh_len? These would then encapsulate all the magic for error events.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 13:38:37
On Fri 15-10-21 10:56:38, Amir Goldstein wrote:
On Fri, Oct 15, 2021 at 12:39 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 pre-allocate the maximum file
handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid
FID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- pass fsid from handle_events
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 | 8 ++++++++
2 files changed, 23 insertions(+)
@@ -202,6 +202,10 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */__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];};
This struct duplicates most of struct fanotify_fid_event.
How about:
#define FANOTIFY_ERROR_FH_LEN \
(MAX_HANDLE_SZ - FANOTIFY_INLINE_FH_LEN)
struct fanotify_error_event {
u32 err_count; /* Suppressed errors count */
struct fanotify_event ffe;
/* Reserve space in ffe.object_fh.buf[] - access with
fanotify_fh_buf() */
unsigned char _fh_buf[FANOTIFY_ERROR_FH_LEN];
}
Or leaving out the struct padding and passing
FANOTIFY_ERROR_EVENT_SIZE as mempool object size?
#define FANOTIFY_ERROR_EVENT_SIZE \
(sizeof(struct fanotify_error_event) + FANOTIFY_ERROR_FH_LEN)
Hrm, I don't like either of these two options too much as IMHO it's rather
hard to understand what's going on. If we want to avoid the duplication,
then I see two relatively clean ways to do it:
a) Remove _inline_fh_buf from fanotify_fid_event (probably just leave a
comment there explaining how space is preallocated) and make sure kmem
cache for fanotify_fid_event has FANOTIFY_INLINE_FH_LEN in each object for
the fh, similarly error event would have MAX_HANDLE_SZ in the object.
b) Define a macro that expands to a struct definition with appropriate
buffer length.
I guess a) seems a bit more obvious to me but I can live with both...
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-15 13:39:54
On Thu 14-10-21 18:36:20, Gabriel Krisman Bertazi wrote:
From: Amir Goldstein <amir73il@gmail.com>
Define a new data type to pass for event - FSNOTIFY_EVENT_DENTRY.
Use it to pass the dentry instead of it's ->d_inode where available.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)}}+staticinlinestructdentry*fsnotify_data_dentry(constvoid*data,intdata_type)+{+switch(data_type){+caseFSNOTIFY_EVENT_DENTRY:+/* Non const is needed for dget() */+return(structdentry*)data;+caseFSNOTIFY_EVENT_PATH:+return((conststructpath*)data)->dentry;+default:+returnNULL;+}+}+staticinlineconststructpath*fsnotify_data_path(constvoid*data,intdata_type){
From: Jan Kara <jack@suse.cz> Date: 2021-10-18 09:11:56
On Thu 14-10-21 18:36:20, Gabriel Krisman Bertazi wrote:
From: Amir Goldstein <amir73il@gmail.com>
Define a new data type to pass for event - FSNOTIFY_EVENT_DENTRY.
Use it to pass the dentry instead of it's ->d_inode where available.
Signed-off-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
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)}}+staticinlinestructdentry*fsnotify_data_dentry(constvoid*data,intdata_type)+{+switch(data_type){+caseFSNOTIFY_EVENT_DENTRY:+/* Non const is needed for dget() */+return(structdentry*)data;+caseFSNOTIFY_EVENT_PATH:+return((conststructpath*)data)->dentry;+default:+returnNULL;+}+}+staticinlineconststructpath*fsnotify_data_path(constvoid*data,intdata_type){