[PATCH v7 00/28] file system-wide error monitoring

[PATCH v7 00/28] file system-wide error monitoring

[Gabriel Krisman Bertazi]

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

[PATCH v7 01/28] fsnotify: pass data_type to fsnotify_name()

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 include/linux/fsnotify.h | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 12d3a7d308ab..d1144d7c3536 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -26,20 +26,21 @@
  * FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only
  * the child is interested and not the parent.
  */
-static inline void fsnotify_name(struct inode *dir, __u32 mask,
-				 struct inode *child,
-				 const struct qstr *name, u32 cookie)
+static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
+				struct inode *dir, const struct qstr *name,
+				u32 cookie)
 {
 	if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
-		return;
+		return 0;
 
-	fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie);
+	return fsnotify(mask, data, data_type, dir, name, NULL, cookie);
 }
 
 static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 				   __u32 mask)
 {
-	fsnotify_name(dir, mask, d_inode(dentry), &dentry->d_name, 0);
+	fsnotify_name(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
+		      dir, &dentry->d_name, 0);
 }
 
 static inline void fsnotify_inode(struct inode *inode, __u32 mask)
@@ -154,8 +155,10 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 		new_dir_mask |= FS_ISDIR;
 	}
 
-	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
-	fsnotify_name(new_dir, new_dir_mask, source, new_name, fs_cookie);
+	fsnotify_name(old_dir_mask, source, FSNOTIFY_EVENT_INODE,
+		      old_dir, old_name, fs_cookie);
+	fsnotify_name(new_dir_mask, source, FSNOTIFY_EVENT_INODE,
+		      new_dir, new_name, fs_cookie);
 
 	if (target)
 		fsnotify_link_count(target);
@@ -209,7 +212,8 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode,
 	fsnotify_link_count(inode);
 	audit_inode_child(dir, new_dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	fsnotify_name(dir, FS_CREATE, inode, &new_dentry->d_name, 0);
+	fsnotify_name(FS_CREATE, inode, FSNOTIFY_EVENT_INODE,
+		      dir, &new_dentry->d_name, 0);
 }
 
 /*
-- 
2.33.0

Re: [PATCH v7 01/28] fsnotify: pass data_type to fsnotify_name()

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/fsnotify.h | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 12d3a7d308ab..d1144d7c3536 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -26,20 +26,21 @@
>   * FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only
>   * the child is interested and not the parent.
>   */
> -static inline void fsnotify_name(struct inode *dir, __u32 mask,
> -				 struct inode *child,
> -				 const struct qstr *name, u32 cookie)
> +static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
> +				struct inode *dir, const struct qstr *name,
> +				u32 cookie)
>  {
>  	if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
> -		return;
> +		return 0;
>  
> -	fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie);
> +	return fsnotify(mask, data, data_type, dir, name, NULL, cookie);
>  }
>  
>  static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
>  				   __u32 mask)
>  {
> -	fsnotify_name(dir, mask, d_inode(dentry), &dentry->d_name, 0);
> +	fsnotify_name(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> +		      dir, &dentry->d_name, 0);
>  }
>  
>  static inline void fsnotify_inode(struct inode *inode, __u32 mask)
> @@ -154,8 +155,10 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  		new_dir_mask |= FS_ISDIR;
>  	}
>  
> -	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
> -	fsnotify_name(new_dir, new_dir_mask, source, new_name, fs_cookie);
> +	fsnotify_name(old_dir_mask, source, FSNOTIFY_EVENT_INODE,
> +		      old_dir, old_name, fs_cookie);
> +	fsnotify_name(new_dir_mask, source, FSNOTIFY_EVENT_INODE,
> +		      new_dir, new_name, fs_cookie);
>  
>  	if (target)
>  		fsnotify_link_count(target);
> @@ -209,7 +212,8 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode,
>  	fsnotify_link_count(inode);
>  	audit_inode_child(dir, new_dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify_name(dir, FS_CREATE, inode, &new_dentry->d_name, 0);
> +	fsnotify_name(FS_CREATE, inode, FSNOTIFY_EVENT_INODE,
> +		      dir, &new_dentry->d_name, 0);
>  }
>  
>  /*
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 02/28] fsnotify: pass dentry instead of inode data

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 include/linux/fsnotify.h         |  5 ++---
 include/linux/fsnotify_backend.h | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index d1144d7c3536..df0fa4687a18 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -39,8 +39,7 @@ static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
 static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 				   __u32 mask)
 {
-	fsnotify_name(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
-		      dir, &dentry->d_name, 0);
+	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name, 0);
 }
 
 static inline void fsnotify_inode(struct inode *inode, __u32 mask)
@@ -87,7 +86,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
  */
 static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
 {
-	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE);
+	fsnotify_parent(dentry, mask, dentry, FSNOTIFY_EVENT_DENTRY);
 }
 
 static inline int fsnotify_file(struct file *file, __u32 mask)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 1ce66748a2d2..a2db821e8a8f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -248,6 +248,7 @@ enum fsnotify_data_type {
 	FSNOTIFY_EVENT_NONE,
 	FSNOTIFY_EVENT_PATH,
 	FSNOTIFY_EVENT_INODE,
+	FSNOTIFY_EVENT_DENTRY,
 };
 
 static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
@@ -255,6 +256,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
 	switch (data_type) {
 	case FSNOTIFY_EVENT_INODE:
 		return (struct inode *)data;
+	case FSNOTIFY_EVENT_DENTRY:
+		return d_inode(data);
 	case FSNOTIFY_EVENT_PATH:
 		return d_inode(((const struct path *)data)->dentry);
 	default:
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
 	}
 }
 
+static inline struct dentry *fsnotify_data_dentry(const void *data, int data_type)
+{
+	switch (data_type) {
+	case FSNOTIFY_EVENT_DENTRY:
+		/* Non const is needed for dget() */
+		return (struct dentry *)data;
+	case FSNOTIFY_EVENT_PATH:
+		return ((const struct path *)data)->dentry;
+	default:
+		return NULL;
+	}
+}
+
 static inline const struct path *fsnotify_data_path(const void *data,
 						    int data_type)
 {
-- 
2.33.0

Re: [PATCH v7 02/28] fsnotify: pass dentry instead of inode data

[Jan Kara]

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 <krisman@collabora.com>

Did this ever get used in this series?

								Honza

> ---
>  include/linux/fsnotify.h         |  5 ++---
>  include/linux/fsnotify_backend.h | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index d1144d7c3536..df0fa4687a18 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -39,8 +39,7 @@ static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
>  static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
>  				   __u32 mask)
>  {
> -	fsnotify_name(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> -		      dir, &dentry->d_name, 0);
> +	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name, 0);
>  }
>  
>  static inline void fsnotify_inode(struct inode *inode, __u32 mask)
> @@ -87,7 +86,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>   */
>  static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
>  {
> -	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE);
> +	fsnotify_parent(dentry, mask, dentry, FSNOTIFY_EVENT_DENTRY);
>  }
>  
>  static inline int fsnotify_file(struct file *file, __u32 mask)
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 1ce66748a2d2..a2db821e8a8f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -248,6 +248,7 @@ enum fsnotify_data_type {
>  	FSNOTIFY_EVENT_NONE,
>  	FSNOTIFY_EVENT_PATH,
>  	FSNOTIFY_EVENT_INODE,
> +	FSNOTIFY_EVENT_DENTRY,
>  };
>  
>  static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
> @@ -255,6 +256,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
>  	switch (data_type) {
>  	case FSNOTIFY_EVENT_INODE:
>  		return (struct inode *)data;
> +	case FSNOTIFY_EVENT_DENTRY:
> +		return d_inode(data);
>  	case FSNOTIFY_EVENT_PATH:
>  		return d_inode(((const struct path *)data)->dentry);
>  	default:
> @@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
>  	}
>  }
>  
> +static inline struct dentry *fsnotify_data_dentry(const void *data, int data_type)
> +{
> +	switch (data_type) {
> +	case FSNOTIFY_EVENT_DENTRY:
> +		/* Non const is needed for dget() */
> +		return (struct dentry *)data;
> +	case FSNOTIFY_EVENT_PATH:
> +		return ((const struct path *)data)->dentry;
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  static inline const struct path *fsnotify_data_path(const void *data,
>  						    int data_type)
>  {
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v7 02/28] fsnotify: pass dentry instead of inode data

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/fsnotify.h         |  5 ++---
>  include/linux/fsnotify_backend.h | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index d1144d7c3536..df0fa4687a18 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -39,8 +39,7 @@ static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
>  static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
>  				   __u32 mask)
>  {
> -	fsnotify_name(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> -		      dir, &dentry->d_name, 0);
> +	fsnotify_name(mask, dentry, FSNOTIFY_EVENT_DENTRY, dir, &dentry->d_name, 0);
>  }
>  
>  static inline void fsnotify_inode(struct inode *inode, __u32 mask)
> @@ -87,7 +86,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>   */
>  static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
>  {
> -	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE);
> +	fsnotify_parent(dentry, mask, dentry, FSNOTIFY_EVENT_DENTRY);
>  }
>  
>  static inline int fsnotify_file(struct file *file, __u32 mask)
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 1ce66748a2d2..a2db821e8a8f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -248,6 +248,7 @@ enum fsnotify_data_type {
>  	FSNOTIFY_EVENT_NONE,
>  	FSNOTIFY_EVENT_PATH,
>  	FSNOTIFY_EVENT_INODE,
> +	FSNOTIFY_EVENT_DENTRY,
>  };
>  
>  static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
> @@ -255,6 +256,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
>  	switch (data_type) {
>  	case FSNOTIFY_EVENT_INODE:
>  		return (struct inode *)data;
> +	case FSNOTIFY_EVENT_DENTRY:
> +		return d_inode(data);
>  	case FSNOTIFY_EVENT_PATH:
>  		return d_inode(((const struct path *)data)->dentry);
>  	default:
> @@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
>  	}
>  }
>  
> +static inline struct dentry *fsnotify_data_dentry(const void *data, int data_type)
> +{
> +	switch (data_type) {
> +	case FSNOTIFY_EVENT_DENTRY:
> +		/* Non const is needed for dget() */
> +		return (struct dentry *)data;
> +	case FSNOTIFY_EVENT_PATH:
> +		return ((const struct path *)data)->dentry;
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  static inline const struct path *fsnotify_data_path(const void *data,
>  						    int data_type)
>  {
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 03/28] fsnotify: clarify contract for create event hooks

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
---
 include/linux/fsnotify.h | 22 ++++++++++++++++------
 kernel/audit_fsnotify.c  |  3 +--
 kernel/audit_watch.c     |  3 +--
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index df0fa4687a18..1e5f7435a4b5 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -192,16 +192,22 @@ static inline void fsnotify_inoderemove(struct inode *inode)
 
 /*
  * fsnotify_create - 'name' was linked in
+ *
+ * Caller must make sure that dentry->d_name is stable.
+ * Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate
+ * ->d_inode later
  */
-static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
+static inline void fsnotify_create(struct inode *dir, struct dentry *dentry)
 {
-	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
+	audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	fsnotify_dirent(inode, dentry, FS_CREATE);
+	fsnotify_dirent(dir, dentry, FS_CREATE);
 }
 
 /*
  * fsnotify_link - new hardlink in 'inode' directory
+ *
+ * Caller must make sure that new_dentry->d_name is stable.
  * Note: We have to pass also the linked inode ptr as some filesystems leave
  *   new_dentry->d_inode NULL and instantiate inode pointer later
  */
@@ -230,12 +236,16 @@ static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)
 
 /*
  * fsnotify_mkdir - directory 'name' was created
+ *
+ * Caller must make sure that dentry->d_name is stable.
+ * Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate
+ * ->d_inode later
  */
-static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
+static inline void fsnotify_mkdir(struct inode *dir, struct dentry *dentry)
 {
-	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
+	audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
+	fsnotify_dirent(dir, dentry, FS_CREATE | FS_ISDIR);
 }
 
 /*
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 60739d5e3373..02348b48447c 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -160,8 +160,7 @@ static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 
 	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
 
-	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group) ||
-	    WARN_ON_ONCE(!inode))
+	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group))
 		return 0;
 
 	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 2acf7ca49154..223eed7b39cd 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -472,8 +472,7 @@ static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 
 	parent = container_of(inode_mark, struct audit_parent, mark);
 
-	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group) ||
-	    WARN_ON_ONCE(!inode))
+	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group))
 		return 0;
 
 	if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
-- 
2.33.0

Re: [PATCH v7 03/28] fsnotify: clarify contract for create event hooks

[Jan Kara]

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 <krisman@collabora.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  include/linux/fsnotify.h | 22 ++++++++++++++++------
>  kernel/audit_fsnotify.c  |  3 +--
>  kernel/audit_watch.c     |  3 +--
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index df0fa4687a18..1e5f7435a4b5 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -192,16 +192,22 @@ static inline void fsnotify_inoderemove(struct inode *inode)
>  
>  /*
>   * fsnotify_create - 'name' was linked in
> + *
> + * Caller must make sure that dentry->d_name is stable.
> + * Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate
> + * ->d_inode later
>   */
> -static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
> +static inline void fsnotify_create(struct inode *dir, struct dentry *dentry)
>  {
> -	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> +	audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify_dirent(inode, dentry, FS_CREATE);
> +	fsnotify_dirent(dir, dentry, FS_CREATE);
>  }
>  
>  /*
>   * fsnotify_link - new hardlink in 'inode' directory
> + *
> + * Caller must make sure that new_dentry->d_name is stable.
>   * Note: We have to pass also the linked inode ptr as some filesystems leave
>   *   new_dentry->d_inode NULL and instantiate inode pointer later
>   */
> @@ -230,12 +236,16 @@ static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)
>  
>  /*
>   * fsnotify_mkdir - directory 'name' was created
> + *
> + * Caller must make sure that dentry->d_name is stable.
> + * Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate
> + * ->d_inode later
>   */
> -static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
> +static inline void fsnotify_mkdir(struct inode *dir, struct dentry *dentry)
>  {
> -	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> +	audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
> +	fsnotify_dirent(dir, dentry, FS_CREATE | FS_ISDIR);
>  }
>  
>  /*
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 60739d5e3373..02348b48447c 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -160,8 +160,7 @@ static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>  
>  	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
>  
> -	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group) ||
> -	    WARN_ON_ONCE(!inode))
> +	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group))
>  		return 0;
>  
>  	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 2acf7ca49154..223eed7b39cd 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -472,8 +472,7 @@ static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>  
>  	parent = container_of(inode_mark, struct audit_parent, mark);
>  
> -	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group) ||
> -	    WARN_ON_ONCE(!inode))
> +	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group))
>  		return 0;
>  
>  	if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 04/28] fsnotify: Don't insert unmergeable events in hashtable

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 057abd2cf887..310246f8d3f1 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -702,6 +702,9 @@ static void fanotify_insert_event(struct fsnotify_group *group,
 
 	assert_spin_locked(&group->notification_lock);
 
+	if (!fanotify_is_hashed_event(event->mask))
+		return;
+
 	pr_debug("%s: group=%p event=%p bucket=%u\n", __func__,
 		 group, event, bucket);
 
@@ -779,8 +782,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 
 	fsn_event = &event->fse;
 	ret = fsnotify_add_event(group, fsn_event, fanotify_merge,
-				 fanotify_is_hashed_event(mask) ?
-				 fanotify_insert_event : NULL);
+				 fanotify_insert_event);
 	if (ret) {
 		/* Permission events shouldn't be merged */
 		BUG_ON(ret == 1 && mask & FANOTIFY_PERM_EVENTS);
