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.