Linux Virtualization - Mar 2019 - [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address

On Thu, Mar 07, 2019 at 10:45:57AM +0800, Jason Wang
wrote:
> 
> On 2019/3/7 ??12:31, Michael S. Tsirkin wrote:
> > > +static void vhost_set_vmap_dirty(struct vhost_vmap *used)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < used->npages; i++)
> > > +		set_page_dirty_lock(used->pages[i]);
> > This seems to rely on page lock to mark page dirty.
> > 
> > Could it happen that page writeback will check the
> > page, find it clean, and then you mark it dirty and then
> > invalidate callback is called?
> > 
> > 
> 
> Yes. But does this break anything?
> The page is still there, we just remove a
> kernel mapping to it.
> 
> Thanks
Yes it's the same problem as e.g. RDMA:
	we've just marked the page as dirty without having buffers.
	Eventually writeback will find it and filesystem will complain...
	So if the pages are backed by a non-RAM-based filesystem, it?s all just broken.

one can hope that RDMA guys will fix it in some way eventually.
For now, maybe add a flag in e.g. VMA that says that there's no
writeback so it's safe to mark page dirty at any point?





-- 
MST

On Thu, Mar 07, 2019 at 10:34:39AM -0500, Michael S. Tsirkin
wrote:
> On Thu, Mar 07, 2019 at 10:45:57AM +0800, Jason Wang wrote:
> > 
> > On 2019/3/7 ??12:31, Michael S. Tsirkin wrote:
> > > > +static void vhost_set_vmap_dirty(struct vhost_vmap *used)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < used->npages; i++)
> > > > +		set_page_dirty_lock(used->pages[i]);
> > > This seems to rely on page lock to mark page dirty.
> > > 
> > > Could it happen that page writeback will check the
> > > page, find it clean, and then you mark it dirty and then
> > > invalidate callback is called?
> > > 
> > > 
> > 
> > Yes. But does this break anything?
> > The page is still there, we just remove a
> > kernel mapping to it.
> > 
> > Thanks
> 
> Yes it's the same problem as e.g. RDMA:
> 	we've just marked the page as dirty without having buffers.
> 	Eventually writeback will find it and filesystem will complain...
> 	So if the pages are backed by a non-RAM-based filesystem, it?s all just
broken.
> 
> one can hope that RDMA guys will fix it in some way eventually.
> For now, maybe add a flag in e.g. VMA that says that there's no
> writeback so it's safe to mark page dirty at any point?
I thought this patch was only for anonymous memory ie not file back ?
If so then set dirty is mostly useless it would only be use for swap
but for this you can use an unlock version to set the page dirty.

Cheers,
J?r?me

On Thu, Mar 07, 2019 at 02:09:10PM -0500, Jerome Glisse
wrote:
> I thought this patch was only for anonymous memory ie not file back ?
Yes, the other common usages are on hugetlbfs/tmpfs that also don't
need to implement writeback and are obviously safe too.
> If so then set dirty is mostly useless it would only be use for swap
> but for this you can use an unlock version to set the page dirty.
It's not a practical issue but a security issue perhaps: you can
change the KVM userland to run on VM_SHARED ext4 as guest physical
memory, you could do that with the qemu command line that is used to
place it on tmpfs or hugetlbfs for example and some proprietary KVM
userland may do for other reasons. In general it shouldn't be possible
to crash the kernel with this, and it wouldn't be nice to fail if
somebody decides to put VM_SHARED ext4 (we could easily allow vhost
ring only backed by anon or tmpfs or hugetlbfs to solve this of
course).

It sounds like we should at least optimize away the _lock from
set_page_dirty if it's anon/hugetlbfs/tmpfs, would be nice if there
was a clean way to do that.

Now assuming we don't nak the use on ext4 VM_SHARED and we stick to
set_page_dirty_lock for such case: could you recap how that
__writepage ext4 crash was solved if try_to_free_buffers() run on a
pinned GUP page (in our vhost case try_to_unmap would have gotten rid
of the pins through the mmu notifier and the page would have been
freed just fine).

The first two things that come to mind is that we can easily forbid
the try_to_free_buffers() if the page might be pinned by GUP, it has
false positives with the speculative pagecache lookups but it cannot
give false negatives. We use those checks to know when a page is
pinned by GUP, for example, where we cannot merge KSM pages with gup
pins etc... However what if the elevated refcount wasn't there when
try_to_free_buffers run and is there when __remove_mapping runs?

What I mean is that it sounds easy to forbid try_to_free_buffers for
the long term pins, but that still won't prevent the same exact issue
for a transient pin (except the window to trigger it will be much smaller).

I basically don't see how long term GUP pins breaks stuff in ext4
while transient short term GUP pins like O_DIRECT don't. The VM code
isn't able to disambiguate if the pin is short or long term and it
won't even be able to tell the difference between a GUP pin (long or
short term) and a speculative get_page_unless_zero run by the
pagecache speculative pagecache lookup. Even a random speculative
pagecache lookup that runs just before __remove_mapping, can cause
__remove_mapping to fail despite try_to_free_buffers() succeeded
before it (like if there was a transient or long term GUP
pin). speculative lookup that can happen across all page struct at all
times and they will cause page_ref_freeze in __remove_mapping to
fail.

I'm sure I'm missing details on the ext4 __writepage problem and how
set_page_dirty_lock broke stuff with long term GUP pins, so I'm
asking...

Thanks!
Andrea

On 2019/3/7 ??11:34, Michael S. Tsirkin wrote:
> On Thu, Mar 07, 2019 at 10:45:57AM +0800, Jason Wang wrote:
>> On 2019/3/7 ??12:31, Michael S. Tsirkin wrote:
>>>> +static void vhost_set_vmap_dirty(struct vhost_vmap *used)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < used->npages; i++)
>>>> +		set_page_dirty_lock(used->pages[i]);
>>> This seems to rely on page lock to mark page dirty.
>>>
>>> Could it happen that page writeback will check the
>>> page, find it clean, and then you mark it dirty and then
>>> invalidate callback is called?
>>>
>>>
>> Yes. But does this break anything?
>> The page is still there, we just remove a
>> kernel mapping to it.
>>
>> Thanks
> Yes it's the same problem as e.g. RDMA:
> 	we've just marked the page as dirty without having buffers.
> 	Eventually writeback will find it and filesystem will complain...
> 	So if the pages are backed by a non-RAM-based filesystem, it?s all just
broken.

Yes, we can't depend on the pages that might have been invalidated. As 
suggested, the only suitable place is the MMU notifier callbacks.

Thanks

> one can hope that RDMA guys will fix it in some way eventually.
> For now, maybe add a flag in e.g. VMA that says that there's no
> writeback so it's safe to mark page dirty at any point?
>
>
>
>
>