[1/5] syscalls/fanotify01: check events also on mountpoint mark
diff mbox series

Message ID 20181116065119.6912-2-amir73il@gmail.com
State Accepted
Headers show
Series
  • [1/5] syscalls/fanotify01: check events also on mountpoint mark
Related show

Commit Message

Amir Goldstein Nov. 16, 2018, 6:51 a.m. UTC
Add index to test and repeat it for mark types inode and mountpoint.
Move fanotify_init() into the test to simplify setup/cleanup.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify01.c     | 83 ++++++++++++-------
 1 file changed, 51 insertions(+), 32 deletions(-)

Comments

Cyril Hrubis Nov. 21, 2018, 12:01 p.m. UTC | #1
Hi!
>  	/* This event should be ignored */
> @@ -181,13 +200,13 @@ void test01(void)
>  
>  	/* Now remove open and close from ignored mask */
>  	if (fanotify_mark(fd_notify,
> -			  FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
> +			  FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
>  			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
>  		tst_brk(TBROK | TERRNO,
> -			"fanotify_mark (%d, FAN_MARK_REMOVE | "
> -			"FAN_MARK_IGNORED_MASK, FAN_OPEN | "
> -			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
> -			fname);
> +			"fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
> +			"FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
> +			"AT_FDCWD, %s) failed", fd_notify,
> +			mark->name, fname);
>  	}
>  
>  	SAFE_CLOSE(fd);
> @@ -283,12 +302,13 @@ pass:
>  
>  	}
>  	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> -	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
> -			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
> +	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
> +			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
> +			  AT_FDCWD, fname) < 0) {
>  		tst_brk(TBROK | TERRNO,
> -			"fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
> +			"fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
>  			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
> -			"failed", fd_notify, fname);
> +			"failed", fd_notify, mark->name, fname);
>  	}
>  }
>  
> @@ -296,8 +316,6 @@ static void setup(void)
>  {
>  	sprintf(fname, "tfile_%d", getpid());
>  	SAFE_FILE_PRINTF(fname, "1");
> -
> -	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);

I've added SAFE_CLOSE(fd_notify) here so that we do not use all
available file descriptors with large enough -i parameter and pushed.

I do wonder what is the reason to move the fanotify_init() from setup
the the test_fanotify(), is there a problem with recycling the same fd
for different tests?
Amir Goldstein Nov. 21, 2018, 4:18 p.m. UTC | #2
On Wed, Nov 21, 2018 at 2:02 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> >       /* This event should be ignored */
> > @@ -181,13 +200,13 @@ void test01(void)
> >
> >       /* Now remove open and close from ignored mask */
> >       if (fanotify_mark(fd_notify,
> > -                       FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
> > +                       FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
> >                         FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
> >               tst_brk(TBROK | TERRNO,
> > -                     "fanotify_mark (%d, FAN_MARK_REMOVE | "
> > -                     "FAN_MARK_IGNORED_MASK, FAN_OPEN | "
> > -                     "FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
> > -                     fname);
> > +                     "fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
> > +                     "FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
> > +                     "AT_FDCWD, %s) failed", fd_notify,
> > +                     mark->name, fname);
> >       }
> >
> >       SAFE_CLOSE(fd);
> > @@ -283,12 +302,13 @@ pass:
> >
> >       }
> >       /* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
> > -     if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
> > -                         FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
> > +     if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
> > +                       FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
> > +                       AT_FDCWD, fname) < 0) {
> >               tst_brk(TBROK | TERRNO,
> > -                     "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
> > +                     "fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
> >                       "FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
> > -                     "failed", fd_notify, fname);
> > +                     "failed", fd_notify, mark->name, fname);
> >       }
> >  }
> >
> > @@ -296,8 +316,6 @@ static void setup(void)
> >  {
> >       sprintf(fname, "tfile_%d", getpid());
> >       SAFE_FILE_PRINTF(fname, "1");
> > -
> > -     fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
>
> I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> available file descriptors with large enough -i parameter and pushed.
>
> I do wonder what is the reason to move the fanotify_init() from setup
> the the test_fanotify(), is there a problem with recycling the same fd
> for different tests?
>

Closing fd_notify is an easier and better cleanup between test cases.
Otherwise we would need to FAN_MARK_FLUSH all the individual marks
that we added.

Thanks,
Amir.
Cyril Hrubis Nov. 22, 2018, 4:06 p.m. UTC | #3
Hi!
> > I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> > available file descriptors with large enough -i parameter and pushed.
> >
> > I do wonder what is the reason to move the fanotify_init() from setup
> > the the test_fanotify(), is there a problem with recycling the same fd
> > for different tests?
> >
> 
> Closing fd_notify is an easier and better cleanup between test cases.
> Otherwise we would need to FAN_MARK_FLUSH all the individual marks
> that we added.

Wouldn't that cover more of kernel codepaths? If we reused the fd and
cleared all the marks we would test that the API for clearing the marks
works as expected. Or should we write a separate test for that?
Amir Goldstein Nov. 22, 2018, 4:57 p.m. UTC | #4
On Thu, Nov 22, 2018 at 6:07 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > I've added SAFE_CLOSE(fd_notify) here so that we do not use all
> > > available file descriptors with large enough -i parameter and pushed.
> > >
> > > I do wonder what is the reason to move the fanotify_init() from setup
> > > the the test_fanotify(), is there a problem with recycling the same fd
> > > for different tests?
> > >
> >
> > Closing fd_notify is an easier and better cleanup between test cases.
> > Otherwise we would need to FAN_MARK_FLUSH all the individual marks
> > that we added.
>
> Wouldn't that cover more of kernel codepaths? If we reused the fd and
> cleared all the marks we would test that the API for clearing the marks
> works as expected. Or should we write a separate test for that?

I see that fanotify04 exercises FAN_MARK_FLUSH.
Besides it is probably the less common case (??).
Most of the time I imagine that a monitoring program sets a mark
and watches events until it closes the fd.

Thanks,
Amir.

Patch
diff mbox series

diff --git a/testcases/kernel/syscalls/fanotify/fanotify01.c b/testcases/kernel/syscalls/fanotify/fanotify01.c
index e1210134b..5dfb67b61 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify01.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify01.c
@@ -32,26 +32,46 @@ 
 #define BUF_SIZE 256
 #define TST_TOTAL 12
 
+static struct tcase {
+	const char *tname;
+	struct fanotify_mark_type mark;
+} tcases[] = {
+	{
+		"inode mark events",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+	},
+	{
+		"mount mark events",
+		INIT_FANOTIFY_MARK_TYPE(MOUNT),
+	},
+};
+
 static char fname[BUF_SIZE];
 static char buf[BUF_SIZE];
-static int fd, fd_notify;
+static int fd_notify;
 
 static unsigned long long event_set[EVENT_MAX];
 
 static char event_buf[EVENT_BUF_LEN];
 
-void test01(void)
+static void test_fanotify(unsigned int n)
 {
-	int ret, len, i = 0, test_num = 0;
-
+	struct tcase *tc = &tcases[n];
+	struct fanotify_mark_type *mark = &tc->mark;
+	int fd, ret, len, i = 0, test_num = 0;
 	int tst_count = 0;
 
-	if (fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_ACCESS | FAN_MODIFY |
-			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
+	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+
+	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
+
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
+			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
+			  AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | %s | "
 			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-			"failed", fd_notify, fname);
+			"failed", fd_notify, mark->name, fname);
 	}
 
 	/*
@@ -101,12 +121,12 @@  void test01(void)
 
 	/* Ignore access events */
 	if (fanotify_mark(fd_notify,
-			  FAN_MARK_ADD | FAN_MARK_IGNORED_MASK,
+			  FAN_MARK_ADD | mark->flag | FAN_MARK_IGNORED_MASK,
 			  FAN_ACCESS, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD | "
-			"FAN_MARK_IGNORED_MASK, FAN_ACCESS, "
-			"AT_FDCWD, %s) failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | %s | "
+			"FAN_MARK_IGNORED_MASK, FAN_ACCESS, AT_FDCWD, %s) "
+			"failed", fd_notify, mark->name, fname);
 	}
 
 	fd = SAFE_OPEN(fname, O_RDWR);