-- 
2.33.0

[PATCH v7 05/28] fanotify: Fold event size calculation to its own function

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6facdf476255..6895ec310b5d 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -126,17 +126,24 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
 		       FANOTIFY_EVENT_ALIGN);
 }
 
-static int fanotify_event_info_len(unsigned int info_mode,
-				   struct fanotify_event *event)
+static size_t fanotify_event_len(unsigned int info_mode,
+				 struct fanotify_event *event)
 {
-	struct fanotify_info *info = fanotify_event_info(event);
-	int dir_fh_len = fanotify_event_dir_fh_len(event);
-	int fh_len = fanotify_event_object_fh_len(event);
-	int info_len = 0;
+	size_t event_len = FAN_EVENT_METADATA_LEN;
+	struct fanotify_info *info;
+	int dir_fh_len;
+	int fh_len;
 	int dot_len = 0;
 
+	if (!info_mode)
+		return event_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);
 	} else if ((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);
 
-	return info_len;
+	return event_len;
 }
 
 /*
@@ -181,7 +188,7 @@ static void fanotify_unhash_event(struct fsnotify_group *group,
 static struct fanotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
 {
-	size_t event_size = FAN_EVENT_METADATA_LEN;
+	size_t event_size;
 	struct fanotify_event *event = NULL;
 	struct fsnotify_event *fsn_event;
 	unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
@@ -194,8 +201,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
 		goto out;
 
 	event = FANOTIFY_E(fsn_event);
-	if (info_mode)
-		event_size += fanotify_event_info_len(info_mode, event);
+	event_size = fanotify_event_len(info_mode, event);
 
 	if (event_size > count) {
 		event = ERR_PTR(-EINVAL);
@@ -537,8 +543,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	metadata.event_len = FAN_EVENT_METADATA_LEN +
-				fanotify_event_info_len(info_mode, event);
+	metadata.event_len = fanotify_event_len(info_mode, event);
 	metadata.metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
-- 
2.33.0

[PATCH v7 06/28] fanotify: Split fsid check from other fid mode checks

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6895ec310b5d..adeae6d65e35 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1300,16 +1300,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	return fd;
 }
 
-/* Check if filesystem can encode a unique fid */
-static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
+static int fanotify_test_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
 {
 	__kernel_fsid_t root_fsid;
 	int err;
 
 	/*
-	 * Make sure path is not in filesystem with zero fsid (e.g. tmpfs).
+	 * Make sure dentry is not of a filesystem with zero fsid (e.g. fuse).
 	 */
-	err = vfs_get_fsid(path->dentry, fsid);
+	err = vfs_get_fsid(dentry, fsid);
 	if (err)
 		return err;
 
@@ -1317,10 +1316,10 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
 		return -ENODEV;
 
 	/*
-	 * Make sure path is not inside a filesystem subvolume (e.g. btrfs)
+	 * Make sure dentry is not of a filesystem subvolume (e.g. btrfs)
 	 * which uses a different fsid than sb root.
 	 */
-	err = vfs_get_fsid(path->dentry->d_sb->s_root, &root_fsid);
+	err = vfs_get_fsid(dentry->d_sb->s_root, &root_fsid);
 	if (err)
 		return err;
 
@@ -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;
 
+	return 0;
+}
+
+/* Check if filesystem can encode a unique fid */
+static int fanotify_test_fid(struct dentry *dentry)
+{
 	/*
 	 * We need to make sure that the file system supports at least
 	 * encoding a file handle so user can use name_to_handle_at() to
@@ -1335,8 +1340,8 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
 	 * objects. However, name_to_handle_at() requires that the
 	 * filesystem also supports decoding file handles.
 	 */
-	if (!path->dentry->d_sb->s_export_op ||
-	    !path->dentry->d_sb->s_export_op->fh_to_dentry)
+	if (!dentry->d_sb->s_export_op ||
+	    !dentry->d_sb->s_export_op->fh_to_dentry)
 		return -EOPNOTSUPP;
 
 	return 0;
@@ -1487,7 +1492,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	}
 
 	if (fid_mode) {
-		ret = fanotify_test_fid(&path, &__fsid);
+		ret = fanotify_test_fsid(path.dentry, &__fsid);
+		if (ret)
+			goto path_put_and_out;
+
+		ret = fanotify_test_fid(path.dentry);
 		if (ret)
 			goto path_put_and_out;
 
-- 
2.33.0

[PATCH v7 07/28] inotify: Don't force FS_IN_IGNORED

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/inotify/inotify_user.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 62051247f6d2..29fca3284bb5 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -94,10 +94,10 @@ static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)
 	__u32 mask;
 
 	/*
-	 * Everything should accept their own ignored and should receive events
-	 * when the inode is unmounted.  All directories care about children.
+	 * Everything should receive events when the inode is unmounted.
+	 * All directories care about children.
 	 */
-	mask = (FS_IN_IGNORED | FS_UNMOUNT);
+	mask = (FS_UNMOUNT);
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_EVENT_ON_CHILD;
 
-- 
2.33.0

[PATCH v7 08/28] fsnotify: Add helper to detect overflow_event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.h    | 3 ++-
 include/linux/fsnotify_backend.h | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 4a5e555dc3d2..c42cf8fd7d79 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -315,7 +315,8 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event)
  */
 static inline bool fanotify_is_hashed_event(u32 mask)
 {
-	return !fanotify_is_perm_event(mask) && !(mask & FS_Q_OVERFLOW);
+	return !(fanotify_is_perm_event(mask) ||
+		 fsnotify_is_overflow_event(mask));
 }
 
 static inline unsigned int fanotify_event_hash_bucket(
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a2db821e8a8f..749bc85e1d1c 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -510,6 +510,11 @@ static inline void fsnotify_queue_overflow(struct fsnotify_group *group)
 	fsnotify_add_event(group, group->overflow_event, NULL, NULL);
 }
 
+static inline bool fsnotify_is_overflow_event(u32 mask)
+{
+	return mask & FS_Q_OVERFLOW;
+}
+
 static inline bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group)
 {
 	assert_spin_locked(&group->notification_lock);
-- 
2.33.0

[PATCH v7 09/28] fsnotify: Add wrapper around fsnotify_add_event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 310246f8d3f1..f82e20228999 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -781,8 +781,8 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 	}
 
 	fsn_event = &event->fse;
-	ret = fsnotify_add_event(group, fsn_event, fanotify_merge,
-				 fanotify_insert_event);
+	ret = fsnotify_insert_event(group, fsn_event, fanotify_merge,
+				    fanotify_insert_event);
 	if (ret) {
 		/* Permission events shouldn't be merged */
 		BUG_ON(ret == 1 && mask & FANOTIFY_PERM_EVENTS);
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index d1a64daa0171..a96582cbfad1 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -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);
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 32f45543b9c6..44bb10f50715 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -78,12 +78,12 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
  * 2 if the event was not queued - either the queue of events has overflown
  *   or the group is shutting down.
  */
-int fsnotify_add_event(struct fsnotify_group *group,
-		       struct fsnotify_event *event,
-		       int (*merge)(struct fsnotify_group *,
-				    struct fsnotify_event *),
-		       void (*insert)(struct fsnotify_group *,
-				      struct fsnotify_event *))
+int fsnotify_insert_event(struct fsnotify_group *group,
+			  struct fsnotify_event *event,
+			  int (*merge)(struct fsnotify_group *,
+				       struct fsnotify_event *),
+			  void (*insert)(struct fsnotify_group *,
+					 struct fsnotify_event *))
 {
 	int ret = 0;
 	struct list_head *list = &group->notification_list;
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 749bc85e1d1c..b323d0c4b967 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -498,16 +498,25 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);
 extern void fsnotify_destroy_event(struct fsnotify_group *group,
 				   struct fsnotify_event *event);
 /* attach the event to the group notification queue */
-extern int fsnotify_add_event(struct fsnotify_group *group,
-			      struct fsnotify_event *event,
-			      int (*merge)(struct fsnotify_group *,
-					   struct fsnotify_event *),
-			      void (*insert)(struct fsnotify_group *,
-					     struct fsnotify_event *));
+extern int fsnotify_insert_event(struct fsnotify_group *group,
+				 struct fsnotify_event *event,
+				 int (*merge)(struct fsnotify_group *,
+					      struct fsnotify_event *),
+				 void (*insert)(struct fsnotify_group *,
+						struct fsnotify_event *));
+
+static inline int fsnotify_add_event(struct fsnotify_group *group,
+				     struct fsnotify_event *event,
+				     int (*merge)(struct fsnotify_group *,
+						  struct fsnotify_event *))
+{
+	return fsnotify_insert_event(group, event, merge, NULL);
+}
+
 /* Queue overflow event to a notification group */
 static inline void fsnotify_queue_overflow(struct fsnotify_group *group)
 {
-	fsnotify_add_event(group, group->overflow_event, NULL, NULL);
+	fsnotify_add_event(group, group->overflow_event, NULL);
 }
 
 static inline bool fsnotify_is_overflow_event(u32 mask)
-- 
2.33.0

[PATCH v7 10/28] fsnotify: Retrieve super block from the data field

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

--
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(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 963e6ce75b96..fde3a1115a17 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -455,16 +455,16 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
  *		@file_name is relative to
  * @file_name:	optional file name associated with event
  * @inode:	optional inode associated with event -
- *		either @dir or @inode must be non-NULL.
- *		if both are non-NULL event may be reported to both.
+ *		If @dir and @inode are both 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)
 {
 	const struct path *path = fsnotify_data_path(data, data_type);
+	struct super_block *sb = fsnotify_data_sb(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 +483,6 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		 */
 		parent = dir;
 	}
-	sb = inode->i_sb;
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b323d0c4b967..035438fe4a43 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -289,6 +289,21 @@ static inline const struct path *fsnotify_data_path(const void *data,
 	}
 }
 
+static inline struct super_block *fsnotify_data_sb(const void *data,
+						   int data_type)
+{
+	switch (data_type) {
+	case FSNOTIFY_EVENT_INODE:
+		return ((struct inode *)data)->i_sb;
+	case FSNOTIFY_EVENT_DENTRY:
+		return ((struct dentry *)data)->d_sb;
+	case FSNOTIFY_EVENT_PATH:
+		return ((const struct path *)data)->dentry->d_sb;
+	default:
+		return NULL;
+	}
+}
+
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_INODE,
 	FSNOTIFY_OBJ_TYPE_PARENT,
-- 
2.33.0

Re: [PATCH v7 10/28] fsnotify: Retrieve super block from the data field

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

>
> --
> 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(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 963e6ce75b96..fde3a1115a17 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -455,16 +455,16 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
>   *             @file_name is relative to
>   * @file_name: optional file name associated with event
>   * @inode:     optional inode associated with event -
> - *             either @dir or @inode must be non-NULL.
> - *             if both are non-NULL event may be reported to both.
> + *             If @dir and @inode are both 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)
>  {
>         const struct path *path = fsnotify_data_path(data, data_type);
> +       struct super_block *sb = fsnotify_data_sb(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 +483,6 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>                  */
>                 parent = dir;
>         }
> -       sb = inode->i_sb;
>
>         /*
>          * Optimization: srcu_read_lock() has a memory barrier which can
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b323d0c4b967..035438fe4a43 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -289,6 +289,21 @@ static inline const struct path *fsnotify_data_path(const void *data,
>         }
>  }
>
> +static inline struct super_block *fsnotify_data_sb(const void *data,
> +                                                  int data_type)
> +{
> +       switch (data_type) {
> +       case FSNOTIFY_EVENT_INODE:
> +               return ((struct inode *)data)->i_sb;
> +       case FSNOTIFY_EVENT_DENTRY:
> +               return ((struct dentry *)data)->d_sb;
> +       case FSNOTIFY_EVENT_PATH:
> +               return ((const struct path *)data)->dentry->d_sb;
> +       default:
> +               return NULL;
> +       }
> +}
> +
>  enum fsnotify_obj_type {
>         FSNOTIFY_OBJ_TYPE_INODE,
>         FSNOTIFY_OBJ_TYPE_PARENT,
> --
> 2.33.0
>

Re: [PATCH v7 10/28] fsnotify: Retrieve super block from the data field

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> --
> 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(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 963e6ce75b96..fde3a1115a17 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -455,16 +455,16 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
>   *		@file_name is relative to
>   * @file_name:	optional file name associated with event
>   * @inode:	optional inode associated with event -
> - *		either @dir or @inode must be non-NULL.
> - *		if both are non-NULL event may be reported to both.
> + *		If @dir and @inode are both 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)
>  {
>  	const struct path *path = fsnotify_data_path(data, data_type);
> +	struct super_block *sb = fsnotify_data_sb(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 +483,6 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  		 */
>  		parent = dir;
>  	}
> -	sb = inode->i_sb;
>  
>  	/*
>  	 * Optimization: srcu_read_lock() has a memory barrier which can
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b323d0c4b967..035438fe4a43 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -289,6 +289,21 @@ static inline const struct path *fsnotify_data_path(const void *data,
>  	}
>  }
>  
> +static inline struct super_block *fsnotify_data_sb(const void *data,
> +						   int data_type)
> +{
> +	switch (data_type) {
> +	case FSNOTIFY_EVENT_INODE:
> +		return ((struct inode *)data)->i_sb;
> +	case FSNOTIFY_EVENT_DENTRY:
> +		return ((struct dentry *)data)->d_sb;
> +	case FSNOTIFY_EVENT_PATH:
> +		return ((const struct path *)data)->dentry->d_sb;
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  enum fsnotify_obj_type {
>  	FSNOTIFY_OBJ_TYPE_INODE,
>  	FSNOTIFY_OBJ_TYPE_PARENT,
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 11/28] fsnotify: Pass group argument to free_event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index f82e20228999..c620b4f6fe12 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -835,7 +835,8 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 	kfree(FANOTIFY_NE(event));
 }
 
-static void fanotify_free_event(struct fsnotify_event *fsn_event)
+static void fanotify_free_event(struct fsnotify_group *group,
+				struct fsnotify_event *fsn_event)
 {
 	struct fanotify_event *event;
 
diff --git a/fs/notify/group.c b/fs/notify/group.c
index fb89c351295d..6a297efc4788 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -88,7 +88,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
 	 * that deliberately ignores overflow events.
 	 */
 	if (group->overflow_event)
