From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-yw1-f65.google.com ([209.85.161.65]:46568 "EHLO
        mail-yw1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727789AbeKOB6m (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 14 Nov 2018 20:58:42 -0500
MIME-Version: 1.0
References: <cover.1541639254.git.mbobrowski@mbobrowski.org>
 <20181113175346.GC12023@quack2.suse.cz> <CAOQ4uxiSN7wGBejSGAYyqQTbe64SbWirWAwFq+FV8TsVOD16sA@mail.gmail.com>
 <CAOQ4uxjgToX6RZ+oGZgx_sfRL1ET1gvUWwDntGvN4h5-B6On9w@mail.gmail.com> <20181114120247.GA16849@quack2.suse.cz>
In-Reply-To: <20181114120247.GA16849@quack2.suse.cz>
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 14 Nov 2018 17:54:42 +0200
Message-ID: <CAOQ4uxjzqR4kAM3rdOimTwFr8n0VeHfGirpJg+ecR9paaoyzTw@mail.gmail.com>
Subject: Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC
 and FAN_OPEN_EXEC_PERM
To: Jan Kara <jack@suse.cz>
Cc: Matthew Bobrowski <mbobrowski@mbobrowski.org>,
        linux-api@vger.kernel.org,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Steve Grubb <sgrubb@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Wed, Nov 14, 2018 at 2:02 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 14-11-18 05:43:25, Amir Goldstein wrote:
> > On Tue, Nov 13, 2018 at 8:01 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > > LTP tests for this feature are on my 'fanotify-exec' branch here:
> > > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files
> > > > > that contains the test cases are provided below:
> > > > >
> > > > > syscalls/fanotify03: test cases have been updated to cover
> > > > >                      FAN_OPEN_EXEC_PERM events
> > > > > syscalls/fanotify12: newly introduced LTP test file to cover
> > > > >                      FAN_OPEN_EXEC events
> > > >
> > > > I have been wondering for a while why the testcases passed when ignore mask
> > > > hasn't been properly treated in fanotify_group_event_mask() but then I
> > > > realized that the generic code will not even call to fanotify if ignore
> > > > masks result in clearing the event.
> > >
> > > So does that means we have missing test coverage?
> > >
> >
> > This is covered by test case #3 of Matthew's proposed LTP test.
> > https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76
>
> This testcase does not catch the bug we had in fanotify_group_event_mask()
> because the masking by mark->mask already hides the fact that we failed to
> apply the ignore mask.
>
> What does catch this kind of bug (tested) is a testcase (admittedly
> somewhat silly) like this:
>
> {
>         "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC",
>         INIT_FANOTIFY_MARK_TYPE(INODE),
>         FAN_OPEN | FAN_OPEN_EXEC,
>         FAN_OPEN_EXEC,
>         2,
>         {FAN_OPEN, FAN_OPEN}
> },
>

Yah. that is simple to add.
Matthew please add it to your test.

> A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore
> FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug
> we'd get FAN_OPEN | FAN_OPEN_EXEC.
>
> But creating such test would be slightly more involved. But probably it is
> worth it. Matthew?

Not a problem to add a test case to fanotify10 which checks combination
of different mark type with ignore mask.
Matthew, you should have no problem to extend fanotify10 by making the
mask and ignore mask and result mask variables of the test case.
Please make that change based on fanotify_sb branch, which extends
fanotify10 to filesystem mark test cases.

Thanks,
Amir.