Re: Potential regression after fsnotify_nameremove() rework in 5.3

[Ivan Delalande <colona@arista.com>]

On Sat, Jan 15, 2022 at 09:50:20PM +0200, Amir Goldstein wrote:
> On Sat, Jan 15, 2022 at 5:11 AM Ivan Delalande <colona@arista.com> wrote:
>> Sorry to bring this up so late but we might have found a regression
>> introduced by your "Sort out fsnotify_nameremove() mess" patch series
>> merged in 5.3 (116b9731ad76..7377f5bec133), and that can still be
>> reproduced on v5.16.
>>
>> Some of our processes use inotify to watch for IN_DELETE events (for
>> files on tmpfs mostly), and relied on the fact that once such events are
>> received, the files they refer to have actually been unlinked and can't
>> be open/read. So if and once open() succeeds then it is a new version of
>> the file that has been recreated with new content.
>>
>> This was true and working reliably before 5.3, but changed after
>> 49246466a989 ("fsnotify: move fsnotify_nameremove() hook out of
>> d_delete()") specifically. There is now a time window where a process
>> receiving one of those IN_DELETE events may still be able to open the
>> file and read its old content before it's really unlinked from the FS.
> 
> This is a bit surprising to me.
> Do you have a reproducer?

Yeah, I was using the following one to bisect this. It will print a
message every time it succeeds to read the file after receiving a
IN_DELETE event when run with something like `mkdir /tmp/foo;
./indelchurn /tmp/foo`. It seems to hit pretty frequently and reliably
on various systems after 5.3, even for different #define-parameters.

>> I'm not very familiar with the VFS and fsnotify internals, would you
>> consider this a regression, or was there never any intentional guarantee
>> for that behavior and it's best we work around this change in userspace?
> 
> It may be a regression if applications depend on behavior that changed,
> but if are open for changes in your application please describe in more details
> what it tries to accomplish using IN_DELETE and the ekernel your application
> is running on and then I may be able to recommend a more reliable method.

I can discuss it with our team and get more details on this but it may
be pretty complicated and costly to change. My understanding is that
these watched files are used as ID/version references for in-memory
objects shared between multiple processes to synchronize state, and
resynchronize when there are crashes or restarts. So they can be
recreated in place with the same or a different content, and so their
inode number or mtime etc. may not be useable as additonnal check.

I think we can also have a very large number of these objects and files
on some configurations, so waiting to see if we have a following
IN_CREATE, or adding more checks/synchronization logic will probably
have a significant impact at scale.

And we're targeting 5.10 and building it with our own config.

Thanks for your help and insight on this,

-- 
Ivan Delalande
Arista Networks

# ------------------------ >8 ------------------------

#include <fcntl.h>
#include <poll.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/inotify.h>
#include <sys/wait.h>
#include <unistd.h>

#define CHURN_FILES 9999
#define CHURN_SLEEP 2
#define BATCH_UNLINK 0
#define EVENTS_BUF 32

void churn(char* base)
{
	static size_t iter = 0;
	char path[128];
	int ret;

	for (int i = 0; i <= CHURN_FILES; ++i) {
		snprintf(path, sizeof(path), "%s/%d", base, i);
		FILE* f = fopen(path, "w");
		if (!f)
			exit(1);
		ret = fprintf(f, "content, iter %lu\n", iter);
		if (ret < 16)
			exit(1);
		fclose(f);
#if BATCH_UNLINK
	}
	for (int i = 0; i <= CHURN_FILES; ++i) {
		snprintf(path, sizeof(path), "%s/%d", base, i);
#endif
		ret = unlink(path);
		if (ret < 0)
			exit(1);
	}
	++iter;
}

int main(int argc, char* argv[])
{
	char events_buf[EVENTS_BUF * sizeof(struct inotify_event)];
	struct pollfd pfd = { .events = POLLIN };
	const struct inotify_event *event;
	char path[128];
	char buf[128];
	int ret;

	if (argc != 2 || access(argv[1], R_OK|W_OK)) {
		printf("%s TESTDIR\n", argv[0]);
		exit(1);
	}

	int pid = fork();
	if (pid == 0) {
		while (1) {
			churn(argv[1]);
			sleep(CHURN_SLEEP);
		}
		return 0;
	}
	else if (pid < 0)
		exit(1);

	pfd.fd = inotify_init();
	if (pfd.fd < 0)
		goto out;
	ret = inotify_add_watch(pfd.fd, argv[1], IN_DELETE);
	if (ret < 0)
		goto out;

	while (1) {
		ret = poll(&pfd, 1, -1);
		if (ret < 0)
			goto out;
		if (!(pfd.revents & POLLIN))
			continue;

		ssize_t len = read(pfd.fd, events_buf, sizeof(events_buf));
		if (len < 0)
			goto out;
		for (char *ptr = events_buf; ptr < events_buf + len;
		     ptr += sizeof(struct inotify_event) + event->len) {
			event = (const struct inotify_event *)ptr;
			if (!(event->mask & IN_DELETE))
				continue;
			snprintf(path, sizeof(path), "%s/%s", argv[1],
				 event->name);
			int f = open(path, O_RDONLY);
			if (f < 0)
				continue;
			ret = read(f, buf, sizeof(buf));
			if (ret >= 0)
				printf("Read %d from %.*s: %.*s", ret,
				       sizeof(path), path, ret, buf);
			close(f);
		}
	}

out:
	kill(pid, SIGTERM);
	wait(NULL);
	return 0;
}