-		group->ops->free_event(group->overflow_event);
+		group->ops->free_event(group, group->overflow_event);
 
 	fsnotify_put_group(group);
 }
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index a96582cbfad1..d92d7b0adc9a 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -177,7 +177,8 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
 		dec_inotify_instances(group->inotify_data.ucounts);
 }
 
-static void inotify_free_event(struct fsnotify_event *fsn_event)
+static void inotify_free_event(struct fsnotify_group *group,
+			       struct fsnotify_event *fsn_event)
 {
 	kfree(INOTIFY_E(fsn_event));
 }
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 44bb10f50715..9022ae650cf8 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -64,7 +64,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
 		WARN_ON(!list_empty(&event->list));
 		spin_unlock(&group->notification_lock);
 	}
-	group->ops->free_event(event);
+	group->ops->free_event(group, event);
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 035438fe4a43..1e69e9fe45c9 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -155,7 +155,7 @@ struct fsnotify_ops {
 			    const struct qstr *file_name, u32 cookie);
 	void (*free_group_priv)(struct fsnotify_group *group);
 	void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
-	void (*free_event)(struct fsnotify_event *event);
+	void (*free_event)(struct fsnotify_group *group, struct fsnotify_event *event);
 	/* called on final put+free to free memory */
 	void (*free_mark)(struct fsnotify_mark *mark);
 };
-- 
2.33.0

Re: [PATCH v7 11/28] fsnotify: Pass group argument to free_event

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index f82e20228999..c620b4f6fe12 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -835,7 +835,8 @@ static void fanotify_free_name_event(struct fanotify_event *event)
>         kfree(FANOTIFY_NE(event));
>  }
>
> -static void fanotify_free_event(struct fsnotify_event *fsn_event)
> +static void fanotify_free_event(struct fsnotify_group *group,
> +                               struct fsnotify_event *fsn_event)
>  {
>         struct fanotify_event *event;
>
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index fb89c351295d..6a297efc4788 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -88,7 +88,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
>          * that deliberately ignores overflow events.
>          */
>         if (group->overflow_event)
> -               group->ops->free_event(group->overflow_event);
> +               group->ops->free_event(group, group->overflow_event);
>
>         fsnotify_put_group(group);
>  }
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index a96582cbfad1..d92d7b0adc9a 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -177,7 +177,8 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>                 dec_inotify_instances(group->inotify_data.ucounts);
>  }
>
> -static void inotify_free_event(struct fsnotify_event *fsn_event)
> +static void inotify_free_event(struct fsnotify_group *group,
> +                              struct fsnotify_event *fsn_event)
>  {
>         kfree(INOTIFY_E(fsn_event));
>  }
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 44bb10f50715..9022ae650cf8 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -64,7 +64,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
>                 WARN_ON(!list_empty(&event->list));
>                 spin_unlock(&group->notification_lock);
>         }
> -       group->ops->free_event(event);
> +       group->ops->free_event(group, event);
>  }
>
>  /*
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 035438fe4a43..1e69e9fe45c9 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -155,7 +155,7 @@ struct fsnotify_ops {
>                             const struct qstr *file_name, u32 cookie);
>         void (*free_group_priv)(struct fsnotify_group *group);
>         void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
> -       void (*free_event)(struct fsnotify_event *event);
> +       void (*free_event)(struct fsnotify_group *group, struct fsnotify_event *event);
>         /* called on final put+free to free memory */
>         void (*free_mark)(struct fsnotify_mark *mark);
>  };
> --
> 2.33.0
>

Re: [PATCH v7 11/28] fsnotify: Pass group argument to free_event

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  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(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index f82e20228999..c620b4f6fe12 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -835,7 +835,8 @@ static void fanotify_free_name_event(struct fanotify_event *event)
>  	kfree(FANOTIFY_NE(event));
>  }
>  
> -static void fanotify_free_event(struct fsnotify_event *fsn_event)
> +static void fanotify_free_event(struct fsnotify_group *group,
> +				struct fsnotify_event *fsn_event)
>  {
>  	struct fanotify_event *event;
>  
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index fb89c351295d..6a297efc4788 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -88,7 +88,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
>  	 * that deliberately ignores overflow events.
>  	 */
>  	if (group->overflow_event)
> -		group->ops->free_event(group->overflow_event);
> +		group->ops->free_event(group, group->overflow_event);
>  
>  	fsnotify_put_group(group);
>  }
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index a96582cbfad1..d92d7b0adc9a 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -177,7 +177,8 @@ static void inotify_free_group_priv(struct fsnotify_group *group)
>  		dec_inotify_instances(group->inotify_data.ucounts);
>  }
>  
> -static void inotify_free_event(struct fsnotify_event *fsn_event)
> +static void inotify_free_event(struct fsnotify_group *group,
> +			       struct fsnotify_event *fsn_event)
>  {
>  	kfree(INOTIFY_E(fsn_event));
>  }
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 44bb10f50715..9022ae650cf8 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -64,7 +64,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
>  		WARN_ON(!list_empty(&event->list));
>  		spin_unlock(&group->notification_lock);
>  	}
> -	group->ops->free_event(event);
> +	group->ops->free_event(group, event);
>  }
>  
>  /*
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 035438fe4a43..1e69e9fe45c9 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -155,7 +155,7 @@ struct fsnotify_ops {
>  			    const struct qstr *file_name, u32 cookie);
>  	void (*free_group_priv)(struct fsnotify_group *group);
>  	void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
> -	void (*free_event)(struct fsnotify_event *event);
> +	void (*free_event)(struct fsnotify_group *group, struct fsnotify_event *event);
>  	/* called on final put+free to free memory */
>  	void (*free_mark)(struct fsnotify_mark *mark);
>  };
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 12/28] fanotify: Support null inode event in fanotify_dfid_inode

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index c620b4f6fe12..397ee623ff1e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -452,7 +452,7 @@ static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data,
 	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
 		return dir;
 
-	if (S_ISDIR(inode->i_mode))
+	if (inode && S_ISDIR(inode->i_mode))
 		return inode;
 
 	return dir;
-- 
2.33.0

Re: [PATCH v7 12/28] fanotify: Support null inode event in fanotify_dfid_inode

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>


Please also fortify fsnotify_handle_inode_event() against calling
->handle_inode_event() with NULL inode.

> ---
>  fs/notify/fanotify/fanotify.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index c620b4f6fe12..397ee623ff1e 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -452,7 +452,7 @@ static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data,
>         if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
>                 return dir;
>
> -       if (S_ISDIR(inode->i_mode))
> +       if (inode && S_ISDIR(inode->i_mode))
>                 return inode;
>
>         return dir;
> --
> 2.33.0
>

Re: [PATCH v7 12/28] fanotify: Support null inode event in fanotify_dfid_inode

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/notify/fanotify/fanotify.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index c620b4f6fe12..397ee623ff1e 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -452,7 +452,7 @@ static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data,
>  	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
>  		return dir;
>  
> -	if (S_ISDIR(inode->i_mode))
> +	if (inode && S_ISDIR(inode->i_mode))
>  		return inode;
>  
>  	return dir;
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 13/28] fanotify: Allow file handle encoding for unhashed events

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 397ee623ff1e..ec84fee7ad01 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -403,8 +403,12 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	fh->type = type;
 	fh->len = fh_len;
 
-	/* Mix fh into event merge key */
-	*hash ^= fanotify_hash_fh(fh);
+	/*
+	 * Mix fh into event merge key.  Hash might be NULL in case of
+	 * unhashed FID events (i.e. FAN_FS_ERROR).
+	 */
+	if (hash)
+		*hash ^= fanotify_hash_fh(fh);
 
 	return FANOTIFY_FH_HDR_LEN + fh_len;
 
-- 
2.33.0

[PATCH v7 14/28] fanotify: Encode empty file handle when no inode is provided

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index ec84fee7ad01..c64d61b673ca 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -370,8 +370,14 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	fh->type = FILEID_ROOT;
 	fh->len = 0;
 	fh->flags = 0;
+
+	/*
+	 * Invalid FHs are used by FAN_FS_ERROR for errors not
+	 * linked to any inode. The f_handle won't be reported
+	 * back to userspace.
+	 */
 	if (!inode)
-		return 0;
+		goto out;
 
 	/*
 	 * !gpf means preallocated variable size fh, but fh_len could
@@ -403,6 +409,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	fh->type = type;
 	fh->len = fh_len;
 
+out:
 	/*
 	 * Mix fh into event merge key.  Hash might be NULL in case of
 	 * unhashed FID events (i.e. FAN_FS_ERROR).
-- 
2.33.0

Re: [PATCH v7 14/28] fanotify: Encode empty file handle when no inode is provided

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:38 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

>
> ---
> 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(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index ec84fee7ad01..c64d61b673ca 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -370,8 +370,14 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>         fh->type = FILEID_ROOT;
>         fh->len = 0;
>         fh->flags = 0;
> +
> +       /*
> +        * Invalid FHs are used by FAN_FS_ERROR for errors not
> +        * linked to any inode. The f_handle won't be reported
> +        * back to userspace.
> +        */
>         if (!inode)
> -               return 0;
> +               goto out;
>
>         /*
>          * !gpf means preallocated variable size fh, but fh_len could
> @@ -403,6 +409,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>         fh->type = type;
>         fh->len = fh_len;
>
> +out:
>         /*
>          * Mix fh into event merge key.  Hash might be NULL in case of
>          * unhashed FID events (i.e. FAN_FS_ERROR).
> --
> 2.33.0
>

Re: [PATCH v7 14/28] fanotify: Encode empty file handle when no inode is provided

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
 
> ---
> 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(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index ec84fee7ad01..c64d61b673ca 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -370,8 +370,14 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>  	fh->type = FILEID_ROOT;
>  	fh->len = 0;
>  	fh->flags = 0;
> +
> +	/*
> +	 * Invalid FHs are used by FAN_FS_ERROR for errors not
> +	 * linked to any inode. The f_handle won't be reported
> +	 * back to userspace.
> +	 */
>  	if (!inode)
> -		return 0;
> +		goto out;
>  
>  	/*
>  	 * !gpf means preallocated variable size fh, but fh_len could
> @@ -403,6 +409,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>  	fh->type = type;
>  	fh->len = fh_len;
>  
> +out:
>  	/*
>  	 * Mix fh into event merge key.  Hash might be NULL in case of
>  	 * unhashed FID events (i.e. FAN_FS_ERROR).
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 15/28] fanotify: Require fid_mode for any non-fd event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index adeae6d65e35..66ee3c2805c7 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1458,14 +1458,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		goto fput_and_out;
 
 	/*
-	 * 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.
 	 */
 	fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
-	if (mask & FANOTIFY_INODE_EVENTS &&
+	if (mask & ~(FANOTIFY_FD_EVENTS|FANOTIFY_EVENT_FLAGS) &&
 	    (!fid_mode || mark_type == FAN_MARK_MOUNT))
 		goto fput_and_out;
 
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index eec3b7c40811..52d464802d99 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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)
-- 
2.33.0

[PATCH v7 16/28] fsnotify: Support FS_ERROR event type

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 1e5f7435a4b5..787545e87eeb 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -339,4 +339,17 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
 		fsnotify_dentry(dentry, mask);
 }
 
+static inline int fsnotify_sb_error(struct super_block *sb, struct inode *inode,
+				    int error)
+{
+	struct fs_error_report report = {
+		.error = error,
+		.inode = inode,
+		.sb = sb,
+	};
+
+	return fsnotify(FS_ERROR, &report, FSNOTIFY_EVENT_ERROR,
+			NULL, NULL, NULL, 0);
+}
+
 #endif	/* _LINUX_FS_NOTIFY_H */
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 1e69e9fe45c9..a378a314e309 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -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_IGNORED overloads FS_ERROR.  It is only used internally by inotify
+ * which does not support FS_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 | \
@@ -249,6 +256,13 @@ enum fsnotify_data_type {
 	FSNOTIFY_EVENT_PATH,
 	FSNOTIFY_EVENT_INODE,
 	FSNOTIFY_EVENT_DENTRY,
+	FSNOTIFY_EVENT_ERROR,
+};
+
+struct fs_error_report {
+	int error;
+	struct inode *inode;
+	struct super_block *sb;
 };
 
 static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
@@ -260,6 +274,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)
 		return d_inode(data);
 	case FSNOTIFY_EVENT_PATH:
 		return d_inode(((const struct path *)data)->dentry);
+	case FSNOTIFY_EVENT_ERROR:
+		return ((struct fs_error_report *)data)->inode;
 	default:
 		return NULL;
 	}
@@ -299,6 +315,20 @@ static inline struct super_block *fsnotify_data_sb(const void *data,
 		return ((struct dentry *)data)->d_sb;
 	case FSNOTIFY_EVENT_PATH:
 		return ((const struct path *)data)->dentry->d_sb;
+	case FSNOTIFY_EVENT_ERROR:
+		return ((struct fs_error_report *) data)->sb;
+	default:
+		return NULL;
+	}
+}
+
+static inline struct fs_error_report *fsnotify_data_error_report(
+							const void *data,
+							int data_type)
+{
+	switch (data_type) {
+	case FSNOTIFY_EVENT_ERROR:
+		return (struct fs_error_report *) data;
 	default:
 		return NULL;
 	}
-- 
2.33.0

[PATCH v7 17/28] fanotify: Reserve UAPI bits for FAN_FS_ERROR

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.c | 1 +
 include/uapi/linux/fanotify.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index c64d61b673ca..8f152445d75c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -752,6 +752,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
 	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
 	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
+	BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
 
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 64553df9d735..2990731ddc8b 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -20,6 +20,7 @@
 #define FAN_OPEN_EXEC		0x00001000	/* File was opened for exec */
 
 #define FAN_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
+#define FAN_FS_ERROR		0x00008000	/* Filesystem error */
 
 #define FAN_OPEN_PERM		0x00010000	/* File open in perm check */
 #define FAN_ACCESS_PERM		0x00020000	/* File accessed in perm check */
-- 
2.33.0

Re: [PATCH v7 17/28] fanotify: Reserve UAPI bits for FAN_FS_ERROR

[Jan Kara]

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'

> 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 <krisman@collabora.com>
> ---
>  fs/notify/fanotify/fanotify.c | 1 +
>  include/uapi/linux/fanotify.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index c64d61b673ca..8f152445d75c 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -752,6 +752,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
>  	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
>  	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
>  	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
> +	BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
>  
>  	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
>  
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index 64553df9d735..2990731ddc8b 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -20,6 +20,7 @@
>  #define FAN_OPEN_EXEC		0x00001000	/* File was opened for exec */
>  
>  #define FAN_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
> +#define FAN_FS_ERROR		0x00008000	/* Filesystem error */
>  
>  #define FAN_OPEN_PERM		0x00010000	/* File open in perm check */
>  #define FAN_ACCESS_PERM		0x00020000	/* File accessed in perm check */
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 18/28] fanotify: Pre-allocate pool of error events

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 8f152445d75c..01d68dfc74aa 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group)
 	if (group->fanotify_data.ucounts)
 		dec_ucount(group->fanotify_data.ucounts,
 			   UCOUNT_FANOTIFY_GROUPS);
+
+	if (mempool_initialized(&group->fanotify_data.error_events_pool))
+		mempool_exit(&group->fanotify_data.error_events_pool);
 }
 
 static void fanotify_free_path_event(struct fanotify_event *event)
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index c42cf8fd7d79..a577e87fac2b 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -141,6 +141,7 @@ enum fanotify_event_type {
 	FANOTIFY_EVENT_TYPE_PATH,
 	FANOTIFY_EVENT_TYPE_PATH_PERM,
 	FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
+	FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */
 	__FANOTIFY_EVENT_TYPE_NUM
 };
 
@@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event)
 	return container_of(event, struct fanotify_name_event, fae);
 }
 
+struct fanotify_error_event {
+	struct fanotify_event fae;
+};
+
+static inline struct fanotify_error_event *
+FANOTIFY_EE(struct fanotify_event *event)
+{
+	return container_of(event, struct fanotify_error_event, fae);
+}
+
 static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
 {
 	if (event->type == FANOTIFY_EVENT_TYPE_FID)
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 66ee3c2805c7..f1cf863d6f9f 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -30,6 +30,7 @@
 #define FANOTIFY_DEFAULT_MAX_EVENTS	16384
 #define FANOTIFY_OLD_DEFAULT_MAX_MARKS	8192
 #define FANOTIFY_DEFAULT_MAX_GROUPS	128
+#define FANOTIFY_DEFAULT_FEE_POOL	32
 
 /*
  * Legacy fanotify marks limits (8192) is per group and we introduced a tunable
@@ -1054,6 +1055,15 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 	return ERR_PTR(ret);
 }
 
+static int fanotify_group_init_error_pool(struct fsnotify_group *group)
+{
+	if (mempool_initialized(&group->fanotify_data.error_events_pool))
+		return 0;
+
+	return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool,
+					 FANOTIFY_DEFAULT_FEE_POOL,
+					 sizeof(struct fanotify_error_event));
+}
 
 static int fanotify_add_mark(struct fsnotify_group *group,
 			     fsnotify_connp_t *connp, unsigned int type,
@@ -1062,6 +1072,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 {
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
+	int ret = 0;
 
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(connp, group);
@@ -1072,13 +1083,26 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			return PTR_ERR(fsn_mark);
 		}
 	}
+
+	/*
+	 * Error events are pre-allocated per group, only if strictly
+	 * needed (i.e. FAN_FS_ERROR was requested).
+	 */
+	if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) {
+		ret = fanotify_group_init_error_pool(group);
+		if (ret)
+			goto out;
+	}
+
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
 	if (added & ~fsnotify_conn_mask(fsn_mark->connector))
 		fsnotify_recalc_mask(fsn_mark->connector);
+
+out:
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
-	return 0;
+	return ret;
 }
 
 static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a378a314e309..9941c06b8c8a 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/user_namespace.h>
 #include <linux/refcount.h>
+#include <linux/mempool.h>
 
 /*
  * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
@@ -245,6 +246,7 @@ struct fsnotify_group {
 			int flags;           /* flags from fanotify_init() */
 			int f_flags; /* event_f_flags from fanotify_init() */
 			struct ucounts *ucounts;
+			mempool_t error_events_pool;
 		} fanotify_data;
 #endif /* CONFIG_FANOTIFY */
 	};
-- 
2.33.0

Re: [PATCH v7 18/28] fanotify: Pre-allocate pool of error events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> 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 <krisman@collabora.com>
> ---
>  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(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 8f152445d75c..01d68dfc74aa 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group)
>         if (group->fanotify_data.ucounts)
>                 dec_ucount(group->fanotify_data.ucounts,
>                            UCOUNT_FANOTIFY_GROUPS);
> +
> +       if (mempool_initialized(&group->fanotify_data.error_events_pool))
> +               mempool_exit(&group->fanotify_data.error_events_pool);
>  }
>
>  static void fanotify_free_path_event(struct fanotify_event *event)
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index c42cf8fd7d79..a577e87fac2b 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -141,6 +141,7 @@ enum fanotify_event_type {
>         FANOTIFY_EVENT_TYPE_PATH,
>         FANOTIFY_EVENT_TYPE_PATH_PERM,
>         FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
> +       FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */
>         __FANOTIFY_EVENT_TYPE_NUM
>  };
>
> @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event)
>         return container_of(event, struct fanotify_name_event, fae);
>  }
>
> +struct fanotify_error_event {
> +       struct fanotify_event fae;
> +};
> +
> +static inline struct fanotify_error_event *
> +FANOTIFY_EE(struct fanotify_event *event)
> +{
> +       return container_of(event, struct fanotify_error_event, fae);
> +}
> +
>  static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
>  {
>         if (event->type == FANOTIFY_EVENT_TYPE_FID)
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 66ee3c2805c7..f1cf863d6f9f 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -30,6 +30,7 @@
>  #define FANOTIFY_DEFAULT_MAX_EVENTS    16384
>  #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192
>  #define FANOTIFY_DEFAULT_MAX_GROUPS    128
> +#define FANOTIFY_DEFAULT_FEE_POOL      32
>

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.

Re: [PATCH v7 18/28] fanotify: Pre-allocate pool of error events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 9:19 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> >
> > 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 <krisman@collabora.com>
> > ---
> >  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(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 8f152445d75c..01d68dfc74aa 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group)
> >         if (group->fanotify_data.ucounts)
> >                 dec_ucount(group->fanotify_data.ucounts,
> >                            UCOUNT_FANOTIFY_GROUPS);
> > +
> > +       if (mempool_initialized(&group->fanotify_data.error_events_pool))
> > +               mempool_exit(&group->fanotify_data.error_events_pool);
> >  }
> >
> >  static void fanotify_free_path_event(struct fanotify_event *event)
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index c42cf8fd7d79..a577e87fac2b 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -141,6 +141,7 @@ enum fanotify_event_type {
> >         FANOTIFY_EVENT_TYPE_PATH,
> >         FANOTIFY_EVENT_TYPE_PATH_PERM,
> >         FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
> > +       FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */
> >         __FANOTIFY_EVENT_TYPE_NUM
> >  };
> >
> > @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event)
> >         return container_of(event, struct fanotify_name_event, fae);
> >  }
> >
> > +struct fanotify_error_event {
> > +       struct fanotify_event fae;
> > +};
> > +
> > +static inline struct fanotify_error_event *
> > +FANOTIFY_EE(struct fanotify_event *event)
> > +{
> > +       return container_of(event, struct fanotify_error_event, fae);
> > +}
> > +
> >  static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
> >  {
> >         if (event->type == FANOTIFY_EVENT_TYPE_FID)
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 66ee3c2805c7..f1cf863d6f9f 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -30,6 +30,7 @@
> >  #define FANOTIFY_DEFAULT_MAX_EVENTS    16384
> >  #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192
> >  #define FANOTIFY_DEFAULT_MAX_GROUPS    128
> > +#define FANOTIFY_DEFAULT_FEE_POOL      32
> >
>
> 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.

Re: [PATCH v7 18/28] fanotify: Pre-allocate pool of error events

[Jan Kara]

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.

... "shared by all FS_ERROR marks", right?
 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

The code otherwise looks OK to me.

								Honza

> ---
>  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(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 8f152445d75c..01d68dfc74aa 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group)
>  	if (group->fanotify_data.ucounts)
>  		dec_ucount(group->fanotify_data.ucounts,
>  			   UCOUNT_FANOTIFY_GROUPS);
> +
> +	if (mempool_initialized(&group->fanotify_data.error_events_pool))
> +		mempool_exit(&group->fanotify_data.error_events_pool);
>  }
>  
>  static void fanotify_free_path_event(struct fanotify_event *event)
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index c42cf8fd7d79..a577e87fac2b 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -141,6 +141,7 @@ enum fanotify_event_type {
>  	FANOTIFY_EVENT_TYPE_PATH,
>  	FANOTIFY_EVENT_TYPE_PATH_PERM,
>  	FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
> +	FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */
>  	__FANOTIFY_EVENT_TYPE_NUM
>  };
>  
> @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event)
>  	return container_of(event, struct fanotify_name_event, fae);
>  }
>  
> +struct fanotify_error_event {
> +	struct fanotify_event fae;
> +};
> +
> +static inline struct fanotify_error_event *
> +FANOTIFY_EE(struct fanotify_event *event)
> +{
> +	return container_of(event, struct fanotify_error_event, fae);
> +}
> +
>  static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
>  {
>  	if (event->type == FANOTIFY_EVENT_TYPE_FID)
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 66ee3c2805c7..f1cf863d6f9f 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -30,6 +30,7 @@
>  #define FANOTIFY_DEFAULT_MAX_EVENTS	16384
>  #define FANOTIFY_OLD_DEFAULT_MAX_MARKS	8192
>  #define FANOTIFY_DEFAULT_MAX_GROUPS	128
> +#define FANOTIFY_DEFAULT_FEE_POOL	32
>  
>  /*
>   * Legacy fanotify marks limits (8192) is per group and we introduced a tunable
> @@ -1054,6 +1055,15 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
>  	return ERR_PTR(ret);
>  }
>  
> +static int fanotify_group_init_error_pool(struct fsnotify_group *group)
> +{
> +	if (mempool_initialized(&group->fanotify_data.error_events_pool))
> +		return 0;
> +
> +	return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool,
> +					 FANOTIFY_DEFAULT_FEE_POOL,
> +					 sizeof(struct fanotify_error_event));
> +}
>  
>  static int fanotify_add_mark(struct fsnotify_group *group,
>  			     fsnotify_connp_t *connp, unsigned int type,
> @@ -1062,6 +1072,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
>  {
>  	struct fsnotify_mark *fsn_mark;
>  	__u32 added;
> +	int ret = 0;
>  
>  	mutex_lock(&group->mark_mutex);
>  	fsn_mark = fsnotify_find_mark(connp, group);
> @@ -1072,13 +1083,26 @@ static int fanotify_add_mark(struct fsnotify_group *group,
>  			return PTR_ERR(fsn_mark);
>  		}
>  	}
> +
> +	/*
> +	 * Error events are pre-allocated per group, only if strictly
> +	 * needed (i.e. FAN_FS_ERROR was requested).
> +	 */
> +	if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) {
> +		ret = fanotify_group_init_error_pool(group);
> +		if (ret)
> +			goto out;
> +	}
> +
>  	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
>  	if (added & ~fsnotify_conn_mask(fsn_mark->connector))
>  		fsnotify_recalc_mask(fsn_mark->connector);
> +
> +out:
>  	mutex_unlock(&group->mark_mutex);
>  
>  	fsnotify_put_mark(fsn_mark);
> -	return 0;
> +	return ret;
>  }
>  
>  static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index a378a314e309..9941c06b8c8a 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -19,6 +19,7 @@
>  #include <linux/atomic.h>
>  #include <linux/user_namespace.h>
>  #include <linux/refcount.h>
> +#include <linux/mempool.h>
>  
>  /*
>   * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
> @@ -245,6 +246,7 @@ struct fsnotify_group {
>  			int flags;           /* flags from fanotify_init() */
>  			int f_flags; /* event_f_flags from fanotify_init() */
>  			struct ucounts *ucounts;
> +			mempool_t error_events_pool;
>  		} fanotify_data;
>  #endif /* CONFIG_FANOTIFY */
>  	};
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 19/28] fanotify: Limit number of marks with FAN_FS_ERROR per group

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
 include/linux/fsnotify_backend.h   |  1 +
 2 files changed, 11 insertions(+)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index f1cf863d6f9f..5324890500fc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
 
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 umask, &destroy_mark);
+
+	if (removed & FAN_FS_ERROR)
+		group->fanotify_data.error_event_marks--;
+
 	if (removed & fsnotify_conn_mask(fsn_mark->connector))
 		fsnotify_recalc_mask(fsn_mark->connector);
 	if (destroy_mark)
@@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 
 static int fanotify_group_init_error_pool(struct fsnotify_group *group)
 {
+	if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL)
+		return -ENOMEM;
+
 	if (mempool_initialized(&group->fanotify_data.error_events_pool))
 		return 0;
 
@@ -1098,6 +1105,9 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 	if (added & ~fsnotify_conn_mask(fsn_mark->connector))
 		fsnotify_recalc_mask(fsn_mark->connector);
 
+	if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR))
+		group->fanotify_data.error_event_marks++;
+
 out:
 	mutex_unlock(&group->mark_mutex);
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9941c06b8c8a..96e1d31394ce 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -247,6 +247,7 @@ struct fsnotify_group {
 			int f_flags; /* event_f_flags from fanotify_init() */
 			struct ucounts *ucounts;
 			mempool_t error_events_pool;
+			unsigned int error_event_marks;
 		} fanotify_data;
 #endif /* CONFIG_FANOTIFY */
 	};
-- 
2.33.0

