Re: [PATCH v4 2/2] mm/gup/writeback: add callbacks for inaccessible pages

Dave Hansen
2020-04-13T20:22:30Z

On 3/6/20 5:25 AM, Claudio Imbrenda wrote:
> On s390x the function is not supposed to fail, so it is ok to use a
> WARN_ON on failure. If we ever need some more finegrained handling
> we can tackle this when we know the details.

Could you explain a bit why the function can't fail?

If the guest has secret data in the page, then it *can* and does fail.
It won't fail, though, if the host and guest agree on whether the page
is protected.

Right?

> @@ -2807,6 +2807,13 @@ int __test_set_page_writeback(struct page *page, bool keep_write)
> inc_zone_page_state(page, NR_ZONE_WRITE_PENDING);
> }
> unlock_page_memcg(page);
> + access_ret = arch_make_page_accessible(page);
> + /*
> + * If writeback has been triggered on a page that cannot be made
> + * accessible, it is too late to recover here.
> + */
> + VM_BUG_ON_PAGE(access_ret != 0, page);
> +
> return ret;
>
> }

This seems like a really odd place to do this. Writeback is specific to
block I/O. I would have thought there were other kinds of devices that
matter, not just block devices.

Also, this patch seems odd that it only does the
arch_make_page_accessible() half. Where's the other half where the page
is made inaccessible?

I assume it's OK to "leak" things like this, it's just not clear to me
_why_ it's OK.