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: <20211019000015.1666608-1-krisman@collabora.com> <20211019000015.1666608-12-krisman@collabora.com>
In-Reply-To: <20211019000015.1666608-12-krisman@collabora.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 19 Oct 2021 08:34:41 +0300
Message-ID: <CAOQ4uxhyW1O6tEKsEvnyV9ovmM=On0KWoe9Oq-HZou7MdR0GaA@mail.gmail.com>
Subject: Re: [PATCH v8 11/32] fsnotify: Protect fsnotify_handle_inode_event
 from no-inode events
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     Jan Kara <jack@suse.com>, "Darrick J. Wong" <djwong@kernel.org>,
        Theodore Tso <tytso@mit.edu>,
        Dave Chinner <david@fromorbit.com>,
        David Howells <dhowells@redhat.com>,
        Khazhismel Kumykov <khazhy@google.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Ext4 <linux-ext4@vger.kernel.org>,
        Linux API <linux-api@vger.kernel.org>, kernel@collabora.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> FAN_FS_ERROR allows events without inodes - i.e. for file system-wide
> errors.  Even though fsnotify_handle_inode_event is not currently used
> by fanotify, this patch protects this path to handle this new case.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  fs/notify/fsnotify.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index fde3a1115a17..47f931fb571c 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
>         if (WARN_ON_ONCE(!ops->handle_inode_event))
>                 return 0;
>
> +       if (!inode)
> +               return 0;
> +

Sigh.. the plot thickens.
There are three in-tree backends that implement the ->handle_inode_event()
interface.

inotify and dnotify can take NULL inode and the above will make the CREATE
events on kernfs vanish, so we cannot do that.
Sorry for not noticing this earlier when I asked for this change.

nfsd_file_fsnotify_handle_event() can most certainly not take NULL inode,
but nfsd does not watch for CREATE events.

I think what we need to do is (Jan please correct me if you think otherwise):
1. Document the handle_inode_event() interface that either inode or dir
    must be non-NULL
2. WARN_ON_ONCE(!inode && !dir) instead of just (!inode) above
3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event()
    in nfsd_file_fsnotify_handle_event()

Apologies, Gabriel, for having to cleanup our mess ;-)

Thanks,
Amir.