Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210629191035.681913-1-krisman@collabora.com>
 <20210629191035.681913-13-krisman@collabora.com> <CAOQ4uxiUYAwj561=ap_Hq6AwRdAdZFY1yQ99Y9_ahsd82-qFug@mail.gmail.com>
 <87v95vgsey.fsf@collabora.com>
In-Reply-To: <87v95vgsey.fsf@collabora.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 1 Jul 2021 09:37:26 +0300
Message-ID: <CAOQ4uxjPHQdsPcKY-WL-WE7tWGzaTmF3gzDEhCEPxW2-U3zjcg@mail.gmail.com>
Subject: Re: [PATCH v3 12/15] fanotify: Introduce FAN_FS_ERROR event
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     "Darrick J. Wong" <djwong@kernel.org>,
        Theodore Tso <tytso@mit.edu>,
        Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.com>,
        David Howells <dhowells@redhat.com>,
        Khazhismel Kumykov <khazhy@google.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Ext4 <linux-ext4@vger.kernel.org>, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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