Re: [PATCH v7 19/28] fanotify: Limit number of marks with FAN_FS_ERROR per group

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> 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 <krisman@collabora.com>
> ---
>  fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
>  include/linux/fsnotify_backend.h   |  1 +
>  2 files changed, 11 insertions(+)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index f1cf863d6f9f..5324890500fc 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
>
>         removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
>                                                  umask, &destroy_mark);
> +
> +       if (removed & FAN_FS_ERROR)
> +               group->fanotify_data.error_event_marks--;
> +
>         if (removed & fsnotify_conn_mask(fsn_mark->connector))
>                 fsnotify_recalc_mask(fsn_mark->connector);
>         if (destroy_mark)
> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
>
>  static int fanotify_group_init_error_pool(struct fsnotify_group *group)
>  {
> +       if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL)
> +               return -ENOMEM;

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.

Re: [PATCH v7 19/28] fanotify: Limit number of marks with FAN_FS_ERROR per group

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>> 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 <krisman@collabora.com>
>> ---
>>  fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
>>  include/linux/fsnotify_backend.h   |  1 +
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index f1cf863d6f9f..5324890500fc 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
>>
>>         removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
>>                                                  umask, &destroy_mark);
>> +
>> +       if (removed & FAN_FS_ERROR)
>> +               group->fanotify_data.error_event_marks--;
>> +
>>         if (removed & fsnotify_conn_mask(fsn_mark->connector))
>>                 fsnotify_recalc_mask(fsn_mark->connector);
>>         if (destroy_mark)
>> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
>>
>>  static int fanotify_group_init_error_pool(struct fsnotify_group *group)
>>  {
>> +       if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL)
>> +               return -ENOMEM;
>
> Why not try to mempool_resize()?

Jan suggested we might not need to bother with it, but I can do that for
the next version.

> 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?

I don't understand what you mean by two slots for alloc-pre-enqueue and
free-post-dequeue.  I suspect it is no longer necessary now that
FAN_FS_ERROR is handled like any other event on enqueue/dequeue, but can
you confirm or clarify?

Thanks,

-- 
Gabriel Krisman Bertazi

Re: [PATCH v7 19/28] fanotify: Limit number of marks with FAN_FS_ERROR per group

[Amir Goldstein]

On Fri, Oct 15, 2021 at 7:53 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> > <krisman@collabora.com> wrote:
> >>
> >> 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 <krisman@collabora.com>
> >> ---
> >>  fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
> >>  include/linux/fsnotify_backend.h   |  1 +
> >>  2 files changed, 11 insertions(+)
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index f1cf863d6f9f..5324890500fc 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
> >>
> >>         removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
> >>                                                  umask, &destroy_mark);
> >> +
> >> +       if (removed & FAN_FS_ERROR)
> >> +               group->fanotify_data.error_event_marks--;
> >> +
> >>         if (removed & fsnotify_conn_mask(fsn_mark->connector))
> >>                 fsnotify_recalc_mask(fsn_mark->connector);
> >>         if (destroy_mark)
> >> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
> >>
> >>  static int fanotify_group_init_error_pool(struct fsnotify_group *group)
> >>  {
> >> +       if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL)
> >> +               return -ENOMEM;
> >
> > Why not try to mempool_resize()?
>
> Jan suggested we might not need to bother with it, but I can do that for
> the next version.
>
> > 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?
>
> I don't understand what you mean by two slots for alloc-pre-enqueue and
> free-post-dequeue.  I suspect it is no longer necessary now that
> FAN_FS_ERROR is handled like any other event on enqueue/dequeue, but can
> you confirm or clarify?
>

What I meant was, your code is counting error_event_marks.
Every mark accounts for either 1 or 0 queued events, because more
errors from the same fs would merge with the single queued event.

I thought we could have one more event per fs being copied to user
but really we could potentially have many events allocated, before
being "merged", so my calculation was wrong.

Anyway, as Jan explained, the limited size of mempool does not doom
allocations to fail, so we probably have nothing to worry about and
there is no need to mempool_resize().

Thanks,
Amir.

[PATCH v7 20/28] fanotify: Support enqueueing of error events

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
 fs/notify/fanotify/fanotify.h |  6 ++++++
 2 files changed, 41 insertions(+)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 01d68dfc74aa..9b970359570a 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 	return &fne->fae;
 }
 
+static struct fanotify_event *fanotify_alloc_error_event(
+						struct fsnotify_group *group,
+						__kernel_fsid_t *fsid,
+						const void *data, int data_type)
+{
+	struct fs_error_report *report =
+			fsnotify_data_error_report(data, data_type);
+	struct fanotify_error_event *fee;
+
+	if (WARN_ON(!report))
+		return NULL;
+
+	fee = mempool_alloc(&group->fanotify_data.error_events_pool, GFP_NOFS);
+	if (!fee)
+		return NULL;
+
+	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
+
+	return &fee->fae;
+}
+
 static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						   u32 mask, const void *data,
 						   int data_type, struct inode *dir,
@@ -641,6 +662,9 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 
 	if (fanotify_is_perm_event(mask)) {
 		event = fanotify_alloc_perm_event(path, gfp);
+	} else if (fanotify_is_error_event(mask)) {
+		event = fanotify_alloc_error_event(group, fsid, data,
+						   data_type);
 	} else if (name_event && (file_name || child)) {
 		event = fanotify_alloc_name_event(id, fsid, file_name, child,
 						  &hash, gfp);
@@ -850,6 +874,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 	kfree(FANOTIFY_NE(event));
 }
 
+static void fanotify_free_error_event(struct fsnotify_group *group,
+				      struct fanotify_event *event)
+{
+	struct fanotify_error_event *fee = FANOTIFY_EE(event);
+
+	mempool_free(fee, &group->fanotify_data.error_events_pool);
+}
+
 static void fanotify_free_event(struct fsnotify_group *group,
 				struct fsnotify_event *fsn_event)
 {
@@ -873,6 +905,9 @@ static void fanotify_free_event(struct fsnotify_group *group,
 	case FANOTIFY_EVENT_TYPE_OVERFLOW:
 		kfree(event);
 		break;
+	case FANOTIFY_EVENT_TYPE_FS_ERROR:
+		fanotify_free_error_event(group, event);
+		break;
 	default:
 		WARN_ON_ONCE(1);
 	}
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index a577e87fac2b..ebef952481fa 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -298,6 +298,11 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
 	return container_of(fse, struct fanotify_event, fse);
 }
 
+static inline bool fanotify_is_error_event(u32 mask)
+{
+	return mask & FAN_FS_ERROR;
+}
+
 static inline bool fanotify_event_has_path(struct fanotify_event *event)
 {
 	return event->type == FANOTIFY_EVENT_TYPE_PATH ||
@@ -327,6 +332,7 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event)
 static inline bool fanotify_is_hashed_event(u32 mask)
 {
 	return !(fanotify_is_perm_event(mask) ||
+		 fanotify_is_error_event(mask) ||
 		 fsnotify_is_overflow_event(mask));
 }
 
-- 
2.33.0

Re: [PATCH v7 20/28] fanotify: Support enqueueing of error events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
> ---
>  fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
>  fs/notify/fanotify/fanotify.h |  6 ++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 01d68dfc74aa..9b970359570a 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>         return &fne->fae;
>  }
>
> +static struct fanotify_event *fanotify_alloc_error_event(
> +                                               struct fsnotify_group *group,
> +                                               __kernel_fsid_t *fsid,
> +                                               const void *data, int data_type)
> +{
> +       struct fs_error_report *report =
> +                       fsnotify_data_error_report(data, data_type);
> +       struct fanotify_error_event *fee;
> +
> +       if (WARN_ON(!report))

WARN_ON_ONCE please.

Commit message claims to collect the data from the report,
but this commit does nothing with the report??

Thanks,
Amir.

Re: [PATCH v7 20/28] fanotify: Support enqueueing of error events

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> 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 <krisman@collabora.com>
>> ---
>>  fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
>>  fs/notify/fanotify/fanotify.h |  6 ++++++
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> index 01d68dfc74aa..9b970359570a 100644
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>>         return &fne->fae;
>>  }
>>
>> +static struct fanotify_event *fanotify_alloc_error_event(
>> +                                               struct fsnotify_group *group,
>> +                                               __kernel_fsid_t *fsid,
>> +                                               const void *data, int data_type)
>> +{
>> +       struct fs_error_report *report =
>> +                       fsnotify_data_error_report(data, data_type);
>> +       struct fanotify_error_event *fee;
>> +
>> +       if (WARN_ON(!report))
>
> WARN_ON_ONCE please.
>
> Commit message claims to collect the data from the report,
> but this commit does nothing with the report??

I moved it out to a separate commit and forgot to update the commit
message.  Fixed. Thanks!

-- 
Gabriel Krisman Bertazi

Re: [PATCH v7 20/28] fanotify: Support enqueueing of error events

[Jan Kara]

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 <krisman@collabora.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++
>  fs/notify/fanotify/fanotify.h |  6 ++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 01d68dfc74aa..9b970359570a 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>  	return &fne->fae;
>  }
>  
> +static struct fanotify_event *fanotify_alloc_error_event(
> +						struct fsnotify_group *group,
> +						__kernel_fsid_t *fsid,
> +						const void *data, int data_type)
> +{
> +	struct fs_error_report *report =
> +			fsnotify_data_error_report(data, data_type);
> +	struct fanotify_error_event *fee;
> +
> +	if (WARN_ON(!report))
> +		return NULL;
> +
> +	fee = mempool_alloc(&group->fanotify_data.error_events_pool, GFP_NOFS);
> +	if (!fee)
> +		return NULL;
> +
> +	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +
> +	return &fee->fae;
> +}
> +
>  static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  						   u32 mask, const void *data,
>  						   int data_type, struct inode *dir,
> @@ -641,6 +662,9 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  
>  	if (fanotify_is_perm_event(mask)) {
>  		event = fanotify_alloc_perm_event(path, gfp);
> +	} else if (fanotify_is_error_event(mask)) {
> +		event = fanotify_alloc_error_event(group, fsid, data,
> +						   data_type);
>  	} else if (name_event && (file_name || child)) {
>  		event = fanotify_alloc_name_event(id, fsid, file_name, child,
>  						  &hash, gfp);
> @@ -850,6 +874,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)
>  	kfree(FANOTIFY_NE(event));
>  }
>  
> +static void fanotify_free_error_event(struct fsnotify_group *group,
> +				      struct fanotify_event *event)
> +{
> +	struct fanotify_error_event *fee = FANOTIFY_EE(event);
> +
> +	mempool_free(fee, &group->fanotify_data.error_events_pool);
> +}
> +
>  static void fanotify_free_event(struct fsnotify_group *group,
>  				struct fsnotify_event *fsn_event)
>  {
> @@ -873,6 +905,9 @@ static void fanotify_free_event(struct fsnotify_group *group,
>  	case FANOTIFY_EVENT_TYPE_OVERFLOW:
>  		kfree(event);
>  		break;
> +	case FANOTIFY_EVENT_TYPE_FS_ERROR:
> +		fanotify_free_error_event(group, event);
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  	}
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index a577e87fac2b..ebef952481fa 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -298,6 +298,11 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
>  	return container_of(fse, struct fanotify_event, fse);
>  }
>  
> +static inline bool fanotify_is_error_event(u32 mask)
> +{
> +	return mask & FAN_FS_ERROR;
> +}
> +
>  static inline bool fanotify_event_has_path(struct fanotify_event *event)
>  {
>  	return event->type == FANOTIFY_EVENT_TYPE_PATH ||
> @@ -327,6 +332,7 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event)
>  static inline bool fanotify_is_hashed_event(u32 mask)
>  {
>  	return !(fanotify_is_perm_event(mask) ||
> +		 fanotify_is_error_event(mask) ||
>  		 fsnotify_is_overflow_event(mask));
>  }
>  
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 21/28] fanotify: Support merging of error events

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
 fs/notify/fanotify/fanotify.h |  4 +++-
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 9b970359570a..7032083df62a 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
 	return fanotify_info_equal(info1, info2);
 }
 
+static bool fanotify_error_event_equal(struct fanotify_error_event *fee1,
+				       struct fanotify_error_event *fee2)
+{
+	/* Error events against the same file system are always merged. */
+	if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid))
+		return false;
+
+	return true;
+}
+
 static bool fanotify_should_merge(struct fanotify_event *old,
 				  struct fanotify_event *new)
 {
@@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old,
 	case FANOTIFY_EVENT_TYPE_FID_NAME:
 		return fanotify_name_event_equal(FANOTIFY_NE(old),
 						 FANOTIFY_NE(new));
+	case FANOTIFY_EVENT_TYPE_FS_ERROR:
+		return fanotify_error_event_equal(FANOTIFY_EE(old),
+						  FANOTIFY_EE(new));
 	default:
 		WARN_ON_ONCE(1);
 	}
@@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old,
 	return false;
 }
 
+static void fanotify_merge_error_event(struct fanotify_error_event *dest,
+				       struct fanotify_error_event *origin)
+{
+	dest->err_count++;
+}
+
+static void fanotify_merge_event(struct fanotify_event *dest,
+				 struct fanotify_event *origin)
+{
+	dest->mask |= origin->mask;
+
+	if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
+		fanotify_merge_error_event(FANOTIFY_EE(dest),
+					   FANOTIFY_EE(origin));
+}
+
 /* Limit event merges to limit CPU overhead per event */
 #define FANOTIFY_MAX_MERGE_EVENTS 128
 
@@ -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);
 			return 1;
 		}
 	}
@@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 static struct fanotify_event *fanotify_alloc_error_event(
 						struct fsnotify_group *group,
 						__kernel_fsid_t *fsid,
-						const void *data, int data_type)
+						const void *data, int data_type,
+						unsigned int *hash)
 {
 	struct fs_error_report *report =
 			fsnotify_data_error_report(data, data_type);
@@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
 		return NULL;
 
 	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
+	fee->err_count = 1;
+
+	*hash ^= fanotify_hash_fsid(fsid);
 
 	return &fee->fae;
 }
@@ -664,7 +697,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		event = fanotify_alloc_perm_event(path, gfp);
 	} else if (fanotify_is_error_event(mask)) {
 		event = fanotify_alloc_error_event(group, fsid, data,
-						   data_type);
+						   data_type, &hash);
 	} else if (name_event && (file_name || child)) {
 		event = fanotify_alloc_name_event(id, fsid, file_name, child,
 						  &hash, gfp);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index ebef952481fa..2b032b79d5b0 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -199,6 +199,9 @@ FANOTIFY_NE(struct fanotify_event *event)
 
 struct fanotify_error_event {
 	struct fanotify_event fae;
+	u32 err_count; /* Suppressed errors count */
+
+	__kernel_fsid_t fsid; /* FSID this error refers to. */
 };
 
 static inline struct fanotify_error_event *
@@ -332,7 +335,6 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event)
 static inline bool fanotify_is_hashed_event(u32 mask)
 {
 	return !(fanotify_is_perm_event(mask) ||
-		 fanotify_is_error_event(mask) ||
 		 fsnotify_is_overflow_event(mask));
 }
 
-- 
2.33.0

Re: [PATCH v7 21/28] fanotify: Support merging of error events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
> ---
>  fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
>  fs/notify/fanotify/fanotify.h |  4 +++-
>  2 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 9b970359570a..7032083df62a 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
>         return fanotify_info_equal(info1, info2);
>  }
>
> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1,
> +                                      struct fanotify_error_event *fee2)
> +{
> +       /* Error events against the same file system are always merged. */
> +       if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid))
> +               return false;
> +
> +       return true;
> +}
> +
>  static bool fanotify_should_merge(struct fanotify_event *old,
>                                   struct fanotify_event *new)
>  {
> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>         case FANOTIFY_EVENT_TYPE_FID_NAME:
>                 return fanotify_name_event_equal(FANOTIFY_NE(old),
>                                                  FANOTIFY_NE(new));
> +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
> +               return fanotify_error_event_equal(FANOTIFY_EE(old),
> +                                                 FANOTIFY_EE(new));
>         default:
>                 WARN_ON_ONCE(1);
>         }
> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>         return false;
>  }
>
> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
> +                                      struct fanotify_error_event *origin)
> +{
> +       dest->err_count++;
> +}
> +
> +static void fanotify_merge_event(struct fanotify_event *dest,
> +                                struct fanotify_event *origin)
> +{
> +       dest->mask |= origin->mask;
> +
> +       if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
> +               fanotify_merge_error_event(FANOTIFY_EE(dest),
> +                                          FANOTIFY_EE(origin));
> +}
> +
>  /* Limit event merges to limit CPU overhead per event */
>  #define FANOTIFY_MAX_MERGE_EVENTS 128
>
> @@ -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);
>                         return 1;
>                 }
>         }
> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>  static struct fanotify_event *fanotify_alloc_error_event(
>                                                 struct fsnotify_group *group,
>                                                 __kernel_fsid_t *fsid,
> -                                               const void *data, int data_type)
> +                                               const void *data, int data_type,
> +                                               unsigned int *hash)
>  {
>         struct fs_error_report *report =
>                         fsnotify_data_error_report(data, data_type);
> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>                 return NULL;
>
>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +       fee->err_count = 1;
> +
> +       *hash ^= fanotify_hash_fsid(fsid);
>
>         return &fee->fae;
>  }

Forgot to store fee->fsid?

Thanks,
Amir.

Re: [PATCH v7 21/28] fanotify: Support merging of error events

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> 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 <krisman@collabora.com>
>> ---
>>  fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
>>  fs/notify/fanotify/fanotify.h |  4 +++-
>>  2 files changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> index 9b970359570a..7032083df62a 100644
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
>>         return fanotify_info_equal(info1, info2);
>>  }
>>
>> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1,
>> +                                      struct fanotify_error_event *fee2)
>> +{
>> +       /* Error events against the same file system are always merged. */
>> +       if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid))
>> +               return false;
>> +
>> +       return true;
>> +}
>> +
>>  static bool fanotify_should_merge(struct fanotify_event *old,
>>                                   struct fanotify_event *new)
>>  {
>> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>>         case FANOTIFY_EVENT_TYPE_FID_NAME:
>>                 return fanotify_name_event_equal(FANOTIFY_NE(old),
>>                                                  FANOTIFY_NE(new));
>> +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
>> +               return fanotify_error_event_equal(FANOTIFY_EE(old),
>> +                                                 FANOTIFY_EE(new));
>>         default:
>>                 WARN_ON_ONCE(1);
>>         }
>> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>>         return false;
>>  }
>>
>> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
>> +                                      struct fanotify_error_event *origin)
>> +{
>> +       dest->err_count++;
>> +}
>> +
>> +static void fanotify_merge_event(struct fanotify_event *dest,
>> +                                struct fanotify_event *origin)
>> +{
>> +       dest->mask |= origin->mask;
>> +
>> +       if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
>> +               fanotify_merge_error_event(FANOTIFY_EE(dest),
>> +                                          FANOTIFY_EE(origin));
>> +}
>> +
>>  /* Limit event merges to limit CPU overhead per event */
>>  #define FANOTIFY_MAX_MERGE_EVENTS 128
>>
>> @@ -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);
>>                         return 1;
>>                 }
>>         }
>> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>>  static struct fanotify_event *fanotify_alloc_error_event(
>>                                                 struct fsnotify_group *group,
>>                                                 __kernel_fsid_t *fsid,
>> -                                               const void *data, int data_type)
>> +                                               const void *data, int data_type,
>> +                                               unsigned int *hash)
>>  {
>>         struct fs_error_report *report =
>>                         fsnotify_data_error_report(data, data_type);
>> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>>                 return NULL;
>>
>>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
>> +       fee->err_count = 1;
>> +
>> +       *hash ^= fanotify_hash_fsid(fsid);
>>
>>         return &fee->fae;
>>  }
>
> Forgot to store fee->fsid?

Not really. this is part of the FID info record support, which is done
in patch 23.

-- 
Gabriel Krisman Bertazi

Re: [PATCH v7 21/28] fanotify: Support merging of error events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 7:54 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> > <krisman@collabora.com> 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 <krisman@collabora.com>
> >> ---
> >>  fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
> >>  fs/notify/fanotify/fanotify.h |  4 +++-
> >>  2 files changed, 39 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> >> index 9b970359570a..7032083df62a 100644
> >> --- a/fs/notify/fanotify/fanotify.c
> >> +++ b/fs/notify/fanotify/fanotify.c
> >> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
> >>         return fanotify_info_equal(info1, info2);
> >>  }
> >>
> >> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1,
> >> +                                      struct fanotify_error_event *fee2)
> >> +{
> >> +       /* Error events against the same file system are always merged. */
> >> +       if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid))
> >> +               return false;
> >> +
> >> +       return true;
> >> +}
> >> +
> >>  static bool fanotify_should_merge(struct fanotify_event *old,
> >>                                   struct fanotify_event *new)
> >>  {
> >> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old,
> >>         case FANOTIFY_EVENT_TYPE_FID_NAME:
> >>                 return fanotify_name_event_equal(FANOTIFY_NE(old),
> >>                                                  FANOTIFY_NE(new));
> >> +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
> >> +               return fanotify_error_event_equal(FANOTIFY_EE(old),
> >> +                                                 FANOTIFY_EE(new));
> >>         default:
> >>                 WARN_ON_ONCE(1);
> >>         }
> >> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old,
> >>         return false;
> >>  }
> >>
> >> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
> >> +                                      struct fanotify_error_event *origin)
> >> +{
> >> +       dest->err_count++;
> >> +}
> >> +
> >> +static void fanotify_merge_event(struct fanotify_event *dest,
> >> +                                struct fanotify_event *origin)
> >> +{
> >> +       dest->mask |= origin->mask;
> >> +
> >> +       if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
> >> +               fanotify_merge_error_event(FANOTIFY_EE(dest),
> >> +                                          FANOTIFY_EE(origin));
> >> +}
> >> +
> >>  /* Limit event merges to limit CPU overhead per event */
> >>  #define FANOTIFY_MAX_MERGE_EVENTS 128
> >>
> >> @@ -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);
> >>                         return 1;
> >>                 }
> >>         }
> >> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
> >>  static struct fanotify_event *fanotify_alloc_error_event(
> >>                                                 struct fsnotify_group *group,
> >>                                                 __kernel_fsid_t *fsid,
> >> -                                               const void *data, int data_type)
> >> +                                               const void *data, int data_type,
> >> +                                               unsigned int *hash)
> >>  {
> >>         struct fs_error_report *report =
> >>                         fsnotify_data_error_report(data, data_type);
> >> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
> >>                 return NULL;
> >>
> >>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> >> +       fee->err_count = 1;
> >> +
> >> +       *hash ^= fanotify_hash_fsid(fsid);
> >>
> >>         return &fee->fae;
> >>  }
> >
> > Forgot to store fee->fsid?
>
> Not really. this is part of the FID info record support, which is done
> in patch 23.
>

Sure, it does not really matter for bisection when FS_ERROR is not yet
wired, but it is weird to compare event fsid's when they have not been
initialized.

Logically, storing the fsid in this patch would be better.

Thanks,
Amir.

Re: [PATCH v7 21/28] fanotify: Support merging of error events

[Gabriel Krisman Bertazi]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Oct 15, 2021 at 7:54 PM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
>>
>> Amir Goldstein <amir73il@gmail.com> writes:
>>
>> > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
>> > <krisman@collabora.com> 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 <krisman@collabora.com>
>> >> ---
>> >>  fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++---
>> >>  fs/notify/fanotify/fanotify.h |  4 +++-
>> >>  2 files changed, 39 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> >> index 9b970359570a..7032083df62a 100644
>> >> --- a/fs/notify/fanotify/fanotify.c
>> >> +++ b/fs/notify/fanotify/fanotify.c
>> >> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
>> >>         return fanotify_info_equal(info1, info2);
>> >>  }
>> >>
>> >> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1,
>> >> +                                      struct fanotify_error_event *fee2)
>> >> +{
>> >> +       /* Error events against the same file system are always merged. */
>> >> +       if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid))
>> >> +               return false;
>> >> +
>> >> +       return true;
>> >> +}
>> >> +
>> >>  static bool fanotify_should_merge(struct fanotify_event *old,
>> >>                                   struct fanotify_event *new)
>> >>  {
>> >> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>> >>         case FANOTIFY_EVENT_TYPE_FID_NAME:
>> >>                 return fanotify_name_event_equal(FANOTIFY_NE(old),
>> >>                                                  FANOTIFY_NE(new));
>> >> +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
>> >> +               return fanotify_error_event_equal(FANOTIFY_EE(old),
>> >> +                                                 FANOTIFY_EE(new));
>> >>         default:
>> >>                 WARN_ON_ONCE(1);
>> >>         }
>> >> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old,
>> >>         return false;
>> >>  }
>> >>
>> >> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
>> >> +                                      struct fanotify_error_event *origin)
>> >> +{
>> >> +       dest->err_count++;
>> >> +}
>> >> +
>> >> +static void fanotify_merge_event(struct fanotify_event *dest,
>> >> +                                struct fanotify_event *origin)
>> >> +{
>> >> +       dest->mask |= origin->mask;
>> >> +
>> >> +       if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
>> >> +               fanotify_merge_error_event(FANOTIFY_EE(dest),
>> >> +                                          FANOTIFY_EE(origin));
>> >> +}
>> >> +
>> >>  /* Limit event merges to limit CPU overhead per event */
>> >>  #define FANOTIFY_MAX_MERGE_EVENTS 128
>> >>
>> >> @@ -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);
>> >>                         return 1;
>> >>                 }
>> >>         }
>> >> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>> >>  static struct fanotify_event *fanotify_alloc_error_event(
>> >>                                                 struct fsnotify_group *group,
>> >>                                                 __kernel_fsid_t *fsid,
>> >> -                                               const void *data, int data_type)
>> >> +                                               const void *data, int data_type,
>> >> +                                               unsigned int *hash)
>> >>  {
>> >>         struct fs_error_report *report =
>> >>                         fsnotify_data_error_report(data, data_type);
>> >> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>> >>                 return NULL;
>> >>
>> >>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
>> >> +       fee->err_count = 1;
>> >> +
>> >> +       *hash ^= fanotify_hash_fsid(fsid);
>> >>
>> >>         return &fee->fae;
>> >>  }
>> >
>> > Forgot to store fee->fsid?
>>
>> Not really. this is part of the FID info record support, which is done
>> in patch 23.
>>
>
> Sure, it does not really matter for bisection when FS_ERROR is not yet
> wired, but it is weird to compare event fsid's when they have not been
> initialized.
>
> Logically, storing the fsid in this patch would be better.

Makes sense.  Will do!

Thanks,


-- 
Gabriel Krisman Bertazi

Re: [PATCH v7 21/28] fanotify: Support merging of error events

[Jan Kara]

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 <krisman@collabora.com>

...

> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
> +				       struct fanotify_error_event *origin)
> +{
> +	dest->err_count++;
> +}
> +
> +static void fanotify_merge_event(struct fanotify_event *dest,
> +				 struct fanotify_event *origin)
> +{
> +	dest->mask |= origin->mask;
> +
> +	if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
> +		fanotify_merge_error_event(FANOTIFY_EE(dest),
> +					   FANOTIFY_EE(origin));
> +}
> +
>  /* Limit event merges to limit CPU overhead per event */
>  #define FANOTIFY_MAX_MERGE_EVENTS 128
>  
> @@ -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().

> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>  		return NULL;
>  
>  	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +	fee->err_count = 1;
> +
> +	*hash ^= fanotify_hash_fsid(fsid);
>  
>  	return &fee->fae;
>  }

As Amir mentioned, you should set fsid here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 22/28] fanotify: Report FID entry even for zero-length file_handle

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 5324890500fc..39cf8ba4a6ce 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
 		       FANOTIFY_EVENT_ALIGN);
 }
 
