Possible deadlock in fuse write path (Was: Re: [PATCH 0/4] Some more lock_page work..)

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Oct 14, 2020 at 07:44:16PM -0700, Linus Torvalds wrote:
> On Wed, Oct 14, 2020 at 6:48 PM Qian Cai <cai@lca.pw> wrote:
> >
> > While on this topic, I just want to bring up a bug report that we are chasing an
> > issue that a process is stuck in the loop of wait_on_page_bit_common() for more
> > than 10 minutes before I gave up.
> 
> Judging by call trace, that looks like a deadlock rather than a missed wakeup.
> 
> The trace isn't reliable, but I find it suspicious that the call trace
> just before the fault contains that
> "iov_iter_copy_from_user_atomic()".
> 
> IOW, I think you're in fuse_fill_write_pages(), which has allocated
> the page, locked it, and then it takes a page fault.
> 
> And the page fault waits on a page that is locked.
> 
> This is a classic deadlock.
> 
> The *intent* is that iov_iter_copy_from_user_atomic() returns zero,
> and you retry without the page lock held.
> 
> HOWEVER.
> 
> That's not what fuse actually does. Fuse will do multiple pages, and
> it will unlock only the _last_ page. It keeps the other pages locked,
> and puts them in an array:
> 
>                 ap->pages[ap->num_pages] = page;
> 
> And after the iov_iter_copy_from_user_atomic() fails, it does that
> "unlock" and repeat.
> 
> But while the _last_ page was unlocked, the *previous* pages are still
> locked in that array. Deadlock.
> 
> I really don't think this has anything at all to do with page locking,
> and everything to do with fuse_fill_write_pages() having a deadlock if
> the source of data is a mmap of one of the pages it is trying to write
> to (just with an offset, so that it's not the last page).
> 
> See a similar code sequence in generic_perform_write(), but notice how
> that code only has *one* page that it locks, and never holds an array
> of pages around over that iov_iter_fault_in_readable() thing.

Indeed. This is a deadlock in fuse. Thanks for the analysis. I can now
trivially reproduce it with following program.

Thanks
Vivek

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/uio.h>

int main(int argc, char *argv[])
{
	int fd, ret;
	void *map_addr;
	size_t map_length = 2 * 4096;
	char *buf_out = "Hello World";
	struct iovec iov[2];

	if (argc != 2 ) {
		printf("Usage:%s <file-to-write> \n", argv[0]);
		exit(1);
	}

	fd = open(argv[1], O_RDWR | O_TRUNC);
	if (fd == -1) {
		fprintf(stderr, "Failed to open file %s:%s, errorno=%d\n", argv[1], strerror(errno), errno);
		exit(1);
	}

	map_addr = mmap(NULL, map_length, PROT_READ | PROT_WRITE, MAP_SHARED,
			fd, 0);
	if (map_addr == MAP_FAILED) {
		fprintf(stderr, "mmap failed %s, errorno=%d\n", strerror(errno),
			errno);
		exit(1);
	}

	/* Write first page and second page */
	pwrite(fd, buf_out, strlen(buf_out), 0);
	pwrite(fd, buf_out, strlen(buf_out), 4096);

	/* Copy from first page and then second page */
	iov[0].iov_base = map_addr;
	iov[0].iov_len = strlen(buf_out) + 1;

	iov[1].iov_base = map_addr + 4096;
	iov[1].iov_len = strlen(buf_out) + 1;

	/*
	 * Write second page offset (4K - 12), reading from first page and
	 * then second page. In first iteration we should fault in first
	 * page and lock second page. And in second iteration we should
	 * try fault in second page which is locked. Deadlock.
	 */
	ret = pwritev(fd, iov, 2, 4096 + 4096 - 12);
	printf("write() returned=%d\n", ret);
	munmap(map_addr, map_length);
	close(fd);
}