diff mbox series

[4/5] syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events

Message ID 20181116065119.6912-5-amir73il@gmail.com
State Changes Requested
Headers show
Series [1/5] syscalls/fanotify01: check events also on mountpoint mark | expand

Commit Message

Amir Goldstein Nov. 16, 2018, 6:51 a.m. UTC 
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify03.c     | 25 ++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

Cyril Hrubis Nov. 21, 2018, 1:14 p.m. UTC | #1 
Hi!
> -static void setup_mark(unsigned int n)
> +static int setup_mark(unsigned int n)
>  {
>  	struct tcase *tc = &tcases[n];
>  	struct fanotify_mark_type *mark = &tc->mark;
> @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
>  	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
>  			  FAN_ACCESS_PERM | FAN_OPEN_PERM,
>  			  AT_FDCWD, fname) < 0) {
> -		if (errno == EINVAL) {
> +		if (errno == EINVAL && support_perm_events &&
> +		    mark->flag == FAN_MARK_FILESYSTEM) {
> +			tst_res(TCONF,
> +				"FAN_MARK_FILESYSTEM not supported in kernel?");
> +			return -1;
> +		} else if (errno == EINVAL) {
>  			tst_brk(TCONF | TERRNO,
>  				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
>  				"configured in kernel?");
> @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
>  				"AT_FDCWD, %s) failed.",
>  				fd_notify, mark->name, fname);
>  		}
> +	} else {
> +		/*
> +		 * To distigouish between perm event not supported and
> +		 * filesystem mark not supported.
> +		 */
> +		support_perm_events = 1;

I'm a bit puzzled here, so we attempted to cache if perm_events are
supported here?

I guess that we depend on the order of the tcases[] array here, which is
not very nice.

Also it does not have to be in else branch, if we get EINVAL the first
time we call fanotify_mark() tst_brk() is called, which exits the test,
so if we ever get to this point in the program, we did at least one
successful mark.

>  	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
> +	return 0;
>  }
>  
>  static void test_fanotify(unsigned int n)
> @@ -165,7 +182,9 @@ static void test_fanotify(unsigned int n)
>  	int tst_count;
>  	int ret, len = 0, i = 0, test_num = 0;
>  
> -	setup_mark(n);
> +	if (setup_mark(n) != 0)
> +		return;
> +
>  	run_child();
>  
>  	tst_count = 0;
> -- 
> 2.17.1
>
Amir Goldstein Nov. 21, 2018, 4:36 p.m. UTC | #2 
On Wed, Nov 21, 2018 at 3:16 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > -static void setup_mark(unsigned int n)
> > +static int setup_mark(unsigned int n)
> >  {
> >       struct tcase *tc = &tcases[n];
> >       struct fanotify_mark_type *mark = &tc->mark;
> > @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
> >       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> >                         FAN_ACCESS_PERM | FAN_OPEN_PERM,
> >                         AT_FDCWD, fname) < 0) {
> > -             if (errno == EINVAL) {
> > +             if (errno == EINVAL && support_perm_events &&
> > +                 mark->flag == FAN_MARK_FILESYSTEM) {
> > +                     tst_res(TCONF,
> > +                             "FAN_MARK_FILESYSTEM not supported in kernel?");
> > +                     return -1;
> > +             } else if (errno == EINVAL) {
> >                       tst_brk(TCONF | TERRNO,
> >                               "CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
> >                               "configured in kernel?");
> > @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
> >                               "AT_FDCWD, %s) failed.",
> >                               fd_notify, mark->name, fname);
> >               }
> > +     } else {
> > +             /*
> > +              * To distigouish between perm event not supported and
> > +              * filesystem mark not supported.
> > +              */
> > +             support_perm_events = 1;
>
> I'm a bit puzzled here, so we attempted to cache if perm_events are
> supported here?
>

Yes.

> I guess that we depend on the order of the tcases[] array here, which is
> not very nice.
>

Indeed. It is a simplification. All test cases are permission events,
so the order of failure types is not likely to change, but I could add a
comment about the order near test cases definition.
Would you rather that I did an explicit test for permission support at setup()
time?

> Also it does not have to be in else branch, if we get EINVAL the first
> time we call fanotify_mark() tst_brk() is called, which exits the test,
> so if we ever get to this point in the program, we did at least one
> successful mark.

Right. I'll change that.

Thanks,
Amir.
Amir Goldstein Nov. 21, 2018, 5:10 p.m. UTC | #3 
On Wed, Nov 21, 2018 at 6:36 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Nov 21, 2018 at 3:16 PM Cyril Hrubis <chrubis@suse.cz> wrote:
> >
> > Hi!
> > > -static void setup_mark(unsigned int n)
> > > +static int setup_mark(unsigned int n)
> > >  {
> > >       struct tcase *tc = &tcases[n];
> > >       struct fanotify_mark_type *mark = &tc->mark;
> > > @@ -144,7 +149,12 @@ static void setup_mark(unsigned int n)
> > >       if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
> > >                         FAN_ACCESS_PERM | FAN_OPEN_PERM,
> > >                         AT_FDCWD, fname) < 0) {
> > > -             if (errno == EINVAL) {
> > > +             if (errno == EINVAL && support_perm_events &&
> > > +                 mark->flag == FAN_MARK_FILESYSTEM) {
> > > +                     tst_res(TCONF,
> > > +                             "FAN_MARK_FILESYSTEM not supported in kernel?");
> > > +                     return -1;
> > > +             } else if (errno == EINVAL) {
> > >                       tst_brk(TCONF | TERRNO,
> > >                               "CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
> > >                               "configured in kernel?");
> > > @@ -155,9 +165,16 @@ static void setup_mark(unsigned int n)
> > >                               "AT_FDCWD, %s) failed.",
> > >                               fd_notify, mark->name, fname);
> > >               }
> > > +     } else {
> > > +             /*
> > > +              * To distigouish between perm event not supported and
> > > +              * filesystem mark not supported.
> > > +              */
> > > +             support_perm_events = 1;
> >
> > I'm a bit puzzled here, so we attempted to cache if perm_events are
> > supported here?
> >
>
> Yes.
>
> > I guess that we depend on the order of the tcases[] array here, which is
> > not very nice.
> >
>
> Indeed. It is a simplification. All test cases are permission events,
> so the order of failure types is not likely to change, but I could add a
> comment about the order near test cases definition.
> Would you rather that I did an explicit test for permission support at setup()
> time?
>

Nevermind. this discussion is moot because no permission events support
breaks out of the test. I'll drop this logic altogether.

It will have to make a comeback though with Mark's patches to add tests for
FAN_OPEN_EXEC_PERM. (need to distinguish between to support for
FAN_MARK_FILESYSTEM and no support for FAN_OPEN_EXEC_PERM)
so please state your preference for the best way to handle this issue.

Thanks,
Amir.
Cyril Hrubis Nov. 22, 2018, 4:03 p.m. UTC | #4 
Hi!
> > I guess that we depend on the order of the tcases[] array here, which is
> > not very nice.
> >
> 
> Indeed. It is a simplification. All test cases are permission events,
> so the order of failure types is not likely to change, but I could add a
> comment about the order near test cases definition.
> Would you rather that I did an explicit test for permission support at setup()
> time?

Either one would be fine.
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
index 19daf57ef..5c105ed32 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify03.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -46,6 +46,7 @@  static unsigned long long event_set[EVENT_MAX];
 static unsigned int event_resp[EVENT_MAX];
 
 static char event_buf[EVENT_BUF_LEN];
+static int support_perm_events;
 
 static struct tcase {
 	const char *tname;
@@ -59,6 +60,10 @@  static struct tcase {
 		"mount mark permission events",
 		INIT_FANOTIFY_MARK_TYPE(MOUNT),
 	},
+	{
+		"filesystem mark permission events",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+	},
 };
 
 static void generate_events(void)
@@ -134,7 +139,7 @@  static void check_child(void)
 		tst_res(TFAIL, "child %s", tst_strstatus(child_ret));
 }
 