+static bool fanotify_event_allows_empty_fh(struct fanotify_event *event)
+{
+	switch (event->type) {
+	case FANOTIFY_EVENT_TYPE_FS_ERROR:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static size_t fanotify_event_len(unsigned int info_mode,
 				 struct fanotify_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);
 
 	return event_len;
@@ -338,9 +348,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
 	pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
 		 __func__, fh_len, name_len, info_len, count);
 
-	if (!fh_len)
-		return 0;
-
 	if (WARN_ON_ONCE(len < sizeof(info) || len > count))
 		return -EFAULT;
 
@@ -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)) {
 		const char *dot = NULL;
 		int dot_len = 0;
 
-- 
2.33.0

Re: [PATCH v7 22/28] fanotify: Report FID entry even for zero-length file_handle

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> 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 <krisman@collabora.com>
> ---
>  fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 5324890500fc..39cf8ba4a6ce 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
>                        FANOTIFY_EVENT_ALIGN);
>  }
>
> +static bool fanotify_event_allows_empty_fh(struct fanotify_event *event)
> +{
> +       switch (event->type) {
> +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
> +               return true;
> +       default:
> +               return false;
> +       }
> +}
> +
>  static size_t fanotify_event_len(unsigned int info_mode,
>                                  struct fanotify_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);
>
>         return event_len;
> @@ -338,9 +348,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
>         pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
>                  __func__, fh_len, name_len, info_len, count);
>
> -       if (!fh_len)
> -               return 0;
> -
>         if (WARN_ON_ONCE(len < sizeof(info) || len > count))
>                 return -EFAULT;
>
> @@ -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)) {
>                 const char *dot = NULL;
>                 int dot_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.

Re: [PATCH v7 22/28] fanotify: Report FID entry even for zero-length file_handle

[Jan Kara]

On Fri 15-10-21 11:10:58, Amir Goldstein wrote:
> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> >
> > 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 <krisman@collabora.com>
> > ---
> >  fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 5324890500fc..39cf8ba4a6ce 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
> >                        FANOTIFY_EVENT_ALIGN);
> >  }
> >
> > +static bool fanotify_event_allows_empty_fh(struct fanotify_event *event)
> > +{
> > +       switch (event->type) {
> > +       case FANOTIFY_EVENT_TYPE_FS_ERROR:
> > +               return true;
> > +       default:
> > +               return false;
> > +       }
> > +}
> > +
> >  static size_t fanotify_event_len(unsigned int info_mode,
> >                                  struct fanotify_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);
> >
> >         return event_len;
> > @@ -338,9 +348,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
> >         pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
> >                  __func__, fh_len, name_len, info_len, count);
> >
> > -       if (!fh_len)
> > -               return 0;
> > -
> >         if (WARN_ON_ONCE(len < sizeof(info) || len > count))
> >                 return -EFAULT;
> >
> > @@ -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)) {
> >                 const char *dot = NULL;
> >                 int dot_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 <jack@suse.com>
SUSE Labs, CR

[PATCH v7 23/28] fanotify: Report fid info for file related file system errors

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(+)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 7032083df62a..8a60c96f5fb2 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -611,7 +611,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
 {
 	struct fs_error_report *report =
 			fsnotify_data_error_report(data, data_type);
+	struct inode *inode = report->inode;
 	struct fanotify_error_event *fee;
+	int fh_len;
 
 	if (WARN_ON(!report))
 		return NULL;
@@ -622,6 +624,19 @@ static struct fanotify_event *fanotify_alloc_error_event(
 
 	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
 	fee->err_count = 1;
+	fee->fsid = *fsid;
+
+	fh_len = fanotify_encode_fh_len(inode);
+	if (WARN_ON(fh_len > MAX_HANDLE_SZ)) {
+		/*
+		 * Fallback to reporting the error against the super
+		 * block.  It should never happen.
+		 */
+		inode = NULL;
+		fh_len = fanotify_encode_fh_len(NULL);
+	}
+
+	fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0);
 
 	*hash ^= fanotify_hash_fsid(fsid);
 
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 2b032b79d5b0..b58400926f92 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -202,6 +202,10 @@ struct fanotify_error_event {
 	u32 err_count; /* Suppressed errors count */
 
 	__kernel_fsid_t fsid; /* FSID this error refers to. */
+	/* object_fh must be followed by the inline handle buffer. */
+	struct fanotify_fh object_fh;
+	/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */
+	unsigned char _inline_fh_buf[MAX_HANDLE_SZ];
 };
 
 static inline struct fanotify_error_event *
@@ -216,6 +220,8 @@ static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)
 		return &FANOTIFY_FE(event)->fsid;
 	else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
 		return &FANOTIFY_NE(event)->fsid;
+	else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
+		return &FANOTIFY_EE(event)->fsid;
 	else
 		return NULL;
 }
@@ -227,6 +233,8 @@ static inline struct fanotify_fh *fanotify_event_object_fh(
 		return &FANOTIFY_FE(event)->object_fh;
 	else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
 		return fanotify_info_file_fh(&FANOTIFY_NE(event)->info);
+	else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
+		return &FANOTIFY_EE(event)->object_fh;
 	else
 		return NULL;
 }
-- 
2.33.0