@@ -150,15 +170,14 @@  void test01(void)
 	 * Now ignore open & close events regardless of file
 	 * modifications
 	 */
-	if (fanotify_mark(fd_notify,
-			    FAN_MARK_ADD | FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
-			    FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag |
+			  FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
+			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_ADD | "
-			"FAN_MARK_IGNORED_MASK | "
-			"FAN_MARK_IGNORED_SURV_MODIFY, FAN_OPEN | "
-			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-			fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | %s | "
+			"FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY, "
+			"FAN_OPEN | FAN_CLOSE, AT_FDCWD, %s) failed",
+			fd_notify, mark->name, fname);
 	}
 
 	/* This event should be ignored */
@@ -181,13 +200,13 @@  void test01(void)
 
 	/* Now remove open and close from ignored mask */
 	if (fanotify_mark(fd_notify,
-			  FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
+			  FAN_MARK_REMOVE | mark->flag | FAN_MARK_IGNORED_MASK,
 			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_REMOVE | "
-			"FAN_MARK_IGNORED_MASK, FAN_OPEN | "
-			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-			fname);
+			"fanotify_mark (%d, FAN_MARK_REMOVE | %s | "
+			"FAN_MARK_IGNORED_MASK, FAN_OPEN | FAN_CLOSE, "
+			"AT_FDCWD, %s) failed", fd_notify,
+			mark->name, fname);
 	}
 
 	SAFE_CLOSE(fd);
@@ -283,12 +302,13 @@  pass:
 
 	}
 	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
-	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
-			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
+	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | mark->flag,
+			  FAN_ACCESS | FAN_MODIFY | FAN_CLOSE | FAN_OPEN,
+			  AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
+			"fanotify_mark (%d, FAN_MARK_REMOVE | %s, FAN_ACCESS | "
 			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-			"failed", fd_notify, fname);
+			"failed", fd_notify, mark->name, fname);
 	}
 }
 
@@ -296,8 +316,6 @@  static void setup(void)
 {
 	sprintf(fname, "tfile_%d", getpid());
 	SAFE_FILE_PRINTF(fname, "1");
-
-	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
 }
 
 static void cleanup(void)
@@ -307,7 +325,8 @@  static void cleanup(void)
 }
 
 static struct tst_test test = {
-	.test_all = test01,
+	.test = test_fanotify,
+	.tcnt = ARRAY_SIZE(tcases),
 	.setup = setup,
 	.cleanup = cleanup,
 	.needs_tmpdir = 1,