[LTP] [RFC 2/3] syscalls/fanotify03: included execve() to generate_events() to increase test coverage

[Amir Goldstein]

On Thu, Oct 25, 2018 at 9:39 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Wed, Oct 24, 2018 at 08:31:22AM +0300, Amir Goldstein wrote:
> > On Wed, Oct 24, 2018 at 6:28 AM Matthew Bobrowski
> > <mbobrowski@mbobrowski.org> wrote:
> > >
> > > * Created an executable helper program 'fanotify_child' so that can be
> > >   used within fanotify tests
> > >
> > > * Defined .resource_files so that additional test resources can be
> > >   copied across to the tmp working directory i.e. fanotify_child
> > >
> > > * Updated generate_events() so that it now includes a call to execve()
> > >   on fanotify_child. This is so that we can increase the overall test
> > >   coverage by generating more events on a watched object
> > >
> > > * Updated each tcase events[] to accommodate for the additional events
> > >   generated by execve()
> > >
> > > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> > > ---
> > >  testcases/kernel/syscalls/fanotify/.gitignore |  1 +
> > >  .../kernel/syscalls/fanotify/fanotify03.c     | 83 ++++++++++++-------
> > >  .../kernel/syscalls/fanotify/fanotify_child.c | 14 ++++
> > >  3 files changed, 66 insertions(+), 32 deletions(-)
> > >  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_child.c
> > >
> > > diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
> > > index c26f2bd27..af420b8b3 100644
> > > --- a/testcases/kernel/syscalls/fanotify/.gitignore
> > > +++ b/testcases/kernel/syscalls/fanotify/.gitignore
> > > @@ -8,3 +8,4 @@
> > >  /fanotify08
> > >  /fanotify09
> > >  /fanotify10
> > > +/fanotify_child
> > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > > index cca15aa00..f9418ee6b 100644
> > > --- a/testcases/kernel/syscalls/fanotify/fanotify03.c
> > > +++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
> > > @@ -35,6 +35,7 @@
> > >
> > >  #define BUF_SIZE 256
> > >  #define TST_TOTAL 3
> > > +#define TEST_APP "fanotify_child"
> > >
> > >  static char fname[BUF_SIZE];
> > >  static char buf[BUF_SIZE];
> > > @@ -60,28 +61,31 @@ static struct tcase {
> > >         {
> > >                 "inode mark permission events",
> > >                 INIT_FANOTIFY_MARK_TYPE(INODE),
> > > -               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 3,
> > >                 {
> > >                         {FAN_OPEN_PERM, FAN_ALLOW},
> > > -                       {FAN_ACCESS_PERM, FAN_DENY}
> > > +                       {FAN_ACCESS_PERM, FAN_DENY},
> > > +                       {FAN_OPEN_PERM, FAN_DENY}
> > >                 }
> > >         },
> > >         {
> > >                 "mount mark permission events",
> > >                 INIT_FANOTIFY_MARK_TYPE(MOUNT),
> > > -               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 3,
> > >                 {
> > >                         {FAN_OPEN_PERM, FAN_ALLOW},
> > > -                       {FAN_ACCESS_PERM, FAN_DENY}
> > > +                       {FAN_ACCESS_PERM, FAN_DENY},
> > > +                       {FAN_OPEN_PERM, FAN_DENY}
> > >                 }
> > >         },
> > >         {
> > >                 "filesystem mark permission events",
> > >                 INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
> > > -               FAN_OPEN_PERM | FAN_ACCESS_PERM, 2,
> > > +               FAN_OPEN_PERM | FAN_ACCESS_PERM, 3,
> > >                 {
> > >                         {FAN_OPEN_PERM, FAN_ALLOW},
> > > -                       {FAN_ACCESS_PERM, FAN_DENY}
> > > +                       {FAN_ACCESS_PERM, FAN_DENY},
> > > +                       {FAN_OPEN_PERM, FAN_DENY}
> > >                 }
> > >         }
> > >  };
> > > @@ -89,9 +93,10 @@ static struct tcase {
> > >  static void generate_events(void)
> > >  {
> > >         int fd;
> > > +       char *const argv[] = {TEST_APP, NULL};
> > >
> > >         /*
> > > -        * generate sequence of events
> > > +        * Generate sequence of events
> > >          */
> > >         if ((fd = open(fname, O_RDWR | O_CREAT, 0700)) == -1)
> > >                 exit(1);
> > > @@ -104,6 +109,9 @@ static void generate_events(void)
> > >
> > >         if (close(fd) == -1)
> > >                 exit(4);
> > > +
> > > +       if (execve(TEST_APP, argv, environ) != -1)
> > > +               exit(5);
> > >  }
> > >
> >
> > I am a bit puzzled.
> > Did you test this point in the series?
> >
> Yes, I did test it at this point.
>
> > If child is denied read access then it exits before execve(), so you
> > shouldn't be adding an extra OPEN event in event_set of exiting
> > test cases, which DENY ACCESS??
> >
> I don't know whether I'm understanding you correctly, but I do believe
> that it's needed based on the following facts. Within generate_events()
> the system logic flow is as follows:
>
> open(...)       == -1
> write(...)      == -1
> read(...)       != -1
> close(...)      == -1
> execve(...)     == -1
>
> Based on the sequence of system calls and comparison operators above, then
> if read access is denied (FAN_DENY) then it does not exit, as that is what
> is expected, so it falls through to the next set system calls. Based on
> that fact, an additional event {FAN_OPEN_PERM, FAN_DENY} is needed as this
> is the first event that is generated when the mask FAN_OPEN_EXEC_PERM
> hasn't been provided as a mask.
>

Right. I missed the fact that read failure is expected in generate_events
You really managed to extend the test coverage with minimal changes
to the test ;-)

Thanks,
Amir.