[v3] fsnotify: rework unlink/rmdir notify events

A regression was introduced by 116b9731ad76 ("fsnotify: add empty
fsnotify_{unlink,rmdir}() hooks"), which moved the fsnotify event for
unlink and rmdir to before the d_delete.  This was noticed by a tool we
have internally for validating a FUSE file system.  This tool watches
for IN_DELETE events and then stat's the file to make sure the file was
actually deleted.  This started failing on our newer kernels, and it was
traced to this patch.

The problem is there's a slight window where we emit the event and
we delete the dentry.  We can easily get the event before we've called
d_delete, and then stat the file before we're able to remove it.  This
is easily reproducible with the following reproducer

static int failed = 0;

static void *watch_inotify(void *arg)
{
	char name_buf[4096];
	char *buf = malloc(EVENT_BUF_LEN);
	char *directory = (char *)arg;
	int fd;
	int wd;

	if (!buf) {
		fprintf(stderr, "Failed to allocate inotify buf\n");
		return NULL;
	}

	fd = inotify_init();
	if (fd < 0) {
		fprintf(stderr, "Failed to init inotify %d %s\n",
				errno, strerror(errno));
		return NULL;
	}

	wd = inotify_add_watch(fd, directory, IN_DELETE);
	if (wd < 0) {
		fprintf(stderr, "inotify watch failed %d %s\n", errno,
				strerror(errno));
		close(fd);
		return NULL;
	}

	while (!failed) {
		ssize_t len = read(fd, buf, EVENT_BUF_LEN);
		size_t cur = 0;

		if (len < 0) {
			fprintf(stderr, "failed to read from inotify %d %s\n",
					errno, strerror(errno));
			break;
		}

		while (cur < len) {
			struct inotify_event *event =
				(struct inotify_event *)(buf + cur);
			struct stat st;
			int ret;

			cur += EVENT_SIZE + event->len;
			if (event->len == 0)
				continue;
			if (!(event->mask & IN_DELETE))
				continue;
			ret = snprintf(name_buf, 4096, "%s/%s", directory,
					event->name);
			if (ret < 0) {
				fprintf(stderr, "Couldn't snprintf %d (%s)\n",
						errno, strerror(errno));
				break;
			}
			if (ret >= 4096) {
				fprintf(stderr, "Name was truncated, pick a shorter dir name\n");
				break;
			}
			ret = stat(name_buf, &st);
			if (!ret) {
				fprintf(stderr, "Found file %s after a delete event\n",
						name_buf);
				failed = 1;
				break;
			}
		}
	}
	inotify_rm_watch( fd, wd );
	close(fd);
	return NULL;
}

int main(int argc, char **argv)
{
	char buf[4096];
	char *dir;
	pthread_t inotify_thread;
	int ret, i;

	if (argc != 2) {
		fprintf(stderr, "Must specify a directory to use\n");
		return 1;
	}

	dir = strdup(argv[1]);
	if (!dir) {
		fprintf(stderr, "Couldn't dup directory\n");
		return 1;
	}

	printf("Creating files...\n");
	for (i = 0; i < NR_FILES; i++) {
		ret = snprintf(buf, 4096, "%s/file_%d", dir, i);
		if (ret < 0) {
			fprintf(stderr, "Couldn't snprintf %d (%s)\n",
					errno, strerror(errno));
			goto out;
		}
		if (ret >= 4096) {
			fprintf(stderr, "Name was truncated, pick a shorter dir name\n");
			goto out;
		}
		ret = creat(buf, 0600);
		if (ret < 0) {
			fprintf(stderr, "Failed to create %s %d %s\n",
					buf, errno, strerror(errno));
			goto out;
		}
		close(ret);
	}

	printf("Starting inotify thread\n");
	ret = pthread_create(&inotify_thread, NULL, watch_inotify, dir);
	if (ret) {
		fprintf(stderr, "Couldn't create inotify thread %d (%s)\n",
				errno, strerror(errno));
		goto out;
	}

	printf("Sleeping for a second\n");
	sleep(1);
	printf("Starting delete\n");
	for (i = 0; i < NR_FILES; i++) {
		ret = snprintf(buf, 4096, "%s/file_%d", dir, i);
		if (ret < 0) {
			fprintf(stderr, "Couldn't snprintf %d (%s)\n",
					errno, strerror(errno));
			goto out_pthread;
		}
		if (ret >= 4096) {
			fprintf(stderr, "Name was truncated, pick a shorter dir name\n");
			goto out_pthread;
		}
		ret = unlink(buf);
		if (ret)
			goto out_pthread;
	}
out_pthread:
	pthread_cancel(inotify_thread);
	pthread_join(inotify_thread, NULL);
out:
	free(dir);
	return ret ? 1 : failed;
}

Fix this by introducing a d_delete_notify() and a fsnotify_delete()
helper.  d_delete_notify() will hold onto the dentry inode, do the
d_delete, and then call fsnotify_delete() so that we avoid the race.
Then fix up all callers of the fsnotify_unlink/fsnotify_rmdir helpers to
either use d_delete_notify(), or use the fsnotify_delete() helper with
the appropriate changes to protect the lifetime of the inode.  This
patch makes the test no longer fail.

Fixes: 116b9731ad76 ("fsnotify: add empty fsnotify_{unlink,rmdir}() hooks")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v2->v3:
- Added an EXPORT_SYMBOL() for d_delete_notify, cause I'm an idiot.

v1->v2:
- Took Amir's suggestion and wrapped all the weird work into a d_delete_notify()
  helper and used that everywhere.
- Removed fsnotify_unlink/fsnotify_rmdir, replaced it with a fsnotify_delete()
  helper.

 fs/btrfs/ioctl.c         |  6 ++----
 fs/configfs/dir.c        |  6 ++----
 fs/dcache.c              | 19 +++++++++++++++++++
 fs/devpts/inode.c        |  7 ++++++-
 fs/libfs.c               | 10 +++++-----
 fs/namei.c               |  6 ++----
 fs/nfsd/nfsctl.c         |  3 +--
 include/linux/dcache.h   |  1 +
 include/linux/fsnotify.h | 25 +++++++------------------
 net/sunrpc/rpc_pipe.c    | 15 +++++++++++----
 10 files changed, 56 insertions(+), 42 deletions(-)

Message ID	568db8243e9faa0efb9ffb545ffac5a2f87e65ef.1620999079.git.josef@toxicpanda.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-fsdevel-owner@kernel.org> From: Josef Bacik <josef@toxicpanda.com> To: linux-btrfs@vger.kernel.org, kernel-team@fb.com, linux-fsdevel@vger.kernel.org, viro@ZenIV.linux.org.uk, jack@suse.cz, amir73il@gmail.com, wangyugui@e16-tech.com Subject: [PATCH v3] fsnotify: rework unlink/rmdir notify events Date: Fri, 14 May 2021 09:34:30 -0400 Message-Id: <568db8243e9faa0efb9ffb545ffac5a2f87e65ef.1620999079.git.josef@toxicpanda.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[v3] fsnotify: rework unlink/rmdir notify events \| expand [v3] fsnotify: rework unlink/rmdir notify events

[v3] fsnotify: rework unlink/rmdir notify events

Commit Message

Comments

Patch