* [LTP] [PATCH v2 0/1] fix fanotify anonymous pipe testcases @ 2024-03-12 12:08 Mete Durlu 2024-03-12 12:08 ` [LTP] [PATCH v2 1/1] fanotify14: fix " Mete Durlu 0 siblings, 1 reply; 6+ messages in thread From: Mete Durlu @ 2024-03-12 12:08 UTC (permalink / raw) To: ltp * v1 -> v2: - accept both EINVAL and EACCES when SElinux is configured. Checking SElinux policies would be too much effort. - replace TST_EXP_FD_OR_FAIL with TST_EXP_FAIL since fanotify_mark() returns 0 on success and -1 with errno on failure. * v0 -> v1: - sent wrong patch Mete Durlu (1): fanotify14: fix anonymous pipe testcases .../kernel/syscalls/fanotify/fanotify14.c | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) -- 2.44.0 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH v2 1/1] fanotify14: fix anonymous pipe testcases 2024-03-12 12:08 [LTP] [PATCH v2 0/1] fix fanotify anonymous pipe testcases Mete Durlu @ 2024-03-12 12:08 ` Mete Durlu 2024-03-12 15:00 ` Amir Goldstein 0 siblings, 1 reply; 6+ messages in thread From: Mete Durlu @ 2024-03-12 12:08 UTC (permalink / raw) To: ltp When SElinux is in enforcing state and SEpolicies disallow anonymous pipe usage with fanotify_mark(), related fanotify14 testcases fail with EACCES instead of EINVAL. Accept both errnos when SElinux is in enforcing state to correctly evaluate test results. Replace TST_EXP_FD_OR_FAIL with TST_EXP_FAIL when testing fanotify_mark() as it returns -1 on failure and 0 on success not a file descriptor. Signed-off-by: Mete Durlu <meted@linux.ibm.com> --- .../kernel/syscalls/fanotify/fanotify14.c | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c index d02d81495..52c327dff 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify14.c +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c @@ -27,12 +27,14 @@ #define _GNU_SOURCE #include "tst_test.h" #include <errno.h> +#include <stdlib.h> #ifdef HAVE_SYS_FANOTIFY_H #include "fanotify.h" #define MNTPOINT "mntpoint" #define FILE1 MNTPOINT"/file1" +#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce" /* * List of inode events that are only available when notification group is @@ -240,6 +242,19 @@ static struct test_case_t { }, }; +static int is_selinux_enforcing(void) +{ + char res; + int fd; + + fd = open(SELINUX_STATUS_PATH, O_RDONLY); + if (fd <= 0) + return 0; + SAFE_READ(1, fd, &res, 1); + SAFE_CLOSE(fd); + return atoi(&res); +} + static void do_test(unsigned int number) { struct test_case_t *tc = &test_cases[number]; @@ -275,17 +290,28 @@ static void do_test(unsigned int number) /* Set mark on non-dir only when expecting error ENOTDIR */ const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; int dirfd = AT_FDCWD; + int se_enforcing = 0; if (tc->pfd) { dirfd = tc->pfd[0]; path = NULL; + se_enforcing = is_selinux_enforcing(); } tst_res(TINFO, "Testing %s with %s", tc->mark.desc, tc->mask.desc); - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, - tc->mask.flags, dirfd, path), - tc->expected_errno); + + if (tc->pfd && se_enforcing) { + const int exp_errs[] = {tc->expected_errno, EACCES}; + + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + exp_errs); + } else { + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + tc->expected_errno); + } /* * ENOTDIR are errors for events/flags not allowed on a non-dir inode. -- 2.44.0 -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH v2 1/1] fanotify14: fix anonymous pipe testcases 2024-03-12 12:08 ` [LTP] [PATCH v2 1/1] fanotify14: fix " Mete Durlu @ 2024-03-12 15:00 ` Amir Goldstein 2024-03-13 7:26 ` Petr Vorel 0 siblings, 1 reply; 6+ messages in thread From: Amir Goldstein @ 2024-03-12 15:00 UTC (permalink / raw) To: Mete Durlu; +Cc: Jan Kara, ltp On Tue, Mar 12, 2024 at 2:09 PM Mete Durlu <meted@linux.ibm.com> wrote: > > When SElinux is in enforcing state and SEpolicies disallow anonymous > pipe usage with fanotify_mark(), related fanotify14 testcases fail with > EACCES instead of EINVAL. Accept both errnos when SElinux is in > enforcing state to correctly evaluate test results. > > Replace TST_EXP_FD_OR_FAIL with TST_EXP_FAIL when testing > fanotify_mark() as it returns -1 on failure and 0 on success not a file > descriptor. > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > --- > .../kernel/syscalls/fanotify/fanotify14.c | 32 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > index d02d81495..52c327dff 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c > @@ -27,12 +27,14 @@ > #define _GNU_SOURCE > #include "tst_test.h" > #include <errno.h> > +#include <stdlib.h> > > #ifdef HAVE_SYS_FANOTIFY_H > #include "fanotify.h" > > #define MNTPOINT "mntpoint" > #define FILE1 MNTPOINT"/file1" > +#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce" > > /* > * List of inode events that are only available when notification group is > @@ -240,6 +242,19 @@ static struct test_case_t { > }, > }; > > +static int is_selinux_enforcing(void) > +{ > + char res; > + int fd; > + > + fd = open(SELINUX_STATUS_PATH, O_RDONLY); > + if (fd <= 0) > + return 0; > + SAFE_READ(1, fd, &res, 1); > + SAFE_CLOSE(fd); > + return atoi(&res); > +} > + > static void do_test(unsigned int number) > { > struct test_case_t *tc = &test_cases[number]; > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > /* Set mark on non-dir only when expecting error ENOTDIR */ > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > int dirfd = AT_FDCWD; > + int se_enforcing = 0; > > if (tc->pfd) { > dirfd = tc->pfd[0]; > path = NULL; > + se_enforcing = is_selinux_enforcing(); > } > > tst_res(TINFO, "Testing %s with %s", > tc->mark.desc, tc->mask.desc); > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > - tc->mask.flags, dirfd, path), > - tc->expected_errno); > + > + if (tc->pfd && se_enforcing) { > + const int exp_errs[] = {tc->expected_errno, EACCES}; > + > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + exp_errs); > + } else { > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + tc->expected_errno); > + } > This looks fine to me, but on second thought I am not sure how important it is to special case se_enforcing. We could probably always check for either error value. Let's see what Jan and Petr think. Thanks, Amir. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH v2 1/1] fanotify14: fix anonymous pipe testcases 2024-03-12 15:00 ` Amir Goldstein @ 2024-03-13 7:26 ` Petr Vorel 2024-03-13 16:47 ` Jan Kara 2024-03-14 8:34 ` Mete Durlu 0 siblings, 2 replies; 6+ messages in thread From: Petr Vorel @ 2024-03-13 7:26 UTC (permalink / raw) To: Amir Goldstein; +Cc: Mete Durlu, Jan Kara, ltp Hi all, ... > > static void do_test(unsigned int number) > > { > > struct test_case_t *tc = &test_cases[number]; > > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > > /* Set mark on non-dir only when expecting error ENOTDIR */ > > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > > int dirfd = AT_FDCWD; > > + int se_enforcing = 0; > > if (tc->pfd) { > > dirfd = tc->pfd[0]; > > path = NULL; > > + se_enforcing = is_selinux_enforcing(); nit: this check should be in the setup function to be done only once. (by default it's done twice, because we have 2 testcases with pfd, we support -iN parameter, thus it's actually 2*N.). I'll fix it before merge. > > } > > tst_res(TINFO, "Testing %s with %s", > > tc->mark.desc, tc->mask.desc); > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > - tc->mask.flags, dirfd, path), > > - tc->expected_errno); > > + > > + if (tc->pfd && se_enforcing) { > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > + > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + exp_errs); > > + } else { > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + tc->expected_errno); > > + } > This looks fine to me, but on second thought I am not sure how important > it is to special case se_enforcing. > We could probably always check for either error value. I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? With a diff below (I can change it before merge + I would do the following work to integrate this into the LTP C API): Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr diff --git testcases/kernel/syscalls/fanotify/fanotify14.c testcases/kernel/syscalls/fanotify/fanotify14.c index 52c327dff..89d59c8b2 100644 --- testcases/kernel/syscalls/fanotify/fanotify14.c +++ testcases/kernel/syscalls/fanotify/fanotify14.c @@ -49,6 +49,7 @@ static int pipes[2] = {-1, -1}; static int fanotify_fd; static int ignore_mark_unsupported; static int filesystem_mark_unsupported; +static int se_enforcing; static unsigned int supported_init_flags; struct test_case_flags_t { @@ -290,12 +291,10 @@ static void do_test(unsigned int number) /* Set mark on non-dir only when expecting error ENOTDIR */ const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; int dirfd = AT_FDCWD; - int se_enforcing = 0; if (tc->pfd) { dirfd = tc->pfd[0]; path = NULL; - se_enforcing = is_selinux_enforcing(); } tst_res(TINFO, "Testing %s with %s", @@ -360,6 +359,8 @@ static void do_setup(void) SAFE_FILE_PRINTF(FILE1, "0"); /* Create anonymous pipes to place marks on */ SAFE_PIPE2(pipes, O_CLOEXEC); + + se_enforcing = is_selinux_enforcing(); } static void do_cleanup(void) > Let's see what Jan and Petr think. > Thanks, > Amir. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH v2 1/1] fanotify14: fix anonymous pipe testcases 2024-03-13 7:26 ` Petr Vorel @ 2024-03-13 16:47 ` Jan Kara 2024-03-14 8:34 ` Mete Durlu 1 sibling, 0 replies; 6+ messages in thread From: Jan Kara @ 2024-03-13 16:47 UTC (permalink / raw) To: Petr Vorel; +Cc: Mete Durlu, Jan Kara, ltp On Wed 13-03-24 08:26:23, Petr Vorel wrote: > Hi all, > ... > > > > static void do_test(unsigned int number) > > > { > > > struct test_case_t *tc = &test_cases[number]; > > > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > > > /* Set mark on non-dir only when expecting error ENOTDIR */ > > > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > > > int dirfd = AT_FDCWD; > > > + int se_enforcing = 0; > > > > if (tc->pfd) { > > > dirfd = tc->pfd[0]; > > > path = NULL; > > > + se_enforcing = is_selinux_enforcing(); > nit: this check should be in the setup function to be done only once. > (by default it's done twice, because we have 2 testcases with pfd, we support > -iN parameter, thus it's actually 2*N.). I'll fix it before merge. > > > } > > > > tst_res(TINFO, "Testing %s with %s", > > > tc->mark.desc, tc->mask.desc); > > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > - tc->mask.flags, dirfd, path), > > > - tc->expected_errno); > > > + > > > + if (tc->pfd && se_enforcing) { > > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > > + > > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > + tc->mask.flags, dirfd, path), > > > + exp_errs); > > > + } else { > > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > + tc->mask.flags, dirfd, path), > > > + tc->expected_errno); > > > + } > > > > This looks fine to me, but on second thought I am not sure how important > > it is to special case se_enforcing. > > We could probably always check for either error value. > > I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? > > With a diff below (I can change it before merge + I would do the following work > to integrate this into the LTP C API): > Reviewed-by: Petr Vorel <pvorel@suse.cz> Yes, looks fine to me as well. I don't feel strongly whether we should accept EACCESS unconditionally or only with SELinux. I suspect eventually we might need to accept it unconditionally as there may be other security modules that would block addition of the mark. But let's see what the future brings. So feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH v2 1/1] fanotify14: fix anonymous pipe testcases 2024-03-13 7:26 ` Petr Vorel 2024-03-13 16:47 ` Jan Kara @ 2024-03-14 8:34 ` Mete Durlu 1 sibling, 0 replies; 6+ messages in thread From: Mete Durlu @ 2024-03-14 8:34 UTC (permalink / raw) To: Petr Vorel, Amir Goldstein; +Cc: Jan Kara, ltp On 3/13/24 08:26, Petr Vorel wrote: Hi, thanks for the review. I can send a v3 with the suggested changes if that will make things easier. Just let me know. >>> if (tc->pfd) { >>> dirfd = tc->pfd[0]; >>> path = NULL; >>> + se_enforcing = is_selinux_enforcing(); > nit: this check should be in the setup function to be done only once. > (by default it's done twice, because we have 2 testcases with pfd, we support > -iN parameter, thus it's actually 2*N.). I'll fix it before merge. >>> } > Nice catch! I fully forgot that there was a setup function while I was trying to find the best TST_ macro to use. >>> tst_res(TINFO, "Testing %s with %s", >>> tc->mark.desc, tc->mask.desc); >>> - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> - tc->mask.flags, dirfd, path), >>> - tc->expected_errno); >>> + >>> + if (tc->pfd && se_enforcing) { >>> + const int exp_errs[] = {tc->expected_errno, EACCES}; >>> + >>> + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> + tc->mask.flags, dirfd, path), >>> + exp_errs); >>> + } else { >>> + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> + tc->mask.flags, dirfd, path), >>> + tc->expected_errno); >>> + } > > >> This looks fine to me, but on second thought I am not sure how important >> it is to special case se_enforcing. >> We could probably always check for either error value. > > I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? > > With a diff below (I can change it before merge + I would do the following work > to integrate this into the LTP C API): > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > > diff --git testcases/kernel/syscalls/fanotify/fanotify14.c testcases/kernel/syscalls/fanotify/fanotify14.c > index 52c327dff..89d59c8b2 100644 > --- testcases/kernel/syscalls/fanotify/fanotify14.c > +++ testcases/kernel/syscalls/fanotify/fanotify14.c > @@ -49,6 +49,7 @@ static int pipes[2] = {-1, -1}; > static int fanotify_fd; > static int ignore_mark_unsupported; > static int filesystem_mark_unsupported; > +static int se_enforcing; > static unsigned int supported_init_flags; > > struct test_case_flags_t { > @@ -290,12 +291,10 @@ static void do_test(unsigned int number) > /* Set mark on non-dir only when expecting error ENOTDIR */ > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > int dirfd = AT_FDCWD; > - int se_enforcing = 0; > > if (tc->pfd) { > dirfd = tc->pfd[0]; > path = NULL; > - se_enforcing = is_selinux_enforcing(); > } > > tst_res(TINFO, "Testing %s with %s", > @@ -360,6 +359,8 @@ static void do_setup(void) > SAFE_FILE_PRINTF(FILE1, "0"); > /* Create anonymous pipes to place marks on */ > SAFE_PIPE2(pipes, O_CLOEXEC); > + > + se_enforcing = is_selinux_enforcing(); > } > > static void do_cleanup(void) >> Let's see what Jan and Petr think. > >> Thanks, >> Amir. -- Mailing list info: https://lists.linux.it/listinfo/ltp ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-14 8:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-12 12:08 [LTP] [PATCH v2 0/1] fix fanotify anonymous pipe testcases Mete Durlu 2024-03-12 12:08 ` [LTP] [PATCH v2 1/1] fanotify14: fix " Mete Durlu 2024-03-12 15:00 ` Amir Goldstein 2024-03-13 7:26 ` Petr Vorel 2024-03-13 16:47 ` Jan Kara 2024-03-14 8:34 ` Mete Durlu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.