[LTP] [PATCH] syscalls/inotify: New test for IN_DELETE regression
Petr Vorel
pvorel@suse.cz
Mon Feb 7 10:53:37 CET 2022
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];
More information about the ltp
mailing list