-static void setup_mark(unsigned int n)
+static int setup_mark(unsigned int n)
 {
 	struct tcase *tc = &tcases[n];
 	struct fanotify_mark_type *mark = &tc->mark;
@@ -144,7 +149,12 @@  static void setup_mark(unsigned int n)
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag,
 			  FAN_ACCESS_PERM | FAN_OPEN_PERM,
 			  AT_FDCWD, fname) < 0) {
-		if (errno == EINVAL) {
+		if (errno == EINVAL && support_perm_events &&
+		    mark->flag == FAN_MARK_FILESYSTEM) {
+			tst_res(TCONF,
+				"FAN_MARK_FILESYSTEM not supported in kernel?");
+			return -1;
+		} else if (errno == EINVAL) {
 			tst_brk(TCONF | TERRNO,
 				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
 				"configured in kernel?");
@@ -155,9 +165,16 @@  static void setup_mark(unsigned int n)
 				"AT_FDCWD, %s) failed.",
 				fd_notify, mark->name, fname);
 		}
+	} else {
+		/*
+		 * To distigouish between perm event not supported and
+		 * filesystem mark not supported.
+		 */
+		support_perm_events = 1;
 	}
 
 	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+	return 0;
 }
 
 static void test_fanotify(unsigned int n)
@@ -165,7 +182,9 @@  static void test_fanotify(unsigned int n)
 	int tst_count;
 	int ret, len = 0, i = 0, test_num = 0;
 
-	setup_mark(n);
+	if (setup_mark(n) != 0)
+		return;
+
 	run_child();
 
 	tst_count = 0;