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: <20210802214645.2633028-1-krisman@collabora.com>
 <20210802214645.2633028-4-krisman@collabora.com> <CAOQ4uxjMfJM4FM4tWJWgjbK4a2K1hNJdEBRvwQTh9+5su2N0Tw@mail.gmail.com>
 <87fsvphksu.fsf@collabora.com>
In-Reply-To: <87fsvphksu.fsf@collabora.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 4 Aug 2021 08:39:55 +0300
Message-ID: <CAOQ4uxj_WwDPxZv0nr9+Hh+pim6+2onaBdFq_BR-qK=xz+8yUg@mail.gmail.com>
Subject: Re: [PATCH 3/7] syscalls/fanotify20: Validate incoming FID in FAN_FS_ERROR
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     LTP List <ltp@lists.linux.it>, Jan Kara <jack@suse.com>,
        Ext4 <linux-ext4@vger.kernel.org>,
        Khazhismel Kumykov <khazhy@google.com>, kernel@collabora.com,
        Matthew Bobrowski <repnop@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 4, 2021 at 7:54 AM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Tue, Aug 3, 2021 at 12:47 AM Gabriel Krisman Bertazi
> > <krisman@collabora.com> wrote:
> >>
> >> Verify the FID provided in the event.  If the testcase has a null inode,
> >> this is assumed to be a superblock error (i.e. null FH).
> >>
> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> >> ---
> >>  .../kernel/syscalls/fanotify/fanotify20.c     | 51 +++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> index fd5cfb8744f1..d8d788ae685f 100644
> >> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> >> @@ -40,6 +40,14 @@
> >>
> >>  #define FAN_EVENT_INFO_TYPE_ERROR      4
> >>
> >> +#ifndef FILEID_INVALID
> >> +#define        FILEID_INVALID          0xff
> >> +#endif
> >> +
> >> +#ifndef FILEID_INO32_GEN
> >> +#define FILEID_INO32_GEN       1
> >> +#endif
> >> +
> >>  struct fanotify_event_info_error {
> >>         struct fanotify_event_info_header hdr;
> >>         __s32 error;
> >> @@ -57,6 +65,9 @@ static const struct test_case {
> >>         char *name;
> >>         int error;
> >>         unsigned int error_count;
> >> +
> >> +       /* inode can be null for superblock errors */
> >> +       unsigned int *inode;
> >
> > Any reason not to use fanotify_fid_t * like fanotify16.c?
>
> No reason other than I didn't notice they existed. Sorry. I will get
> this fixed.

No problem. That's what review is for ;-)

BTW, unless anyone is specifically interested I don't think there
is a reason to re post the test patches before the submission request.
Certainly not for the small fixes that I requested.

I do request that you post a link to a branch with the fixed test
so that we can experiment with the kernel patches.

I've also CC'ed Matthew who may want to help with review of the test
and man page that you posted in the cover letter [1].

Thanks,
Amir.

[1] https://lore.kernel.org/linux-ext4/20210802214645.2633028-1-krisman@collabora.com/T/#m9cf637c6aca94e28390f61deac5a53afbc9e88ae