On Wed, Jun 30, 2021 at 8:43 PM Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx> wrote: > > Amir Goldstein <amir73il@xxxxxxxxx> writes: > > >> + 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.
