Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: John Hubbard <jhubbard@nvidia.com>
Cc: Jan Kara <jack@suse.cz>, Linux-MM <linux-mm@kvack.org>,
	linux-fsdevel@vger.kernel.org,
	Alex Williamson <alex.williamson@redhat.com>
Subject: Re: Question: "Bare" set_page_dirty_lock() call in vhost.c
Date: Fri, 29 May 2020 09:03:43 +0200
Message-ID: <20200529070343.GL14550@quack2.suse.cz> (raw)
In-Reply-To: <3b2db4da-9e4e-05d1-bf89-a261f0eb6de0@nvidia.com>

Hi!

On Thu 28-05-20 17:59:30, John Hubbard wrote:
> 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.

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 <jack@suse.com>
SUSE Labs, CR

  reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-29  0:59 John Hubbard
2020-05-29  7:03 ` Jan Kara [this message]
2020-05-29  7:28   ` John Hubbard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200529070343.GL14550@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=alex.williamson@redhat.com \
    --cc=jhubbard@nvidia.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git