Re: [PATCH v7 23/28] fanotify: Report fid info for file related file system errors

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> 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 <krisman@collabora.com>
>
> ---
> 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(+)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 7032083df62a..8a60c96f5fb2 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -611,7 +611,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>  {
>         struct fs_error_report *report =
>                         fsnotify_data_error_report(data, data_type);
> +       struct inode *inode = report->inode;
>         struct fanotify_error_event *fee;
> +       int fh_len;
>
>         if (WARN_ON(!report))
>                 return NULL;
> @@ -622,6 +624,19 @@ static struct fanotify_event *fanotify_alloc_error_event(
>
>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
>         fee->err_count = 1;
> +       fee->fsid = *fsid;
> +
> +       fh_len = fanotify_encode_fh_len(inode);
> +       if (WARN_ON(fh_len > MAX_HANDLE_SZ)) {

WARN_ON_ONCE please and I rather that this sanity check is moved inside
fanotify_encode_fh_len() where it will return 0 for encoding failure.

> +               /*
> +                * Fallback to reporting the error against the super
> +                * block.  It should never happen.
> +                */
> +               inode = NULL;
> +               fh_len = fanotify_encode_fh_len(NULL);
> +       }
> +
> +       fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0);
>
>         *hash ^= fanotify_hash_fsid(fsid);
>
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index 2b032b79d5b0..b58400926f92 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -202,6 +202,10 @@ struct fanotify_error_event {
>         u32 err_count; /* Suppressed errors count */
>
>         __kernel_fsid_t fsid; /* FSID this error refers to. */
> +       /* object_fh must be followed by the inline handle buffer. */
> +       struct fanotify_fh object_fh;
> +       /* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */
> +       unsigned char _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.

Re: [PATCH v7 23/28] fanotify: Report fid info for file related file system errors

[Jan Kara]

On Fri 15-10-21 10:56:38, Amir Goldstein wrote:
> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> >
> > 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 <krisman@collabora.com>
> >
> > ---
> > 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(+)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 7032083df62a..8a60c96f5fb2 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -611,7 +611,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
> >  {
> >         struct fs_error_report *report =
> >                         fsnotify_data_error_report(data, data_type);
> > +       struct inode *inode = report->inode;
> >         struct fanotify_error_event *fee;
> > +       int fh_len;
> >
> >         if (WARN_ON(!report))
> >                 return NULL;
> > @@ -622,6 +624,19 @@ static struct fanotify_event *fanotify_alloc_error_event(
> >
> >         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> >         fee->err_count = 1;
> > +       fee->fsid = *fsid;
> > +
> > +       fh_len = fanotify_encode_fh_len(inode);
> > +       if (WARN_ON(fh_len > MAX_HANDLE_SZ)) {
> 
> WARN_ON_ONCE please and I rather that this sanity check is moved inside
> fanotify_encode_fh_len() where it will return 0 for encoding failure.

Yeah, that's better.

> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index 2b032b79d5b0..b58400926f92 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -202,6 +202,10 @@ struct fanotify_error_event {
> >         u32 err_count; /* Suppressed errors count */
> >
> >         __kernel_fsid_t fsid; /* FSID this error refers to. */
> > +       /* object_fh must be followed by the inline handle buffer. */
> > +       struct fanotify_fh object_fh;
> > +       /* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */
> > +       unsigned char _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 <jack@suse.com>
SUSE Labs, CR

[PATCH v7 24/28] fanotify: Emit generic error info for error event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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(+)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 8a60c96f5fb2..47e28f418711 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -623,6 +623,7 @@ static struct fanotify_event *fanotify_alloc_error_event(
 		return NULL;
 
 	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
+	fee->error = report->error;
 	fee->err_count = 1;
 	fee->fsid = *fsid;
 
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index b58400926f92..a0897425df07 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -199,6 +199,7 @@ FANOTIFY_NE(struct fanotify_event *event)
 
 struct fanotify_error_event {
 	struct fanotify_event fae;
+	s32 error; /* Error reported by the Filesystem. */
 	u32 err_count; /* Suppressed errors count */
 
 	__kernel_fsid_t fsid; /* FSID this error refers to. */
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 39cf8ba4a6ce..8f7c2f4ce674 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -115,6 +115,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 	(sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
 #define FANOTIFY_PIDFD_INFO_HDR_LEN \
 	sizeof(struct fanotify_event_info_pidfd)
+#define FANOTIFY_ERROR_INFO_LEN \
+	(sizeof(struct fanotify_event_info_error))
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {
@@ -149,6 +151,9 @@ static size_t fanotify_event_len(unsigned int info_mode,
 	if (!info_mode)
 		return event_len;
 
+	if (fanotify_is_error_event(event->mask))
+		event_len += FANOTIFY_ERROR_INFO_LEN;
+
 	info = fanotify_event_info(event);
 	dir_fh_len = fanotify_event_dir_fh_len(event);
 	fh_len = fanotify_event_object_fh_len(event);
@@ -333,6 +338,28 @@ static int process_access_response(struct fsnotify_group *group,
 	return -ENOENT;
 }
 
+static size_t copy_error_info_to_user(struct fanotify_event *event,
+				      char __user *buf, int count)
+{
+	struct fanotify_event_info_error info;
+	struct fanotify_error_event *fee = FANOTIFY_EE(event);
+
+	info.hdr.info_type = FAN_EVENT_INFO_TYPE_ERROR;
+	info.hdr.pad = 0;
+	info.hdr.len = FANOTIFY_ERROR_INFO_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;
+
+	return info.hdr.len;
+}
+
 static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
 				 int info_type, const char *name,
 				 size_t name_len,
@@ -540,6 +567,14 @@ static int copy_info_records_to_user(struct fanotify_event *event,
 		total_bytes += ret;
 	}
 
+	if (fanotify_is_error_event(event->mask)) {
+		ret = copy_error_info_to_user(event, buf, count);
+		if (ret < 0)
+			return ret;
+		buf += ret;
+		count -= ret;
+	}
+
 	return total_bytes;
 }
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 2990731ddc8b..bd1932c2074d 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -126,6 +126,7 @@ struct fanotify_event_metadata {
 #define FAN_EVENT_INFO_TYPE_DFID_NAME	2
 #define FAN_EVENT_INFO_TYPE_DFID	3
 #define FAN_EVENT_INFO_TYPE_PIDFD	4
+#define FAN_EVENT_INFO_TYPE_ERROR	5
 
 /* Variable length info record following event metadata */
 struct fanotify_event_info_header {
@@ -160,6 +161,12 @@ struct fanotify_event_info_pidfd {
 	__s32 pidfd;
 };
 
+struct fanotify_event_info_error {
+	struct fanotify_event_info_header hdr;
+	__s32 error;
+	__u32 error_count;
+};
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;
-- 
2.33.0

Re: [PATCH v7 24/28] fanotify: Emit generic error info for error event

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

>
> ---
> 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(+)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 8a60c96f5fb2..47e28f418711 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -623,6 +623,7 @@ static struct fanotify_event *fanotify_alloc_error_event(
>                 return NULL;
>
>         fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +       fee->error = report->error;
>         fee->err_count = 1;
>         fee->fsid = *fsid;
>
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index b58400926f92..a0897425df07 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -199,6 +199,7 @@ FANOTIFY_NE(struct fanotify_event *event)
>
>  struct fanotify_error_event {
>         struct fanotify_event fae;
> +       s32 error; /* Error reported by the Filesystem. */
>         u32 err_count; /* Suppressed errors count */
>
>         __kernel_fsid_t fsid; /* FSID this error refers to. */
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 39cf8ba4a6ce..8f7c2f4ce674 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -115,6 +115,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
>         (sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
>  #define FANOTIFY_PIDFD_INFO_HDR_LEN \
>         sizeof(struct fanotify_event_info_pidfd)
> +#define FANOTIFY_ERROR_INFO_LEN \
> +       (sizeof(struct fanotify_event_info_error))
>
>  static int fanotify_fid_info_len(int fh_len, int name_len)
>  {
> @@ -149,6 +151,9 @@ static size_t fanotify_event_len(unsigned int info_mode,
>         if (!info_mode)
>                 return event_len;
>
> +       if (fanotify_is_error_event(event->mask))
> +               event_len += FANOTIFY_ERROR_INFO_LEN;
> +
>         info = fanotify_event_info(event);
>         dir_fh_len = fanotify_event_dir_fh_len(event);
>         fh_len = fanotify_event_object_fh_len(event);
> @@ -333,6 +338,28 @@ static int process_access_response(struct fsnotify_group *group,
>         return -ENOENT;
>  }
>
> +static size_t copy_error_info_to_user(struct fanotify_event *event,
> +                                     char __user *buf, int count)
> +{
> +       struct fanotify_event_info_error info;
> +       struct fanotify_error_event *fee = FANOTIFY_EE(event);
> +
> +       info.hdr.info_type = FAN_EVENT_INFO_TYPE_ERROR;
> +       info.hdr.pad = 0;
> +       info.hdr.len = FANOTIFY_ERROR_INFO_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;
> +
> +       return info.hdr.len;
> +}
> +
>  static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
>                                  int info_type, const char *name,
>                                  size_t name_len,
> @@ -540,6 +567,14 @@ static int copy_info_records_to_user(struct fanotify_event *event,
>                 total_bytes += ret;
>         }
>
> +       if (fanotify_is_error_event(event->mask)) {
> +               ret = copy_error_info_to_user(event, buf, count);
> +               if (ret < 0)
> +                       return ret;
> +               buf += ret;
> +               count -= ret;
> +       }
> +
>         return total_bytes;
>  }
>
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index 2990731ddc8b..bd1932c2074d 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -126,6 +126,7 @@ struct fanotify_event_metadata {
>  #define FAN_EVENT_INFO_TYPE_DFID_NAME  2
>  #define FAN_EVENT_INFO_TYPE_DFID       3
>  #define FAN_EVENT_INFO_TYPE_PIDFD      4
> +#define FAN_EVENT_INFO_TYPE_ERROR      5
>
>  /* Variable length info record following event metadata */
>  struct fanotify_event_info_header {
> @@ -160,6 +161,12 @@ struct fanotify_event_info_pidfd {
>         __s32 pidfd;
>  };
>
> +struct fanotify_event_info_error {
> +       struct fanotify_event_info_header hdr;
> +       __s32 error;
> +       __u32 error_count;
> +};
> +
>  struct fanotify_response {
>         __s32 fd;
>         __u32 response;
> --
> 2.33.0
>

Re: [PATCH v7 24/28] fanotify: Emit generic error info for error event

[Jan Kara]

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 <krisman@collabora.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 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(+)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 8a60c96f5fb2..47e28f418711 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -623,6 +623,7 @@ static struct fanotify_event *fanotify_alloc_error_event(
>  		return NULL;
>  
>  	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +	fee->error = report->error;
>  	fee->err_count = 1;
>  	fee->fsid = *fsid;
>  
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index b58400926f92..a0897425df07 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -199,6 +199,7 @@ FANOTIFY_NE(struct fanotify_event *event)
>  
>  struct fanotify_error_event {
>  	struct fanotify_event fae;
> +	s32 error; /* Error reported by the Filesystem. */
>  	u32 err_count; /* Suppressed errors count */
>  
>  	__kernel_fsid_t fsid; /* FSID this error refers to. */
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 39cf8ba4a6ce..8f7c2f4ce674 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -115,6 +115,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
>  	(sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
>  #define FANOTIFY_PIDFD_INFO_HDR_LEN \
>  	sizeof(struct fanotify_event_info_pidfd)
> +#define FANOTIFY_ERROR_INFO_LEN \
> +	(sizeof(struct fanotify_event_info_error))
>  
>  static int fanotify_fid_info_len(int fh_len, int name_len)
>  {
> @@ -149,6 +151,9 @@ static size_t fanotify_event_len(unsigned int info_mode,
>  	if (!info_mode)
>  		return event_len;
>  
> +	if (fanotify_is_error_event(event->mask))
> +		event_len += FANOTIFY_ERROR_INFO_LEN;
> +
>  	info = fanotify_event_info(event);
>  	dir_fh_len = fanotify_event_dir_fh_len(event);
>  	fh_len = fanotify_event_object_fh_len(event);
> @@ -333,6 +338,28 @@ static int process_access_response(struct fsnotify_group *group,
>  	return -ENOENT;
>  }
>  
> +static size_t copy_error_info_to_user(struct fanotify_event *event,
> +				      char __user *buf, int count)
> +{
> +	struct fanotify_event_info_error info;
> +	struct fanotify_error_event *fee = FANOTIFY_EE(event);
> +
> +	info.hdr.info_type = FAN_EVENT_INFO_TYPE_ERROR;
> +	info.hdr.pad = 0;
> +	info.hdr.len = FANOTIFY_ERROR_INFO_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;
> +
> +	return info.hdr.len;
> +}
> +
>  static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
>  				 int info_type, const char *name,
>  				 size_t name_len,
> @@ -540,6 +567,14 @@ static int copy_info_records_to_user(struct fanotify_event *event,
>  		total_bytes += ret;
>  	}
>  
> +	if (fanotify_is_error_event(event->mask)) {
> +		ret = copy_error_info_to_user(event, buf, count);
> +		if (ret < 0)
> +			return ret;
> +		buf += ret;
> +		count -= ret;
> +	}
> +
>  	return total_bytes;
>  }
>  
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index 2990731ddc8b..bd1932c2074d 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -126,6 +126,7 @@ struct fanotify_event_metadata {
>  #define FAN_EVENT_INFO_TYPE_DFID_NAME	2
>  #define FAN_EVENT_INFO_TYPE_DFID	3
>  #define FAN_EVENT_INFO_TYPE_PIDFD	4
> +#define FAN_EVENT_INFO_TYPE_ERROR	5
>  
>  /* Variable length info record following event metadata */
>  struct fanotify_event_info_header {
> @@ -160,6 +161,12 @@ struct fanotify_event_info_pidfd {
>  	__s32 pidfd;
>  };
>  
> +struct fanotify_event_info_error {
> +	struct fanotify_event_info_header hdr;
> +	__s32 error;
> +	__u32 error_count;
> +};
> +
>  struct fanotify_response {
>  	__s32 fd;
>  	__u32 response;
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 25/28] fanotify: Allow users to request FAN_FS_ERROR events

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 47e28f418711..d449a23d603f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -827,7 +827,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
 	BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 20);
 
 	mask = fanotify_group_event_mask(group, iter_info, mask, data,
 					 data_type, dir);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8f7c2f4ce674..5edfd7e3f356 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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;
+		goto path_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;
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 52d464802d99..616af2ea20f3 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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 | \
-- 
2.33.0

Re: [PATCH v7 25/28] fanotify: Allow users to request FAN_FS_ERROR events

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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 <krisman@collabora.com>
> ---
>  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(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 47e28f418711..d449a23d603f 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -827,7 +827,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
>         BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
>         BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
>
> -       BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
> +       BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 20);
>
>         mask = fanotify_group_event_mask(group, iter_info, mask, data,
>                                          data_type, dir);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8f7c2f4ce674..5edfd7e3f356 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -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;
> +               goto path_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.

Re: [PATCH v7 25/28] fanotify: Allow users to request FAN_FS_ERROR events

[Jan Kara]

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 <krisman@collabora.com>

No other comment besides what Amir has written...

								Honza

> ---
>  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(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 47e28f418711..d449a23d603f 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -827,7 +827,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
>  	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
>  	BUILD_BUG_ON(FAN_FS_ERROR != FS_ERROR);
>  
> -	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
> +	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 20);
>  
>  	mask = fanotify_group_event_mask(group, iter_info, mask, data,
>  					 data_type, dir);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8f7c2f4ce674..5edfd7e3f356 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -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;
> +		goto path_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;
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 52d464802d99..616af2ea20f3 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -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 | \
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v7 26/28] ext4: Send notifications on error

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>
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(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 88d5d274a868..67183e6b1920 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -46,6 +46,7 @@
 #include <linux/part_stat.h>
 #include <linux/kthread.h>
 #include <linux/freezer.h>
+#include <linux/fsnotify.h>
 
 #include "ext4.h"
 #include "ext4_extents.h"	/* Needed for trace points definition */
@@ -759,6 +760,8 @@ void __ext4_error(struct super_block *sb, const char *function,
 		       sb->s_id, function, line, current->comm, &vaf);
 		va_end(args);
 	}
+	fsnotify_sb_error(sb, NULL, error);
+
 	ext4_handle_error(sb, force_ro, error, 0, block, function, line);
 }
 
@@ -789,6 +792,8 @@ void __ext4_error_inode(struct inode *inode, const char *function,
 			       current->comm, &vaf);
 		va_end(args);
 	}
+	fsnotify_sb_error(inode->i_sb, inode, error);
+
 	ext4_handle_error(inode->i_sb, false, error, inode->i_ino, block,
 			  function, line);
 }
@@ -827,6 +832,8 @@ void __ext4_error_file(struct file *file, const char *function,
 			       current->comm, path, &vaf);
 		va_end(args);
 	}
+	fsnotify_sb_error(inode->i_sb, inode, EFSCORRUPTED);
+
 	ext4_handle_error(inode->i_sb, false, EFSCORRUPTED, inode->i_ino, block,
 			  function, line);
 }
@@ -894,6 +901,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
 		printk(KERN_CRIT "EXT4-fs error (device %s) in %s:%d: %s\n",
 		       sb->s_id, function, line, errstr);
 	}
+	fsnotify_sb_error(sb, NULL, errno);
 
 	ext4_handle_error(sb, false, -errno, 0, 0, function, line);
 }
-- 
2.33.0

Re: [PATCH v7 26/28] ext4: Send notifications on error

[Theodore Ts'o]

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 <krisman@collabora.com>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

[PATCH v7 27/28] samples: Add fs error monitoring example

[Gabriel Krisman Bertazi]

Introduce an example of a FAN_FS_ERROR fanotify user to track filesystem
errors.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v4:
  - Protect file_handle defines with ifdef guards

Changes since v1:
  - minor fixes
---
 samples/Kconfig               |   9 +++
 samples/Makefile              |   1 +
 samples/fanotify/Makefile     |   5 ++
 samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+)
 create mode 100644 samples/fanotify/Makefile
 create mode 100644 samples/fanotify/fs-monitor.c

diff --git a/samples/Kconfig b/samples/Kconfig
index b0503ef058d3..88353b8eac0b 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR
 	  with it.
 	  See also Documentation/driver-api/connector.rst
 
+config SAMPLE_FANOTIFY_ERROR
+	bool "Build fanotify error monitoring sample"
+	depends on FANOTIFY
+	help
+	  When enabled, this builds an example code that uses the
+	  FAN_FS_ERROR fanotify mechanism to monitor filesystem
+	  errors.
+	  See also Documentation/admin-guide/filesystem-monitoring.rst.
+
 config SAMPLE_HIDRAW
 	bool "hidraw sample"
 	depends on CC_CAN_LINK && HEADERS_INSTALL
diff --git a/samples/Makefile b/samples/Makefile
index 087e0988ccc5..931a81847c48 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -5,6 +5,7 @@ subdir-$(CONFIG_SAMPLE_AUXDISPLAY)	+= auxdisplay
 subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs
 obj-$(CONFIG_SAMPLE_CONFIGFS)		+= configfs/
 obj-$(CONFIG_SAMPLE_CONNECTOR)		+= connector/
+obj-$(CONFIG_SAMPLE_FANOTIFY_ERROR)	+= fanotify/
 subdir-$(CONFIG_SAMPLE_HIDRAW)		+= hidraw
 obj-$(CONFIG_SAMPLE_HW_BREAKPOINT)	+= hw_breakpoint/
 obj-$(CONFIG_SAMPLE_KDB)		+= kdb/
diff --git a/samples/fanotify/Makefile b/samples/fanotify/Makefile
new file mode 100644
index 000000000000..e20db1bdde3b
--- /dev/null
+++ b/samples/fanotify/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+userprogs-always-y += fs-monitor
+
+userccflags += -I usr/include -Wall
+
diff --git a/samples/fanotify/fs-monitor.c b/samples/fanotify/fs-monitor.c
new file mode 100644
index 000000000000..a0e44cd31e6f
--- /dev/null
+++ b/samples/fanotify/fs-monitor.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021, Collabora Ltd.
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <err.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <sys/fanotify.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <sys/types.h>
+
+#ifndef FAN_FS_ERROR
+#define FAN_FS_ERROR		0x00008000
+#define FAN_EVENT_INFO_TYPE_ERROR	5
+
+struct fanotify_event_info_error {
+	struct fanotify_event_info_header hdr;
+	__s32 error;
+	__u32 error_count;
+};
+#endif
+
+#ifndef FILEID_INO32_GEN
+#define FILEID_INO32_GEN	1
+#endif
+
+#ifndef FILEID_INVALID
+#define	FILEID_INVALID		0xff
+#endif
+
+static void print_fh(struct file_handle *fh)
+{
+	int i;
+	uint32_t *h = (uint32_t *) fh->f_handle;
+
+	printf("\tfh: ");
+	for (i = 0; i < fh->handle_bytes; i++)
+		printf("%hhx", fh->f_handle[i]);
+	printf("\n");
+
+	printf("\tdecoded fh: ");
+	if (fh->handle_type == FILEID_INO32_GEN)
+		printf("inode=%u gen=%u\n", h[0], h[1]);
+	else if (fh->handle_type == FILEID_INVALID && !fh->handle_bytes)
+		printf("Type %d (Superblock error)\n", fh->handle_type);
+	else
+		printf("Type %d (Unknown)\n", fh->handle_type);
+
+}
+
+static void handle_notifications(char *buffer, int len)
+{
+	struct fanotify_event_metadata *event =
+		(struct fanotify_event_metadata *) buffer;
+	struct fanotify_event_info_header *info;
+	struct fanotify_event_info_error *err;
+	struct fanotify_event_info_fid *fid;
+	int off;
+
+	for (; FAN_EVENT_OK(event, len); event = FAN_EVENT_NEXT(event, len)) {
+
+		if (event->mask != FAN_FS_ERROR) {
+			printf("unexpected FAN MARK: %llx\n", event->mask);
+			goto next_event;
+		}
+
+		if (event->fd != FAN_NOFD) {
+			printf("Unexpected fd (!= FAN_NOFD)\n");
+			goto next_event;
+		}
+
+		printf("FAN_FS_ERROR (len=%d)\n", event->event_len);
+
+		for (off = sizeof(*event) ; off < event->event_len;
+		     off += info->len) {
+			info = (struct fanotify_event_info_header *)
+				((char *) event + off);
+
+			switch (info->info_type) {
+			case FAN_EVENT_INFO_TYPE_ERROR:
+				err = (struct fanotify_event_info_error *) info;
+
+				printf("\tGeneric Error Record: len=%d\n",
+				       err->hdr.len);
+				printf("\terror: %d\n", err->error);
+				printf("\terror_count: %d\n", err->error_count);
+				break;
+
+			case FAN_EVENT_INFO_TYPE_FID:
+				fid = (struct fanotify_event_info_fid *) info;
+
+				printf("\tfsid: %x%x\n",
+				       fid->fsid.val[0], fid->fsid.val[1]);
+				print_fh((struct file_handle *) &fid->handle);
+				break;
+
+			default:
+				printf("\tUnknown info type=%d len=%d:\n",
+				       info->info_type, info->len);
+			}
+		}
+next_event:
+		printf("---\n\n");
+	}
+}
+
+int main(int argc, char **argv)
+{
+	int fd;
+
+	char buffer[BUFSIZ];
+
+	if (argc < 2) {
+		printf("Missing path argument\n");
+		return 1;
+	}
+
+	fd = fanotify_init(FAN_CLASS_NOTIF|FAN_REPORT_FID, O_RDONLY);
+	if (fd < 0)
+		errx(1, "fanotify_init");
+
+	if (fanotify_mark(fd, FAN_MARK_ADD|FAN_MARK_FILESYSTEM,
+			  FAN_FS_ERROR, AT_FDCWD, argv[1])) {
+		errx(1, "fanotify_mark");
+	}
+
+	while (1) {
+		int n = read(fd, buffer, BUFSIZ);
+
+		if (n < 0)
+			errx(1, "read");
+
+		handle_notifications(buffer, n);
+	}
+
+	return 0;
+}
-- 
2.33.0

[PATCH v7 28/28] docs: Document the FAN_FS_ERROR event

[Gabriel Krisman Bertazi]

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 <krisman@collabora.com>

---
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

diff --git a/Documentation/admin-guide/filesystem-monitoring.rst b/Documentation/admin-guide/filesystem-monitoring.rst
new file mode 100644
index 000000000000..f1f6476fa4f3
--- /dev/null
+++ b/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.
diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
index dc00afcabb95..1bedab498104 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -82,6 +82,7 @@ configure specific aspects of kernel behavior to your liking.
    edid
    efi-stub
    ext4
+   filesystem-monitoring
    nfs/index
    gpio/index
    highuid
-- 
2.33.0

Re: [PATCH v7 00/28] file system-wide error monitoring

[Amir Goldstein]

On Fri, Oct 15, 2021 at 12:37 AM Gabriel Krisman Bertazi
<krisman@collabora.com> 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.

Re: [PATCH v7 00/28] file system-wide error monitoring

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR