syscalls/fanotify: test fanotify_init new flags FAN_EVENT_INFO_TID

Message ID 20180925084344.4397-1-nixiaoming@huawei.com
State Changes Requested
Headers show
Series
  • syscalls/fanotify: test fanotify_init new flags FAN_EVENT_INFO_TID
Related show

Commit Message

nixiaoming Sept. 25, 2018, 8:43 a.m.
fanotify_info_tid:
	test fanotify_init with FAN_EVENT_INFO_TID
	data->pid is ID(pid)  of the thread that caused the event
fanotify_info_tgid:
	test fanotify_init without FAN_EVENT_INFO_TID
	data->pid is ID(tgid) of the process that caused the event

Signed-off-by: nixiaoming <nixiaoming@huawei.com>
---
 testcases/kernel/syscalls/fanotify/Makefile        |   3 +-
 .../kernel/syscalls/fanotify/fanotify_info_tgid.c  | 200 ++++++++++++++++++++
 .../kernel/syscalls/fanotify/fanotify_info_tid.c   | 204 +++++++++++++++++++++
 3 files changed, 406 insertions(+), 1 deletion(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tid.c

Comments

Amir Goldstein Sept. 25, 2018, 2:37 p.m. | #1
On Tue, Sep 25, 2018 at 12:31 PM nixiaoming <nixiaoming@huawei.com> wrote:
>
> fanotify_info_tid:
>         test fanotify_init with FAN_EVENT_INFO_TID
>         data->pid is ID(pid)  of the thread that caused the event
> fanotify_info_tgid:
>         test fanotify_init without FAN_EVENT_INFO_TID
>         data->pid is ID(tgid) of the process that caused the event

Nixiaoming,

These two tests are identical except for probably a single if condition.
They should be implemented as a single test with two test cases.
See test fanotify10 I posted as an example.

Please name your new test fanotify11. In LTP tests are usually numbered
and not "named".

>
> Signed-off-by: nixiaoming <nixiaoming@huawei.com>
> ---
>  testcases/kernel/syscalls/fanotify/Makefile        |   3 +-
>  .../kernel/syscalls/fanotify/fanotify_info_tgid.c  | 200 ++++++++++++++++++++
>  .../kernel/syscalls/fanotify/fanotify_info_tid.c   | 204 +++++++++++++++++++++

Please see my same patch for required changes for new tests to:
 runtest/syscalls
 testcases/kernel/syscalls/fanotify/.gitignore

>  3 files changed, 406 insertions(+), 1 deletion(-)
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tid.c
>
> diff --git a/testcases/kernel/syscalls/fanotify/Makefile b/testcases/kernel/syscalls/fanotify/Makefile
> index bb58878..d85c755 100644
> --- a/testcases/kernel/syscalls/fanotify/Makefile
> +++ b/testcases/kernel/syscalls/fanotify/Makefile
> @@ -17,7 +17,8 @@
>  #
>
>  top_srcdir             ?= ../../../..
> -
> +fanotify_info_tid: CFLAGS+=-pthread
> +fanotify_info_tgid: CFLAGS+=-pthread
>  include $(top_srcdir)/include/mk/testcases.mk
>
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
> new file mode 100644
> index 0000000..3b10d79
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright (c) 2013 SUSE.  All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + * Further, this software is distributed without any warranty that it is
> + * free of the rightful claim of any third person regarding infringement
> + * or the like.  Any license provided herein, whether implied or
> + * otherwise, applies only to this software file.  Patent licenses, if
> + * any, provided herein do not apply to combinations of this program with
> + * other software, or any other product whatsoever.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Started by nixiaoming <nixiaoming@huawei.com>
> + *
> + * DESCRIPTION
> + *     After fanotify_init without FAN_EVENT_INFO_TID,
> + *     check whether the program can only show tgid
> + *     in the multithreaded program triggered the event..
> + *
> + * This is a regression test for commit 55c5b2ed10331c3b22:

This is not a regression test. Regression means there was a bug and
a fix and this test verifies a fix.
This test checks for correctness of event->pid value before and after
the new FAN_EVENT_INFO_TID feature.

> + *
> + *     fanotify: support reporting thread id instead of process id
> + */
> +#define _GNU_SOURCE
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <pthread.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <linux/limits.h>
> +#include "tst_test.h"
> +#include "fanotify.h"
> +
> +#if defined(HAVE_SYS_FANOTIFY_H)
> +#include <sys/fanotify.h>
> +
> +
> +#define BUF_SIZE 256
> +#define gettid() syscall(SYS_gettid)
> +
> +static char fname[BUF_SIZE];
> +int fd_notify;
> +
> +static int tid;
> +static int tgid;
> +static char tid_file[BUF_SIZE];
> +
> +void *thread_create_file(void *arg LTP_ATTRIBUTE_UNUSED)
> +{
> +       int fd;
> +
> +       tid = gettid();
> +       snprintf(tid_file, sizeof(tid_file), "%s/test_tid_%d", fname, tid);
> +       sleep(1);

Why sleep? Can't call thread_create_file() after setting the mark?

> +       fd = SAFE_OPEN(tid_file, O_WRONLY|O_CREAT, 0600);
> +       tst_res(TINFO, "open file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> +       SAFE_WRITE(1, fd, tid_file, sizeof(tid_file));
> +       tst_res(TINFO, "write file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> +       SAFE_CLOSE(fd);
> +       tst_res(TINFO, "close file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);


SAFE_FILE_PRINTF(tid_file, "1");
instead of all of the above?
and all those TINFO seems too much to me.

> +       SAFE_UNLINK(tid_file);

You don't really need to unlink the file - it is created in a temp dir
and even if 2nd iteration uses same tid, events would still be created.

> +       tst_res(TINFO, "unlink file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> +       sleep(1);
> +       pthread_exit(0);
> +}
> +
> +static const char* mask2str(uint64_t mask)
> +{
> +       static char buffer[10];
> +       int offset = 0;
> +
> +       if (mask & FAN_ACCESS)
> +               buffer[offset++] = 'R';
> +       if (mask & FAN_ONDIR)
> +               buffer[offset++] = 'D';
> +       if (mask & FAN_OPEN)
> +               buffer[offset++] = 'O';
> +       if (mask & FAN_CLOSE_WRITE || mask & FAN_CLOSE_NOWRITE)
> +               buffer[offset++] = 'C';
> +       if (mask & FAN_MODIFY || mask & FAN_CLOSE_WRITE)
> +               buffer[offset++] = 'W';
> +       buffer[offset] = '\0';
> +
> +       return buffer;
> +}
> +
> +
> +static void show_data(const struct fanotify_event_metadata *data)
> +{
> +       static char printbuf[100];
> +       static char pathname[PATH_MAX];
> +       struct stat st;
> +       int len;
> +
> +       snprintf(printbuf, sizeof(printbuf), "/proc/self/fd/%i", data->fd);
> +       len = readlink(printbuf, pathname, sizeof(pathname));
> +       if (len < 0) {
> +               /* fall back to the device/inode */
> +               if (fstat(data->fd, &st) < 0) {
> +                       perror("stat");
> +                       exit(1);
> +               }
> +               snprintf(pathname, sizeof(pathname), "device %i:%i inode %ld\n",
> +                               major(st.st_dev), minor(st.st_dev), st.st_ino);
> +       } else {
> +               pathname[len] = '\0';
> +       }
> +       tst_res(TINFO, "envnt: %i %s %s\n", data->pid, mask2str(data->mask),
> +                       pathname);
> +}

I find all this a bit TMI (too much information) that is not relevant
for this test.

> +
> +void test01(void)
> +{
> +       int res;
> +       int ret;
> +       char buffer[4096] = {0};

You should not define a 4k buffer on the stack but its enough for you
to use a single
struct fanotify_event_metadata event;
as the buffer for this test.

> +       struct fanotify_event_metadata *data;
> +
> +       ret = fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);

You don't really need all these event masks. FAN_MODIFY should do.

> +       if (ret != 0) {
> +               tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN |FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
> +                               "failed, ret=%d\n", fd_notify, fname, ret);
> +       }
> +       tst_res(TINFO, "add notify mark\n");
> +       res = SAFE_READ(0, fd_notify, buffer, 4096);
> +       data = (struct fanotify_event_metadata *) buffer;
> +       while (FAN_EVENT_OK(data, res)) {
> +               show_data(data);
> +               if (data->pid == tid)
> +                       tst_res(TFAIL, "data->pid == envent task thread id %u\n", tid);
> +               else if (data->pid == tgid)
> +                       tst_res(TPASS, "data->pid %u != envent task thread id %u\n", data->pid, tid);
> +               else
> +                       tst_res(TINFO, "Environmental interference....\n");

That should definitely result in TFAIL.

> +               close(data->fd);
if (data->fd != FAN_NOFD)
    SAFE_CLOSE(data->fd);

> +               data = FAN_EVENT_NEXT(data, res);
> +       }
> +       fflush(stdout);
> +
> +       /* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> +       ret = fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
> +       if (ret != 0) {
> +               tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
> +                               "failed, ret=%d\n", fd_notify, fname, ret);
> +       }
> +       tst_res(TINFO, "remove notify mark\n");
> +}
> +
> +static void setup(void)
> +{
> +       pthread_t p_id;
> +
> +       tgid = getpid();
> +       sprintf(fname, "tfile_%d", tgid);
> +       mkdir(fname, 0755);
> +
> +       fd_notify = SAFE_FANOTIFY_INIT(0, 0);

To simplify using test cases, better move fanotify_init into the test itself
and keep SAFE_CLOSE(fd_notify) both at end of test and in cleanup().
In each test cases your test would use different fanotify_init flags and
you should specify FAN_CLASS_NOTIF even though is it 0.

> +       if (fd_notify < 0)
> +               tst_brk(TCONF, "no support\n");

This check is moot because safe_fanotify_init already exists the test
in this case, but for this reason you cannot use SAFE_FANOTIFY_INIT
in your test. You need to use fanotify_init() and check for errno == EINVAL
when you use flag FAN_EVENT_INFO_TID, so you can report TCONF
for feature not supported in kernel - see my change to fanotify01 to
test new init flag FAN_UNPRIVILEGED on branch fanotify_unpriv
in my LTP tree.

> +       pthread_create(&p_id, NULL, thread_create_file, NULL);
> +}
> +

Please look into using SAFE_PTHREAD_CREATE
(and SAFE_PTHREAD_JOIN in cleanup()).

Thanks,
Amir.
Amir Goldstein Sept. 25, 2018, 6:20 p.m. | #2
[CC: Jan Kara]

On Tue, Sep 25, 2018 at 5:37 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Sep 25, 2018 at 12:31 PM nixiaoming <nixiaoming@huawei.com> wrote:
> >
> > fanotify_info_tid:
> >         test fanotify_init with FAN_EVENT_INFO_TID
> >         data->pid is ID(pid)  of the thread that caused the event
> > fanotify_info_tgid:
> >         test fanotify_init without FAN_EVENT_INFO_TID
> >         data->pid is ID(tgid) of the process that caused the event
>
> Nixiaoming,
>
> These two tests are identical except for probably a single if condition.
> They should be implemented as a single test with two test cases.
> See test fanotify10 I posted as an example.
>

Nixiaoming,

Two more things to add:
1. Test coverage would be more complete IMO if test would also generate an event
from a forked child. You have an example of SAFE_FORK and
'forks_child' in fanotify03.
You could execute the thread and the child process sequentially,
reading each event
before executing the other to simplify the test.
2. Since FAN_EVENT_INFO_TID is not yet merged and the API not even
reviewed by Jan,
it should be made clear when posting a test that this is an RFC and
not a request for merge
yet. What you could do though is post the test with a single test case
that only checks
tgid and request to merge it. Later post a patch that adds a test case with the
FAN_EVENT_INFO_TID variant after FAN_EVENT_INFO_TID code is merged upstream
(before that you can post RFC test of course).

Hope I didn't make the process too complicated to understand...
Thanks,
Amir.

> Please name your new test fanotify11. In LTP tests are usually numbered
> and not "named".
>
> >
> > Signed-off-by: nixiaoming <nixiaoming@huawei.com>
> > ---
> >  testcases/kernel/syscalls/fanotify/Makefile        |   3 +-
> >  .../kernel/syscalls/fanotify/fanotify_info_tgid.c  | 200 ++++++++++++++++++++
> >  .../kernel/syscalls/fanotify/fanotify_info_tid.c   | 204 +++++++++++++++++++++
>
> Please see my same patch for required changes for new tests to:
>  runtest/syscalls
>  testcases/kernel/syscalls/fanotify/.gitignore
>
> >  3 files changed, 406 insertions(+), 1 deletion(-)
> >  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
> >  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify_info_tid.c
> >
> > diff --git a/testcases/kernel/syscalls/fanotify/Makefile b/testcases/kernel/syscalls/fanotify/Makefile
> > index bb58878..d85c755 100644
> > --- a/testcases/kernel/syscalls/fanotify/Makefile
> > +++ b/testcases/kernel/syscalls/fanotify/Makefile
> > @@ -17,7 +17,8 @@
> >  #
> >
> >  top_srcdir             ?= ../../../..
> > -
> > +fanotify_info_tid: CFLAGS+=-pthread
> > +fanotify_info_tgid: CFLAGS+=-pthread
> >  include $(top_srcdir)/include/mk/testcases.mk
> >
> >  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
> > new file mode 100644
> > index 0000000..3b10d79
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Copyright (c) 2013 SUSE.  All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it would be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + * Further, this software is distributed without any warranty that it is
> > + * free of the rightful claim of any third person regarding infringement
> > + * or the like.  Any license provided herein, whether implied or
> > + * otherwise, applies only to this software file.  Patent licenses, if
> > + * any, provided herein do not apply to combinations of this program with
> > + * other software, or any other product whatsoever.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write the Free Software Foundation, Inc.,
> > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > + *
> > + * Started by nixiaoming <nixiaoming@huawei.com>
> > + *
> > + * DESCRIPTION
> > + *     After fanotify_init without FAN_EVENT_INFO_TID,
> > + *     check whether the program can only show tgid
> > + *     in the multithreaded program triggered the event..
> > + *
> > + * This is a regression test for commit 55c5b2ed10331c3b22:
>
> This is not a regression test. Regression means there was a bug and
> a fix and this test verifies a fix.
> This test checks for correctness of event->pid value before and after
> the new FAN_EVENT_INFO_TID feature.
>
> > + *
> > + *     fanotify: support reporting thread id instead of process id
> > + */
> > +#define _GNU_SOURCE
> > +#include "config.h"
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <sys/syscall.h>
> > +#include <pthread.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <linux/limits.h>
> > +#include "tst_test.h"
> > +#include "fanotify.h"
> > +
> > +#if defined(HAVE_SYS_FANOTIFY_H)
> > +#include <sys/fanotify.h>
> > +
> > +
> > +#define BUF_SIZE 256
> > +#define gettid() syscall(SYS_gettid)
> > +
> > +static char fname[BUF_SIZE];
> > +int fd_notify;
> > +
> > +static int tid;
> > +static int tgid;
> > +static char tid_file[BUF_SIZE];
> > +
> > +void *thread_create_file(void *arg LTP_ATTRIBUTE_UNUSED)
> > +{
> > +       int fd;
> > +
> > +       tid = gettid();
> > +       snprintf(tid_file, sizeof(tid_file), "%s/test_tid_%d", fname, tid);
> > +       sleep(1);
>
> Why sleep? Can't call thread_create_file() after setting the mark?
>
> > +       fd = SAFE_OPEN(tid_file, O_WRONLY|O_CREAT, 0600);
> > +       tst_res(TINFO, "open file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> > +       SAFE_WRITE(1, fd, tid_file, sizeof(tid_file));
> > +       tst_res(TINFO, "write file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> > +       SAFE_CLOSE(fd);
> > +       tst_res(TINFO, "close file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
>
>
> SAFE_FILE_PRINTF(tid_file, "1");
> instead of all of the above?
> and all those TINFO seems too much to me.
>
> > +       SAFE_UNLINK(tid_file);
>
> You don't really need to unlink the file - it is created in a temp dir
> and even if 2nd iteration uses same tid, events would still be created.
>
> > +       tst_res(TINFO, "unlink file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
> > +       sleep(1);
> > +       pthread_exit(0);
> > +}
> > +
> > +static const char* mask2str(uint64_t mask)
> > +{
> > +       static char buffer[10];
> > +       int offset = 0;
> > +
> > +       if (mask & FAN_ACCESS)
> > +               buffer[offset++] = 'R';
> > +       if (mask & FAN_ONDIR)
> > +               buffer[offset++] = 'D';
> > +       if (mask & FAN_OPEN)
> > +               buffer[offset++] = 'O';
> > +       if (mask & FAN_CLOSE_WRITE || mask & FAN_CLOSE_NOWRITE)
> > +               buffer[offset++] = 'C';
> > +       if (mask & FAN_MODIFY || mask & FAN_CLOSE_WRITE)
> > +               buffer[offset++] = 'W';
> > +       buffer[offset] = '\0';
> > +
> > +       return buffer;
> > +}
> > +
> > +
> > +static void show_data(const struct fanotify_event_metadata *data)
> > +{
> > +       static char printbuf[100];
> > +       static char pathname[PATH_MAX];
> > +       struct stat st;
> > +       int len;
> > +
> > +       snprintf(printbuf, sizeof(printbuf), "/proc/self/fd/%i", data->fd);
> > +       len = readlink(printbuf, pathname, sizeof(pathname));
> > +       if (len < 0) {
> > +               /* fall back to the device/inode */
> > +               if (fstat(data->fd, &st) < 0) {
> > +                       perror("stat");
> > +                       exit(1);
> > +               }
> > +               snprintf(pathname, sizeof(pathname), "device %i:%i inode %ld\n",
> > +                               major(st.st_dev), minor(st.st_dev), st.st_ino);
> > +       } else {
> > +               pathname[len] = '\0';
> > +       }
> > +       tst_res(TINFO, "envnt: %i %s %s\n", data->pid, mask2str(data->mask),
> > +                       pathname);
> > +}
>
> I find all this a bit TMI (too much information) that is not relevant
> for this test.
>
> > +
> > +void test01(void)
> > +{
> > +       int res;
> > +       int ret;
> > +       char buffer[4096] = {0};
>
> You should not define a 4k buffer on the stack but its enough for you
> to use a single
> struct fanotify_event_metadata event;
> as the buffer for this test.
>
> > +       struct fanotify_event_metadata *data;
> > +
> > +       ret = fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
>
> You don't really need all these event masks. FAN_MODIFY should do.
>
> > +       if (ret != 0) {
> > +               tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN |FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
> > +                               "failed, ret=%d\n", fd_notify, fname, ret);
> > +       }
> > +       tst_res(TINFO, "add notify mark\n");
> > +       res = SAFE_READ(0, fd_notify, buffer, 4096);
> > +       data = (struct fanotify_event_metadata *) buffer;
> > +       while (FAN_EVENT_OK(data, res)) {
> > +               show_data(data);
> > +               if (data->pid == tid)
> > +                       tst_res(TFAIL, "data->pid == envent task thread id %u\n", tid);
> > +               else if (data->pid == tgid)
> > +                       tst_res(TPASS, "data->pid %u != envent task thread id %u\n", data->pid, tid);
> > +               else
> > +                       tst_res(TINFO, "Environmental interference....\n");
>
> That should definitely result in TFAIL.
>
> > +               close(data->fd);
> if (data->fd != FAN_NOFD)
>     SAFE_CLOSE(data->fd);
>
> > +               data = FAN_EVENT_NEXT(data, res);
> > +       }
> > +       fflush(stdout);
> > +
> > +       /* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> > +       ret = fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
> > +       if (ret != 0) {
> > +               tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
> > +                               "failed, ret=%d\n", fd_notify, fname, ret);
> > +       }
> > +       tst_res(TINFO, "remove notify mark\n");
> > +}
> > +
> > +static void setup(void)
> > +{
> > +       pthread_t p_id;
> > +
> > +       tgid = getpid();
> > +       sprintf(fname, "tfile_%d", tgid);
> > +       mkdir(fname, 0755);
> > +
> > +       fd_notify = SAFE_FANOTIFY_INIT(0, 0);
>
> To simplify using test cases, better move fanotify_init into the test itself
> and keep SAFE_CLOSE(fd_notify) both at end of test and in cleanup().
> In each test cases your test would use different fanotify_init flags and
> you should specify FAN_CLASS_NOTIF even though is it 0.
>
> > +       if (fd_notify < 0)
> > +               tst_brk(TCONF, "no support\n");
>
> This check is moot because safe_fanotify_init already exists the test
> in this case, but for this reason you cannot use SAFE_FANOTIFY_INIT
> in your test. You need to use fanotify_init() and check for errno == EINVAL
> when you use flag FAN_EVENT_INFO_TID, so you can report TCONF
> for feature not supported in kernel - see my change to fanotify01 to
> test new init flag FAN_UNPRIVILEGED on branch fanotify_unpriv
> in my LTP tree.
>
> > +       pthread_create(&p_id, NULL, thread_create_file, NULL);
> > +}
> > +
>
> Please look into using SAFE_PTHREAD_CREATE
> (and SAFE_PTHREAD_JOIN in cleanup()).
>
> Thanks,
> Amir.
nixiaoming Sept. 26, 2018, 1:07 a.m. | #3
On Wed, Sep 26, 2018 2:20 AM Amir Goldstein <amir73il@gmail.com> wrote:
>[CC: Jan Kara]
>
>On Tue, Sep 25, 2018 at 5:37 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Tue, Sep 25, 2018 at 12:31 PM nixiaoming <nixiaoming@huawei.com> wrote:
>> >
>> > fanotify_info_tid:
>> >         test fanotify_init with FAN_EVENT_INFO_TID
>> >         data->pid is ID(pid)  of the thread that caused the event
>> > fanotify_info_tgid:
>> >         test fanotify_init without FAN_EVENT_INFO_TID
>> >         data->pid is ID(tgid) of the process that caused the event
>>
>> Nixiaoming,
>>
>> These two tests are identical except for probably a single if condition.
>> They should be implemented as a single test with two test cases.
>> See test fanotify10 I posted as an example.
>>
>
>Nixiaoming,
>
>Two more things to add:
>1. Test coverage would be more complete IMO if test would also generate an event
>from a forked child. You have an example of SAFE_FORK and
>'forks_child' in fanotify03.
>You could execute the thread and the child process sequentially,
>reading each event
>before executing the other to simplify the test.
>2. Since FAN_EVENT_INFO_TID is not yet merged and the API not even
>reviewed by Jan,
>it should be made clear when posting a test that this is an RFC and
>not a request for merge
>yet. What you could do though is post the test with a single test case
>that only checks
>tgid and request to merge it. Later post a patch that adds a test case with the
>FAN_EVENT_INFO_TID variant after FAN_EVENT_INFO_TID code is merged upstream
>(before that you can post RFC test of course).
>
>Hope I didn't make the process too complicated to understand...
>Thanks,
>Amir.
>

thanks for your guidance, 
I will refer to your comments and rewrite the test cases as soon as possible.

thanks

Patch

diff --git a/testcases/kernel/syscalls/fanotify/Makefile b/testcases/kernel/syscalls/fanotify/Makefile
index bb58878..d85c755 100644
--- a/testcases/kernel/syscalls/fanotify/Makefile
+++ b/testcases/kernel/syscalls/fanotify/Makefile
@@ -17,7 +17,8 @@ 
 #
 
 top_srcdir		?= ../../../..
-
+fanotify_info_tid: CFLAGS+=-pthread
+fanotify_info_tgid: CFLAGS+=-pthread
 include $(top_srcdir)/include/mk/testcases.mk
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
new file mode 100644
index 0000000..3b10d79
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify_info_tgid.c
@@ -0,0 +1,200 @@ 
+/*
+ * Copyright (c) 2013 SUSE.  All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * Further, this software is distributed without any warranty that it is
+ * free of the rightful claim of any third person regarding infringement
+ * or the like.  Any license provided herein, whether implied or
+ * otherwise, applies only to this software file.  Patent licenses, if
+ * any, provided herein do not apply to combinations of this program with
+ * other software, or any other product whatsoever.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Started by nixiaoming <nixiaoming@huawei.com>
+ *
+ * DESCRIPTION
+ *     After fanotify_init without FAN_EVENT_INFO_TID,
+ *     check whether the program can only show tgid 
+ *     in the multithreaded program triggered the event..
+ *
+ * This is a regression test for commit 55c5b2ed10331c3b22:
+ *
+ *     fanotify: support reporting thread id instead of process id
+ */
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <pthread.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <linux/limits.h>
+#include "tst_test.h"
+#include "fanotify.h"
+
+#if defined(HAVE_SYS_FANOTIFY_H)
+#include <sys/fanotify.h>
+
+
+#define BUF_SIZE 256
+#define gettid() syscall(SYS_gettid)
+
+static char fname[BUF_SIZE];
+int fd_notify;
+
+static int tid;
+static int tgid;
+static char tid_file[BUF_SIZE];
+
+void *thread_create_file(void *arg LTP_ATTRIBUTE_UNUSED)
+{
+	int fd;
+
+	tid = gettid();
+	snprintf(tid_file, sizeof(tid_file), "%s/test_tid_%d", fname, tid);
+	sleep(1);
+	fd = SAFE_OPEN(tid_file, O_WRONLY|O_CREAT, 0600);
+	tst_res(TINFO, "open file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_WRITE(1, fd, tid_file, sizeof(tid_file));
+	tst_res(TINFO, "write file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_CLOSE(fd);
+	tst_res(TINFO, "close file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_UNLINK(tid_file);
+	tst_res(TINFO, "unlink file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	sleep(1);
+	pthread_exit(0);
+}
+
+static const char* mask2str(uint64_t mask)
+{
+	static char buffer[10];
+	int offset = 0;
+
+	if (mask & FAN_ACCESS)
+		buffer[offset++] = 'R';
+	if (mask & FAN_ONDIR)
+		buffer[offset++] = 'D';
+	if (mask & FAN_OPEN)
+		buffer[offset++] = 'O';
+	if (mask & FAN_CLOSE_WRITE || mask & FAN_CLOSE_NOWRITE)
+		buffer[offset++] = 'C';
+	if (mask & FAN_MODIFY || mask & FAN_CLOSE_WRITE)
+		buffer[offset++] = 'W';
+	buffer[offset] = '\0';
+
+	return buffer;
+}
+
+
+static void show_data(const struct fanotify_event_metadata *data)
+{
+	static char printbuf[100];
+	static char pathname[PATH_MAX];
+	struct stat st;
+	int len;
+
+	snprintf(printbuf, sizeof(printbuf), "/proc/self/fd/%i", data->fd);
+	len = readlink(printbuf, pathname, sizeof(pathname));
+	if (len < 0) {
+		/* fall back to the device/inode */
+		if (fstat(data->fd, &st) < 0) {
+			perror("stat");
+			exit(1);
+		}
+		snprintf(pathname, sizeof(pathname), "device %i:%i inode %ld\n",
+				major(st.st_dev), minor(st.st_dev), st.st_ino);
+	} else {
+		pathname[len] = '\0';
+	}
+	tst_res(TINFO, "envnt: %i %s %s\n", data->pid, mask2str(data->mask),
+			pathname);
+}
+
+void test01(void)
+{
+	int res;
+	int ret;
+	char buffer[4096] = {0};
+	struct fanotify_event_metadata *data;
+
+	ret = fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
+	if (ret != 0) {
+		tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN |FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
+				"failed, ret=%d\n", fd_notify, fname, ret);
+	}
+	tst_res(TINFO, "add notify mark\n");
+	res = SAFE_READ(0, fd_notify, buffer, 4096);
+	data = (struct fanotify_event_metadata *) buffer;
+	while (FAN_EVENT_OK(data, res)) {
+		show_data(data);
+		if (data->pid == tid)
+			tst_res(TFAIL, "data->pid == envent task thread id %u\n", tid);
+		else if (data->pid == tgid)
+			tst_res(TPASS, "data->pid %u != envent task thread id %u\n", data->pid, tid);
+		else
+			tst_res(TINFO, "Environmental interference....\n");
+		close(data->fd);
+		data = FAN_EVENT_NEXT(data, res);
+	}
+	fflush(stdout);
+
+	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
+	ret = fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
+	if (ret != 0) {
+		tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
+				"failed, ret=%d\n", fd_notify, fname, ret);
+	}
+	tst_res(TINFO, "remove notify mark\n");
+}
+
+static void setup(void)
+{
+	pthread_t p_id;
+
+	tgid = getpid();
+	sprintf(fname, "tfile_%d", tgid);
+	mkdir(fname, 0755);
+
+	fd_notify = SAFE_FANOTIFY_INIT(0, 0);
+	if (fd_notify < 0)
+		tst_brk(TCONF, "no support\n");
+	pthread_create(&p_id, NULL, thread_create_file, NULL);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+	rmdir(fname);
+}
+
+static struct tst_test test = {
+	.test_all = test01,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+	.needs_root = 1
+};
+
+#else
+TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
diff --git a/testcases/kernel/syscalls/fanotify/fanotify_info_tid.c b/testcases/kernel/syscalls/fanotify/fanotify_info_tid.c
new file mode 100644
index 0000000..4baf714
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify_info_tid.c
@@ -0,0 +1,204 @@ 
+/*
+ * Copyright (c) 2013 SUSE.  All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * Further, this software is distributed without any warranty that it is
+ * free of the rightful claim of any third person regarding infringement
+ * or the like.  Any license provided herein, whether implied or
+ * otherwise, applies only to this software file.  Patent licenses, if
+ * any, provided herein do not apply to combinations of this program with
+ * other software, or any other product whatsoever.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Started by nixiaoming <nixiaoming@huawei.com>
+ *
+ * DESCRIPTION
+ *     After fanotify_init adds flags FAN_EVENT_INFO_TID,
+ *     check whether the program can accurately identify which thread id
+ *     in the multithreaded program triggered the event..
+ *
+ * This is a regression test for commit 55c5b2ed10331c3b22:
+ *
+ *     fanotify: support reporting thread id instead of process id
+ */
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <pthread.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <linux/limits.h>
+#include "tst_test.h"
+#include "fanotify.h"
+
+#if defined(HAVE_SYS_FANOTIFY_H)
+#include <sys/fanotify.h>
+#ifdef FAN_EVENT_INFO_TID
+
+#define BUF_SIZE 256
+#define gettid() syscall(SYS_gettid)
+
+static char fname[BUF_SIZE];
+int fd_notify;
+
+static int tid;
+static int tgid;
+static char tid_file[BUF_SIZE];
+
+void *thread_create_file(void *arg LTP_ATTRIBUTE_UNUSED)
+{
+	int fd;
+
+	tid = gettid();
+	snprintf(tid_file, sizeof(tid_file), "%s/test_tid_%d", fname, tid);
+	sleep(1);
+	fd = SAFE_OPEN(tid_file, O_WRONLY|O_CREAT, 0600);
+	tst_res(TINFO, "open file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_WRITE(1, fd, tid_file, sizeof(tid_file));
+	tst_res(TINFO, "write file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_CLOSE(fd);
+	tst_res(TINFO, "close file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	SAFE_UNLINK(tid_file);
+	tst_res(TINFO, "unlink file=%s. fd=%d, tid=%i\n", tid_file, fd, tid);
+	sleep(1);
+	pthread_exit(0);
+}
+
+static const char* mask2str(uint64_t mask)
+{
+	static char buffer[10];
+	int offset = 0;
+
+	if (mask & FAN_ACCESS)
+		buffer[offset++] = 'R';
+	if (mask & FAN_ONDIR)
+		buffer[offset++] = 'D';
+	if (mask & FAN_OPEN)
+		buffer[offset++] = 'O';
+	if (mask & FAN_CLOSE_WRITE || mask & FAN_CLOSE_NOWRITE)
+		buffer[offset++] = 'C';
+	if (mask & FAN_MODIFY || mask & FAN_CLOSE_WRITE)
+		buffer[offset++] = 'W';
+	buffer[offset] = '\0';
+
+	return buffer;
+}
+
+
+static void show_data(const struct fanotify_event_metadata *data)
+{
+	static char printbuf[100];
+	static char pathname[PATH_MAX];
+	struct stat st;
+	int len;
+
+	snprintf(printbuf, sizeof(printbuf), "/proc/self/fd/%i", data->fd);
+	len = readlink(printbuf, pathname, sizeof(pathname));
+	if (len < 0) {
+		/* fall back to the device/inode */
+		if (fstat(data->fd, &st) < 0) {
+			perror("stat");
+			exit(1);
+		}
+		snprintf(pathname, sizeof(pathname), "device %i:%i inode %ld\n",
+				major(st.st_dev), minor(st.st_dev), st.st_ino);
+	} else {
+		pathname[len] = '\0';
+	}
+	tst_res(TINFO, "envnt: %i %s %s\n", data->pid, mask2str(data->mask),
+			pathname);
+}
+
+void test01(void)
+{
+	int res;
+	int ret;
+	char buffer[4096] = {0};
+	struct fanotify_event_metadata *data;
+
+	ret = fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
+	if (ret != 0) {
+		tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_ADD, FAN_CLOSE | FAN_OPEN |FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
+				"failed, ret=%d\n", fd_notify, fname, ret);
+	}
+	tst_res(TINFO, "add notify mark\n");
+	res = SAFE_READ(0, fd_notify, buffer, 4096);
+	data = (struct fanotify_event_metadata *) buffer;
+	while (FAN_EVENT_OK(data, res)) {
+		show_data(data);
+		if (data->pid == tid)
+			tst_res(TPASS, "data->pid == envent task thread id %u\n", tid);
+		else if (data->pid == tgid)
+			tst_res(TFAIL, "data->pid %u != envent task thread id %u\n", data->pid, tid);
+		else
+			tst_res(TINFO, "Environmental interference....\n");
+		close(data->fd);
+		data = FAN_EVENT_NEXT(data, res);
+	}
+	fflush(stdout);
+
+	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
+	ret = fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, fname);
+	if (ret != 0) {
+		tst_brk(TBROK, "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_CLOSE | FAN_OPEN | FAN_EVENT_ON_CHILD, AT_FDCWD, %s) "
+				"failed, ret=%d\n", fd_notify, fname, ret);
+	}
+	tst_res(TINFO, "remove notify mark\n");
+}
+
+static void setup(void)
+{
+	pthread_t p_id;
+
+	tgid = getpid();
+	sprintf(fname, "tfile_%d", tgid);
+	mkdir(fname, 0755);
+
+	fd_notify = SAFE_FANOTIFY_INIT(FAN_EVENT_INFO_TID, 0);
+	if (fd_notify < 0)
+		tst_brk(TCONF, "no support FAN_EVENT_INFO_TID\n");
+	pthread_create(&p_id, NULL, thread_create_file, NULL);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+	rmdir(fname);
+}
+
+static struct tst_test test = {
+	.test_all = test01,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+	.needs_root = 1
+};
+
+#else
+TST_TEST_TCONF("system doesn't support fanotify_init flags: FAN_EVENT_INFO_TID");
+#endif /* ifdef FAN_EVENT_INFO_TID */
+
+#else
+TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif