Re: [PATCH v3 12/15] fanotify: Introduce FAN_FS_ERROR event

Amir Goldstein [off-list ref] writes:

quoted

+       fee->fsid = fee->mark->connector->fsid;
+
+       fsnotify_get_mark(fee->mark);
+
+       /*
+        * Error reporting needs to happen in atomic context.  If this
+        * inode's file handler is more than we initially predicted,
+        * there is nothing better we can do than report the error with
+        * a bad FH.
+        */
+       fh_len = fanotify_encode_fh_len(inode);
+       if (WARN_ON(fh_len > fee->max_fh_len))

WARN_ON() is not acceptable for things that can logically happen
if you think this is important you could use pr_warn_ratelimited()
like we do in fanotify_encode_fh(),
but since fs-monitor will observe the lack of FID anyway, I think
there is little point in reporting this to kmsg.

Hi Amir,

Thanks for all the review so far.

Consider that fh_len > max_fh_len can happen only if the filesystem
requires a longer handler for the failed inode than it requires for the
root inode.  Looking at the FH types, I don't think this would be
possible to happen currently, but this WARN_ON is trying to catch future
problems.

Notice this would not be a fs-monitor misuse of the uAPI,  but an actual
kernel bug. The FH size we predicted when allocating the static error
slot is not large enough for at least one FH of this filesystem.  So I
think a WARN_ON or a pr_warn is desired.  I will change it to a
pr_warn_ratelimited as you suggested.

quoted

@@ -896,6 +933,43 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
                                    flags, umask);
 }

+static int fanotify_create_fs_error_event(struct fsnotify_mark *fsn_mark,
+                                          fsnotify_connp_t *connp)
+{
+       struct fanotify_sb_mark *sb_mark = FANOTIFY_SB_MARK(fsn_mark);
+       struct super_block *sb =
+               container_of(connp, struct super_block, s_fsnotify_marks);
+       struct fanotify_error_event *fee;
+       int fh_len;
+
+       /*
+        * Since the allocation is done holding group->mark_mutex, the
+        * error event allocation is guaranteed not to race with itself.

If this is protected by a mutex then READ_ONCE/WRITE_ONCE are not need
and the comment above is confusing.
You should fire your code reviewer ;-)

okay :)

quoted

+        */
+       if (READ_ONCE(sb_mark->error_event))
+               return 0;
+
+       /* Since, for error events, every memory must be preallocated,
+        * the FH buffer size is predicted to be the same as the root
+        * inode file handler size.  This should work for file systems
+        * without variable sized FH.
+        */
+       fh_len = fanotify_encode_fh_len(sb->s_root->d_inode);
+
+       fee = kzalloc(sizeof(*fee) + fh_len, GFP_KERNEL);

GFP_KERNEL_ACCOUNT

will do.

quoted

diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index a16dbeced152..d086a19aff63 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h

@@ -81,13 +81,17 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
  */
 #define FANOTIFY_DIRENT_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE)

-/* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */
+#define FANOTIFY_ERROR_EVENTS  (FAN_FS_ERROR)
+
+/* Events that can only be reported to groups that support FID mode */

Let's not do that.
How about the opposite:

/* Events that can be reported with event->fd */
#define FANOTIFY_FD_EVENTS (FANOTIFY_PATH_EVENTS | FANOTIFY_PERM_EVENTS)

        /*
         * 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_FD_EVENTS) &&
            (!fid_mode || mark_type == FAN_MARK_MOUNT))

Will do.

Thanks,

-- 
Gabriel Krisman Bertazi

On Wed, Jun 30, 2021 at 8:43 PM Gabriel Krisman Bertazi
[off-list ref] wrote:

Amir Goldstein [off-list ref] writes:

quoted

quoted

+       fee->fsid = fee->mark->connector->fsid;
+
+       fsnotify_get_mark(fee->mark);
+
+       /*
+        * Error reporting needs to happen in atomic context.  If this
+        * inode's file handler is more than we initially predicted,
+        * there is nothing better we can do than report the error with
+        * a bad FH.
+        */
+       fh_len = fanotify_encode_fh_len(inode);
+       if (WARN_ON(fh_len > fee->max_fh_len))

WARN_ON() is not acceptable for things that can logically happen
if you think this is important you could use pr_warn_ratelimited()
like we do in fanotify_encode_fh(),
but since fs-monitor will observe the lack of FID anyway, I think
there is little point in reporting this to kmsg.

Hi Amir,

Thanks for all the review so far.

Consider that fh_len > max_fh_len can happen only if the filesystem
requires a longer handler for the failed inode than it requires for the
root inode.  Looking at the FH types, I don't think this would be
possible to happen currently, but this WARN_ON is trying to catch future
problems.

Don't get confused by FH types. A filesystem is not obliged to
return a uniform and single handle_type nor uniform handle_size.
Overlayfs FH size depends on the FH size of the fs in the layer
the file is on, which may be different for different files.

Notice this would not be a fs-monitor misuse of the uAPI,  but an actual
kernel bug. The FH size we predicted when allocating the static error
slot is not large enough for at least one FH of this filesystem.  So I
think a WARN_ON or a pr_warn is desired.  I will change it to a
pr_warn_ratelimited as you suggested.

It would be a very minor kernel bug.
It would mean that there is a filesystem that matters in practice
for error reporting with different sizes of FH which you did not
take into account.

There is also a solution, but I think it is an overkill -
If you follow my suggestion to recreate the mark error event
on dequeue, you can update max_fh_len and re-created the
next event with larger buffer size.

In that case, admin will only see a few  pr_warn_ratelimited()
messages until fs-monitors reads the overflowed error event.

Also, I think it would be wise to use the NULL-FID convention
with different handle_types to report the different cases of:
- Failed encode (FILEID_INVALID)
- No inode (FILEID_ROOT)

Also, better use FANOTIFY_INLINE_FH_LEN as mimimum
for error event buffer size.

Thanks,
Amir.