Question: "Bare" set_page_dirty_lock() call in vhost.c

Hi,

While trying to figure out which things to convert from get_user_pages*() to put_user_pages*(), I came across an interesting use of set_page_dirty_lock(), and wanted to ask about it.

Is it safe to call set_page_dirty_lock() like this (for the case when this is file-backed memory):

// drivers/vhost/vhost.c:1757:
static int set_bit_to_user(int nr, void __user *addr)

{
	unsigned long log = (unsigned long)addr;
	struct page *page;
	void *base;
	int bit = nr + (log % PAGE_SIZE) * 8;
	int r;

	r = get_user_pages_fast(log, 1, FOLL_WRITE, &page);
	if (r < 0)
		return r;
	BUG_ON(r != 1);
	base = kmap_atomic(page);
	set_bit(bit, base);
	kunmap_atomic(base);
	set_page_dirty_lock(page);
	put_page(page);
	return 0;
}

?

That is, after the page is unmapped, but before unpinning it? Specifically, I'd expect that the writeback and reclaim code code can end up calling drop_buffers() (because the set_bit() call actually did dirty the pte), after the kunmap_atomic() call. So then when set_page_dirty_lock() runs, it could bug check on ext4_writepage()'s attempt to buffer heads:

ext4_writepage()

	page_bufs = page_buffers(page);
        #define page_buffers(page)					\
        	({							\
        		BUG_ON(!PagePrivate(page));			\
        		((struct buffer_head *)page_private(page));	\
        	})

...which actually is the the case that pin_user_pages*() is ultimately helping to avoid, btw. But in this case, it's all code that runs on a CPU, so no DMA or DIO is involved. But still, the "bare" use of set_page_dirty_lock() seems like a problem here.

thanks,

John Hubbard
NVIDIA

Hi!

I agree that the site like above needs pin_user_pages(). The problem has actually nothing to do with kmap_atomic() - that actually doesn't do anything interesting on x86_64. The moment GUP_fast() returns, page can be unmapped from page tables and written back so this site has exactly same problems as any other using DMA or DIO. As we discussed earlier when designing the patch set, the problem is really with GUP reference being used to access page data. And the method of access (DMA, CPU access, GPU access, ...) doesn't really matter...

Honza

Jan Kara <[email protected]>
SUSE Labs, CR

On 2020-05-29 00:03, Jan Kara wrote:
...

Awesome, I was really starting to wonder what I was missing. This all makes perfect sense, thanks.

Maybe I'll add a "Case 5" to Documentation/core-api/pin_user_pages.rst, to cover this sort of situation. It's not completely obvious from the first four cases, that this code is exposed to that problem.

thanks,

John Hubbard
NVIDIA