Re: [PATCH] xen: speed up grant-table reclaim

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12.06.23 22:09, Demi Marie Obenour wrote:
On Mon, Jun 12, 2023 at 08:27:59AM +0200, Juergen Gross wrote:
On 10.06.23 17:32, Demi Marie Obenour wrote:
When a grant entry is still in use by the remote domain, Linux must put
it on a deferred list.

This lacks quite some context.

The main problem is related to the grant not having been unmapped after
the end of a request, but the side granting the access is assuming this
should be the case.

The GUI agent has relied on deferred grant reclaim for as long as it has
existed.  One could argue that doing so means that the agent is misusing
gntalloc, but this is not documented anywhere.  A better fix would be to
use IOCTL_GNTDEV_SET_UNMAP_NOTIFY in the GUI daemon.

I don't think this is a gntalloc specific problem. You could create the same
problem with a kernel implementation.

And yes, using IOCTL_GNTDEV_SET_UNMAP_NOTIFY might be a good idea.

In general this means that the two sides implementing the protocol don't
agree how it should work, or that the protocol itself has a flaw.

What would a better solution be?  This is going to be particularly
tricky with Wayland, as the wl_shm protocol makes absolutely no
guarantees that compositors will promptly release the mapping and
provides no way whatsoever for Wayland clients to know when this has
happened.  Relying on an LD_PRELOAD hack is not sustainable.

Normally, this list is very short, because
the PV network and block protocols expect the backend to unmap the grant
first.

Normally the list is just empty. Only in very rare cases like premature
PV frontend module unloading it is expected to see cases of deferred
grant reclaims.

In the case of a system using only properly-designed PV protocols
implemented in kernel mode I agree.  However, both libxenvchan and the
Qubes GUI protocol are implemented in user mode and this means that if
the frontend process (the one that uses gntalloc) crashes, deferred
grant reclaims will occur.

This would be the user space variant of frontend driver unloading.

Worse, it is possible for the domain to use
the grant in a PV protocol.  If the PV backend driver then maps and
unmaps the grant and then tells the frontend driver to reclaim it, but
the backend userspace process (the one using gntdev) maps it before the
frontend does reclaim it, the frontend will think the backend is trying
to exploit XSA-396 and freeze the connection.

Let me sum up how I understand above statement:

1. Frontend F in DomA is granting DomB access via grant X for PV-device Y.
2. DomB backend B for PV-device Y is mapping the page using grant X and uses it.
3. DomB backend B unmaps grant X and signals end of usage to DomA frontend F.
4. DomB userspace process maps grant X
5. DomA frontend F tries to reclaim grant X and fails due to DomB userspace
   mapping

So why would DomB userspace process map grant X? This would imply a malicious
process in DomB. This might be possible, but I don't see how such an attack
would relate to your problem. It could happen with any malicious userspace
program with root access in a domain running a PV backend.


However, Qubes OS's GUI protocol is subject to the constraints
of the X Window System, and as such winds up with the frontend unmapping
the window first.  As a result, the list can grow very large, resulting
in a massive memory leak and eventual VM freeze.

I do understand that it is difficult to change the protocol and/or
behavior after the fact, or that performance considerations are in the
way of doing so.

Would the correct fix be to use IOCTL_GNTDEV_SET_UNMAP_NOTIFY?  That
would require that the agent either create a new event channel for each
window or maintain a pool of event channels, but that should be doable.

I think this sounds like a promising idea.

This still does not solve the problem of the frontend exiting
unexpectedly, though.

Such cases need to be handled via deferred reclaim. You could add a flag
to struct deferred_entry when called through gntalloc_vma_close(),
indicating that this is an expected deferred reclaim not leading to
error messages.


To partially solve this problem, make the number of entries that the VM
will attempt to free at each iteration tunable.  The default is still
10, but it can be overridden at compile-time (via Kconfig), boot-time
(via a kernel command-line option), or runtime (via sysfs).

Is there really a need to have another Kconfig option for this? AFAICS
only QubesOS is affected by the problem you are trying to solve. I don't
see why you can't use the command-line option or sysfs node to set the
higher reclaim batch size.

Fair.  In practice, Qubes OS will need to use the sysfs node, since
the other two do not work with in-VM kernels.

Fixes: 569ca5b3f94c ("xen/gnttab: add deferred freeing logic")

I don't think this "Fixes:" tag is appropriate. The mentioned commit didn't
have a bug. You are adding new functionality on top of it.

