| Message ID | 20220203061222.625948-1-amir73il@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/inotify: New test for IN_DELETE regression | expand |
Hi Amir, thanks for this test. > Check that files cannot be opened after IN_DELETE was reported > on them. > This test is based on the reproducer provided by Ivan Delalande > for a regression reported in kernel v5.13: > https://lore.kernel.org/linux-fsdevel/YeNyzoDM5hP5LtGW@visor/ > Reported-by: Ivan Delalande <colona@arista.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > Hi Petr, > The fix for this regression was applied to stable kernels > 5.4.y, 5.10.y, 5.15.y, 5.16.y. > The mentioned git tag is only the upstream commit. > Feel free to add the stable git tags if you think they are needed. FYI we don't track stable git commits which are normally backported (vast majority of commits), we track only stable git specific fixes, i.e.: * c4a23c852e80 ("io_uring: Fix current->fs handling in io_sq_wq_submit_work()") "No upstream commit, this is a fix to a stable 5.4 specific backport." * cac68d12c531 ("io_uring: grab ->fs as part of async offload") [ Upstream commits 9392a27d88b9 and ff002b30181d ] Mentioned at note: https://github.com/linux-test-project/ltp/wiki/C-Test-API#138-test-tags The reason is that user can search simple backports himself. Kind regards, Petr
Hi Amir, Thanks for this test. > Check that files cannot be opened after IN_DELETE was reported > on them. > This test is based on the reproducer provided by Ivan Delalande > for a regression reported in kernel v5.13: > https://lore.kernel.org/linux-fsdevel/YeNyzoDM5hP5LtGW@visor/ Cc in the commit mentions v5.3+, thus we don't need to mention it in the commit message or in docs. ... > diff --git a/testcases/kernel/syscalls/inotify/inotify11.c b/testcases/kernel/syscalls/inotify/inotify11.c > new file mode 100644 > index 000000000..88ac4d2d7 > --- /dev/null > +++ b/testcases/kernel/syscalls/inotify/inotify11.c > @@ -0,0 +1,137 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2022 CTERA Networks. All Rights Reserved. > + * > + * Started by Amir Goldstein <amir73il@gmail.com> > + * based on reproducer from Ivan Delalande <colona@arista.com> > + * > + * DESCRIPTION This should use docparse description header /*\ * [Description] * Test opening files after receiving IN_DELETE. ... FYI we produce doc about test metadata: https://github.com/linux-test-project/ltp/releases/download/20220121/metadata.20220121.html https://github.com/linux-test-project/ltp/releases/download/20220121/metadata.20220121.pdf > + * Test opening files after receiving IN_DELETE. > + * > + * Kernel v5.13 has a regression allowing files to be open after IN_DELETE. > + * > + * The problem has been fixed by commit: > + * a37d9a17f099 "fsnotify: invalidate dcache before IN_DELETE event". > + */ > + > +#include "config.h" > + > +#include <stdio.h> > +#include <unistd.h> > +#include <stdlib.h> > +#include <fcntl.h> > +#include <poll.h> > +#include <time.h> > +#include <signal.h> > +#include <sys/time.h> > +#include <sys/wait.h> > +#include <sys/syscall.h> nit: I'd cleanup some of clearly unused headers: <poll.h> (used in reproducer, but not in LTP code, <sys/syscall.h>, <stdlib.h>, <time.h>, <sys/time.h> (maybe copy paste from previous tests?). I might do a cleanup of the headers in other inotify tests. > + > +#include "tst_test.h" > +#include "inotify.h" > + > +#if defined(HAVE_SYS_INOTIFY_H) > +#include <sys/inotify.h> > + > +/* Number of files to test */ > +#define CHURN_FILES 9999 > + > +#define EVENT_MAX 32 > +/* Size of the event structure, not including the name */ > +#define EVENT_SIZE (sizeof(struct inotify_event)) > +#define EVENT_BUF_LEN (EVENT_MAX * (EVENT_SIZE + 16)) > + > +static pid_t pid; > + > +char event_buf[EVENT_BUF_LEN]; nit: use static > + > +void churn(void) here as well. FYI one of the reasons we're passion about using static is that some people link all tests into single binary to save space (similar way busybox use it). > +{ > + char path[10]; > + int i; > + > + for (i = 0; i <= CHURN_FILES; ++i) { > + snprintf(path, sizeof(path), "%d", i); > + SAFE_FILE_PRINTF(path, "1"); > + SAFE_UNLINK(path); > + } > +} > + > +static void verify_inotify(void) > +{ > + int nevents = 0, opened = 0; > + struct inotify_event *event; > + int inotify_fd; > + > + pid = SAFE_FORK(); > + if (pid == 0) { > + churn(); > + return; > + } > + > + inotify_fd = SAFE_MYINOTIFY_INIT(); > + SAFE_MYINOTIFY_ADD_WATCH(inotify_fd, ".", IN_DELETE); > + > + while (!opened && nevents < CHURN_FILES) { > + int i, fd, len; > + > + len = read(inotify_fd, event_buf, EVENT_BUF_LEN); > + if (len == -1) > + tst_brk(TBROK | TERRNO, "read failed"); Just: len = SAFE_READ(0, inotify_fd, event_buf, EVENT_BUF_LEN); > + > + for (i = 0; i < len; i += EVENT_SIZE + event->len) { > + event = (struct inotify_event *)&event_buf[i]; > + > + if (!(event->mask & IN_DELETE)) > + continue; > + > + nevents++; > + > + /* Open file after IN_DELETE should fail */ > + fd = open(event->name, O_RDONLY); > + if (fd < 0) > + continue; > + > + tst_res(TFAIL, "File %s opened after IN_DELETE", event->name); > + SAFE_CLOSE(fd); > + opened = 1; > + break; > + } > + } > + > + SAFE_CLOSE(inotify_fd); > + > + if (!nevents) > + tst_res(TFAIL, "Didn't get any IN_DELETE events"); > + else if (!opened) > + tst_res(TPASS, "Got %d IN_DELETE events", nevents); Shouldn't we compare nevents == CHURN_FILES instead of just printing the number? > + > + /* Kill the child creating / deleting files and wait for it */ > + SAFE_KILL(pid, SIGKILL); > + pid = 0; > + SAFE_WAIT(NULL); Interesting. I didn't figure out why kill and wait cannot be done just in cleanup. > +} > + > +static void cleanup(void) > +{ > + if (pid) { > + SAFE_KILL(pid, SIGKILL); > + SAFE_WAIT(NULL); > + } > +} > + > +static struct tst_test test = { > + .timeout = 10, I guess you fear of similar problems of fanotify22. Sure, why not (we remove this once we set the default much lower). > + .needs_tmpdir = 1, > + .forks_child = 1, > + .cleanup = cleanup, > + .test_all = verify_inotify, > + .tags = (const struct tst_tag[]) { > + {"linux-git", "a37d9a17f099"}, > + {} > + } > +}; > + > +#else > + TST_TEST_TCONF("system doesn't have required inotify support"); > +#endif Tested-by: Petr Vorel <pvorel@suse.cz> On various systems. Interesting enough on it does not reproduce on affected system with enabled FIPS (at least FIPS on SLES) when run as ordinary user (reproduces when run as root). But that's nothing we should worry about. Reviewed-by: Petr Vorel <pvorel@suse.cz> Suggest to apply with this changes (unless if worth to compare nevents == CHURN_FILES). Kind regards, Petr diff --git testcases/kernel/syscalls/inotify/inotify11.c testcases/kernel/syscalls/inotify/inotify11.c index 88ac4d2d7d..062b92409f 100644 --- testcases/kernel/syscalls/inotify/inotify11.c +++ testcases/kernel/syscalls/inotify/inotify11.c @@ -4,8 +4,10 @@ * * Started by Amir Goldstein <amir73il@gmail.com> * based on reproducer from Ivan Delalande <colona@arista.com> - * - * DESCRIPTION + */ + +/*\ + * [Description] * Test opening files after receiving IN_DELETE. * * Kernel v5.13 has a regression allowing files to be open after IN_DELETE. @@ -18,16 +20,12 @@ #include <stdio.h> #include <unistd.h> -#include <stdlib.h> #include <fcntl.h> -#include <poll.h> -#include <time.h> #include <signal.h> -#include <sys/time.h> #include <sys/wait.h> -#include <sys/syscall.h> #include "tst_test.h" +#include "tst_safe_macros.h" #include "inotify.h" #if defined(HAVE_SYS_INOTIFY_H) @@ -43,9 +41,9 @@ static pid_t pid; -char event_buf[EVENT_BUF_LEN]; +static char event_buf[EVENT_BUF_LEN]; -void churn(void) +static void churn(void) { char path[10]; int i; @@ -75,9 +73,7 @@ static void verify_inotify(void) while (!opened && nevents < CHURN_FILES) { int i, fd, len; - len = read(inotify_fd, event_buf, EVENT_BUF_LEN); - if (len == -1) - tst_brk(TBROK | TERRNO, "read failed"); + len = SAFE_READ(0, inotify_fd, event_buf, EVENT_BUF_LEN); for (i = 0; i < len; i += EVENT_SIZE + event->len) { event = (struct inotify_event *)&event_buf[i];
> > Tested-by: Petr Vorel <pvorel@suse.cz> > On various systems. Interesting enough on it does not reproduce on affected > system with enabled FIPS (at least FIPS on SLES) when run as ordinary user > (reproduces when run as root). But that's nothing we should worry about. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Your fixes look fine to me. > Suggest to apply with this changes (unless if worth to compare nevents == > CHURN_FILES). I wouldn't worry about this - it's not related to the regression that we are testing. Thanks, Amir. > > Kind regards, > Petr > > diff --git testcases/kernel/syscalls/inotify/inotify11.c testcases/kernel/syscalls/inotify/inotify11.c > index 88ac4d2d7d..062b92409f 100644 > --- testcases/kernel/syscalls/inotify/inotify11.c > +++ testcases/kernel/syscalls/inotify/inotify11.c > @@ -4,8 +4,10 @@ > * > * Started by Amir Goldstein <amir73il@gmail.com> > * based on reproducer from Ivan Delalande <colona@arista.com> > - * > - * DESCRIPTION > + */ > + > +/*\ > + * [Description] > * Test opening files after receiving IN_DELETE. > * > * Kernel v5.13 has a regression allowing files to be open after IN_DELETE. > @@ -18,16 +20,12 @@ > > #include <stdio.h> > #include <unistd.h> > -#include <stdlib.h> > #include <fcntl.h> > -#include <poll.h> > -#include <time.h> > #include <signal.h> > -#include <sys/time.h> > #include <sys/wait.h> > -#include <sys/syscall.h> > > #include "tst_test.h" > +#include "tst_safe_macros.h" > #include "inotify.h" > > #if defined(HAVE_SYS_INOTIFY_H) > @@ -43,9 +41,9 @@ > > static pid_t pid; > > -char event_buf[EVENT_BUF_LEN]; > +static char event_buf[EVENT_BUF_LEN]; > > -void churn(void) > +static void churn(void) > { > char path[10]; > int i; > @@ -75,9 +73,7 @@ static void verify_inotify(void) > while (!opened && nevents < CHURN_FILES) { > int i, fd, len; > > - len = read(inotify_fd, event_buf, EVENT_BUF_LEN); > - if (len == -1) > - tst_brk(TBROK | TERRNO, "read failed"); > + len = SAFE_READ(0, inotify_fd, event_buf, EVENT_BUF_LEN); > > for (i = 0; i < len; i += EVENT_SIZE + event->len) { > event = (struct inotify_event *)&event_buf[i];
Hi Amir, > Your fixes look fine to me. > > Suggest to apply with this changes (unless if worth to compare nevents == > > CHURN_FILES). > I wouldn't worry about this - it's not related to the regression that > we are testing. Thanks for your ack, going to merge with just proposed changes. Kind regards, Petr > Thanks, > Amir.
Hi! > > + /* Kill the child creating / deleting files and wait for it */ > > + SAFE_KILL(pid, SIGKILL); > > + pid = 0; > > + SAFE_WAIT(NULL); > Interesting. I didn't figure out why kill and wait cannot be done just in > cleanup. I guess that we want to stop the child in the case that we happened to open the deleted file and did abort the testing. Technically we send a signal to a zombie process in the case that the bug was not reproduced (as the child exits the verify_inotify() with return it will just do exit(0) once it reaches test library code). But that should be harmless. On the other hand I guess that it may make the code a bit more readable if we moved the SAFE_KILL() just after opened = 1 in the inner loop.
Hi Cyril, > Hi! > > > + /* Kill the child creating / deleting files and wait for it */ > > > + SAFE_KILL(pid, SIGKILL); > > > + pid = 0; > > > + SAFE_WAIT(NULL); > > Interesting. I didn't figure out why kill and wait cannot be done just in > > cleanup. > I guess that we want to stop the child in the case that we happened to > open the deleted file and did abort the testing. > Technically we send a signal to a zombie process in the case that the > bug was not reproduced (as the child exits the verify_inotify() with > return it will just do exit(0) once it reaches test library code). But > that should be harmless. > On the other hand I guess that it may make the code a bit more readable > if we moved the SAFE_KILL() just after opened = 1 in the inner loop. Yes, it'd be probably more readable if the code was in after opened = 1. But I'm sorry I already merged the original version. Kind regards, Petr
diff --git a/runtest/syscalls b/runtest/syscalls index 3b2deb64e..2f05dcfa1 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -585,6 +585,7 @@ inotify07 inotify07 inotify08 inotify08 inotify09 inotify09 inotify10 inotify10 +inotify11 inotify11 fanotify01 fanotify01 fanotify02 fanotify02 diff --git a/testcases/kernel/syscalls/inotify/.gitignore b/testcases/kernel/syscalls/inotify/.gitignore index da9bfc767..593cf6c04 100644 --- a/testcases/kernel/syscalls/inotify/.gitignore +++ b/testcases/kernel/syscalls/inotify/.gitignore @@ -8,3 +8,4 @@ /inotify08 /inotify09 /inotify10 +/inotify11 diff --git a/testcases/kernel/syscalls/inotify/inotify11.c b/testcases/kernel/syscalls/inotify/inotify11.c new file mode 100644 index 000000000..88ac4d2d7 --- /dev/null +++ b/testcases/kernel/syscalls/inotify/inotify11.c @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2022 CTERA Networks. All Rights Reserved. + * + * Started by Amir Goldstein <amir73il@gmail.com> + * based on reproducer from Ivan Delalande <colona@arista.com> + * + * DESCRIPTION + * Test opening files after receiving IN_DELETE. + * + * Kernel v5.13 has a regression allowing files to be open after IN_DELETE. + * + * The problem has been fixed by commit: + * a37d9a17f099 "fsnotify: invalidate dcache before IN_DELETE event". + */ + +#include "config.h" + +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <fcntl.h> +#include <poll.h> +#include <time.h> +#include <signal.h> +#include <sys/time.h> +#include <sys/wait.h> +#include <sys/syscall.h> + +#include "tst_test.h" +#include "inotify.h" + +#if defined(HAVE_SYS_INOTIFY_H) +#include <sys/inotify.h> + +/* Number of files to test */ +#define CHURN_FILES 9999 + +#define EVENT_MAX 32 +/* Size of the event structure, not including the name */ +#define EVENT_SIZE (sizeof(struct inotify_event)) +#define EVENT_BUF_LEN (EVENT_MAX * (EVENT_SIZE + 16)) + +static pid_t pid; + +char event_buf[EVENT_BUF_LEN]; + +void churn(void) +{ + char path[10]; + int i; + + for (i = 0; i <= CHURN_FILES; ++i) { + snprintf(path, sizeof(path), "%d", i); + SAFE_FILE_PRINTF(path, "1"); + SAFE_UNLINK(path); + } +} + +static void verify_inotify(void) +{ + int nevents = 0, opened = 0; + struct inotify_event *event; + int inotify_fd; + + pid = SAFE_FORK(); + if (pid == 0) { + churn(); + return; + } + + inotify_fd = SAFE_MYINOTIFY_INIT(); + SAFE_MYINOTIFY_ADD_WATCH(inotify_fd, ".", IN_DELETE); + + while (!opened && nevents < CHURN_FILES) { + int i, fd, len; + + len = read(inotify_fd, event_buf, EVENT_BUF_LEN); + if (len == -1) + tst_brk(TBROK | TERRNO, "read failed"); + + for (i = 0; i < len; i += EVENT_SIZE + event->len) { + event = (struct inotify_event *)&event_buf[i]; + + if (!(event->mask & IN_DELETE)) + continue; + + nevents++; + + /* Open file after IN_DELETE should fail */ + fd = open(event->name, O_RDONLY); + if (fd < 0) + continue; + + tst_res(TFAIL, "File %s opened after IN_DELETE", event->name); + SAFE_CLOSE(fd); + opened = 1; + break; + } + } + + SAFE_CLOSE(inotify_fd); + + if (!nevents) + tst_res(TFAIL, "Didn't get any IN_DELETE events"); + else if (!opened) + tst_res(TPASS, "Got %d IN_DELETE events", nevents); + + /* Kill the child creating / deleting files and wait for it */ + SAFE_KILL(pid, SIGKILL); + pid = 0; + SAFE_WAIT(NULL); +} + +static void cleanup(void) +{ + if (pid) { + SAFE_KILL(pid, SIGKILL); + SAFE_WAIT(NULL); + } +} + +static struct tst_test test = { + .timeout = 10, + .needs_tmpdir = 1, + .forks_child = 1, + .cleanup = cleanup, + .test_all = verify_inotify, + .tags = (const struct tst_tag[]) { + {"linux-git", "a37d9a17f099"}, + {} + } +}; + +#else + TST_TEST_TCONF("system doesn't have required inotify support"); +#endif
Check that files cannot be opened after IN_DELETE was reported on them. This test is based on the reproducer provided by Ivan Delalande for a regression reported in kernel v5.13: https://lore.kernel.org/linux-fsdevel/YeNyzoDM5hP5LtGW@visor/ Reported-by: Ivan Delalande <colona@arista.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Hi Petr, The fix for this regression was applied to stable kernels 5.4.y, 5.10.y, 5.15.y, 5.16.y. The mentioned git tag is only the upstream commit. Feel free to add the stable git tags if you think they are needed. Thanks, Amir, runtest/syscalls | 1 + testcases/kernel/syscalls/inotify/.gitignore | 1 + testcases/kernel/syscalls/inotify/inotify11.c | 137 ++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 testcases/kernel/syscalls/inotify/inotify11.c