I’ll drop the "Fixes:" tag, but I will keep the "Cc: stable@xxxxxxxxxxxxxxx"
as I believe this patch meets the following criterion for stable
backport (from Documentation/process/stable-kernel-rules.rst):

     Serious issues as reported by a user of a distribution kernel may also
     be considered if they fix a notable performance or interactivity issue.


Sure, this seems appropriate.

Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
---
   drivers/xen/Kconfig       | 12 ++++++++++++
   drivers/xen/grant-table.c | 40 ++++++++++++++++++++++++++++-----------
   2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index d5d7c402b65112b8592ba10bd3fd1732c26b771e..8f96e1359eb102d6420775b66e7805004a4ce9fe 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -65,6 +65,18 @@ config XEN_MEMORY_HOTPLUG_LIMIT
   	  This value is used to allocate enough space in internal
   	  tables needed for physical memory administration.
+config XEN_GRANTS_RECLAIM_PER_ITERATION
+	int "Default number of grant entries to reclaim per iteration"
+	default 10
+	range 10 4294967295
+	help
+	  This sets the default value for the grant_table.free_per_iteration
+	  kernel command line option, which sets the number of grants that
+	  Linux will try to reclaim at once.  The default is 10, but
+	  workloads that make heavy use of gntalloc will likely want to
+	  increase this.  The current value can be accessed and/or modified
+	  via /sys/module/grant_table/parameters/free_per_iteration.
+

Apart from the fact that I don't like adding a new Kconfig option, the help
text is not telling the true story. Heavy use of gntalloc isn't to blame, but
the fact that your PV-device implementation is based on the reclaim
functionality. TBH, someone not familiar with the grant reclaim will have no
chance to select a sensible value based on your help text.

That’s a good point.  Qubes OS will need to use the sysfs value anyway
in order to support in-VM kernels, so the Kconfig option is not really
useful.

Nice.


   config XEN_SCRUB_PAGES_DEFAULT
   	bool "Scrub pages before returning them to system by default"
   	depends on XEN_BALLOON
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index e1ec725c2819d4d5dede063eb00d86a6d52944c0..fa666aa6abc3e786dddc94f895641505ec0b23d8 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -498,14 +498,20 @@ static LIST_HEAD(deferred_list);
   static void gnttab_handle_deferred(struct timer_list *);
   static DEFINE_TIMER(deferred_timer, gnttab_handle_deferred);
+static atomic64_t deferred_count;
+static atomic64_t leaked_count;
+static unsigned int free_per_iteration = CONFIG_XEN_GRANTS_RECLAIM_PER_ITERATION;
+
   static void gnttab_handle_deferred(struct timer_list *unused)
   {
-	unsigned int nr = 10;
+	unsigned int nr = READ_ONCE(free_per_iteration);
+	const bool ignore_limit = nr == 0;
   	struct deferred_entry *first = NULL;
   	unsigned long flags;
+	size_t freed = 0;
   	spin_lock_irqsave(&gnttab_list_lock, flags);
-	while (nr--) {
+	while ((ignore_limit || nr--) && !list_empty(&deferred_list)) {
   		struct deferred_entry *entry
   			= list_first_entry(&deferred_list,
   					   struct deferred_entry, list);
@@ -515,10 +521,13 @@ static void gnttab_handle_deferred(struct timer_list *unused)
   		list_del(&entry->list);
   		spin_unlock_irqrestore(&gnttab_list_lock, flags);
   		if (_gnttab_end_foreign_access_ref(entry->ref)) {
+			uint64_t ret = atomic64_sub_return(1, &deferred_count);
   			put_free_entry(entry->ref);
-			pr_debug("freeing g.e. %#x (pfn %#lx)\n",
-				 entry->ref, page_to_pfn(entry->page));
+			pr_debug("freeing g.e. %#x (pfn %#lx), %llu remaining\n",
+				 entry->ref, page_to_pfn(entry->page),
+				 (unsigned long long)ret);

I'd prefer not to issue lots of prints (being it debug or info ones) if the
reclaim is expected to happen with a specific PV-device.

My preferred solution would be a per-device flag, but at least you should
switch to pr_*_ratelimited(). Same applies further down.

What do you mean by “per-device flag”?  These grants are allocated by
userspace using gntalloc, so there is no PV device on which the flag
could be set.

In this case the flag should relate to the file pointer used for
gntalloc_open().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux