hvc/xen: fix event channel handling for secondary consoles

  From: David Woodhouse <dwmw@amazon.co.uk>

The xencons_connect_backend() function allocates a local interdomain
event channel with xenbus_alloc_evtchn(), then calls
bind_interdomain_evtchn_to_irq_lateeoi() to bind to that port# on the
*remote* domain.

That doesn't work very well:

(qemu) device_add xen-console,id=con1,chardev=pty0
[   44.323872] xenconsole console-1: 2 xenbus_dev_probe on device/console/1
[   44.323995] xenconsole: probe of console-1 failed with error -2

Fix it to use bind_evtchn_to_irq_lateeoi(), which does the right thing
by just binding that *local* event channel to an irq. The backend will
do the interdomain binding.

This didn't affect the primary console because the setup for that is
special — the toolstack allocates the guest event channel and the guest
discovers it with HVMOP_get_param.

Once that's fixed, there's also a warning on hot-unplug because
xencons_disconnect_backend() unconditionally calls free_irq() via
unbind_from_irqhandler():

(qemu) device_del con1
[   32.050919] ------------[ cut here ]------------
[   32.050942] Trying to free already-free IRQ 33
[   32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330

Fix that by calling notifier_del_irq() first, which only calls
free_irq() if the irq was requested in the first place. Then use
evtchn_put() to release the irq and event channel. Avoid calling
xenbus_free_evtchn() in the normal case, as evtchn_put() will do that
too. The only time xenbus_free_evtchn() needs to be called is for the
cleanup when bind_evtchn_to_irq_lateeoi() fails.

Finally, fix the error path in xen_hvc_init() when there's no primary
console. It should still register the frontend driver, as there may be
secondary consoles. (Qemu can always add secondary consoles, but only
the toolstack can add the primary because it's special.)

Fixes: fe415186b4 ("xen/console: harden hvc_xen against event channel storms")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Cc: stable@vger.kernel.org
---

Untested on actual Xen.

 drivers/tty/hvc/hvc_xen.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
  

On 18.10.23 01:46, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> The xencons_connect_backend() function allocates a local interdomain
> event channel with xenbus_alloc_evtchn(), then calls
> bind_interdomain_evtchn_to_irq_lateeoi() to bind to that port# on the
> *remote* domain.
> 
> That doesn't work very well:
> 
> (qemu) device_add xen-console,id=con1,chardev=pty0
> [   44.323872] xenconsole console-1: 2 xenbus_dev_probe on device/console/1
> [   44.323995] xenconsole: probe of console-1 failed with error -2
> 
> Fix it to use bind_evtchn_to_irq_lateeoi(), which does the right thing
> by just binding that *local* event channel to an irq. The backend will
> do the interdomain binding.
> 
> This didn't affect the primary console because the setup for that is
> special — the toolstack allocates the guest event channel and the guest
> discovers it with HVMOP_get_param.
> 
> Once that's fixed, there's also a warning on hot-unplug because
> xencons_disconnect_backend() unconditionally calls free_irq() via
> unbind_from_irqhandler():
> 
> (qemu) device_del con1
> [   32.050919] ------------[ cut here ]------------
> [   32.050942] Trying to free already-free IRQ 33
> [   32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330
> 
> Fix that by calling notifier_del_irq() first, which only calls
> free_irq() if the irq was requested in the first place. Then use

I don't think the "if the irq was requested in the first place" is the correct
reasoning.

I think the problem is that notifier_del_irq() will be called another time
through the .notifier_del hook. Two calls of notifier_del_irq() are fine, but
one call of it and another call of free_irq() via unbind_from_irqhandler() is
a problem.

> evtchn_put() to release the irq and event channel. Avoid calling
> xenbus_free_evtchn() in the normal case, as evtchn_put() will do that
> too. The only time xenbus_free_evtchn() needs to be called is for the
> cleanup when bind_evtchn_to_irq_lateeoi() fails.
> 
> Finally, fix the error path in xen_hvc_init() when there's no primary
> console. It should still register the frontend driver, as there may be
> secondary consoles. (Qemu can always add secondary consoles, but only
> the toolstack can add the primary because it's special.)
> 
> Fixes: fe415186b4 ("xen/console: harden hvc_xen against event channel storms")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Cc: stable@vger.kernel.org

With above fixed in the commit message:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
  

On Fri, 2023-10-20 at 10:51 +0200, Juergen Gross wrote:
> 
> > (qemu) device_del con1
> > [   32.050919] ------------[ cut here ]------------
> > [   32.050942] Trying to free already-free IRQ 33
> > [   32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330
> > 
> > Fix that by calling notifier_del_irq() first, which only calls
> > free_irq() if the irq was requested in the first place. Then use
> 
> I don't think the "if the irq was requested in the first place" is the correct
> reasoning.
> 
> I think the problem is that notifier_del_irq() will be called another time
> through the .notifier_del hook. Two calls of notifier_del_irq() are fine, but
> one call of it and another call of free_irq() via unbind_from_irqhandler() is
> a problem.

Er... yes, the HVC tty device still exists, can still be open and in
use by userspace during the time that xencons_disconnect_backend() is
running.

Why does xencons_disconnect_backend() do all the evtchn and gnttab
teardown *first* and then only call hvc_remove() at the end. That seems
weird.

So if I do 'dd if=/dev/zero of=/dev/hvc1' while I remove the device
from qemu... yep, that seems to have filled the ring after the evtchn
was torn down, and it's waiting for ever in domU_write_console().

In fact, that isn't *even* because of the race within
xencons_disconnect_backend(). In xencons_backend_changed() when the
backend goes into state XenbusStateClos{ing,ed} for the disconnect, we
just set the frontend state directly to XenbusStateClosed without even
*calling* xencons_disconnect_backend().

So we were *told* of the impending unplug. We fail to actually stop
using the device, but we tell the backend that it's OK to go away.

Oops :)

The incremental patch below makes it work if I unplug a console while
writing /dev/zero to it.

But I suspect instead of calling xencons_disconnect_backend() when the
backend changes to XenbusStateClos{ing,ed}, it should actually *just*
do the hvc_close() part? The evtchn and gnttab cleanup should wait
until the backend has actually finished closing?

And what locking is there around xencons_disconnect_backend() anyway?
Do we rely on the fact that it can all only happen from the xenstore
watch thread?

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index f0376612b267..0806078835f6 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -377,8 +377,10 @@ void xen_console_resume(void)
 #ifdef CONFIG_HVC_XEN_FRONTEND
 static void xencons_disconnect_backend(struct xencons_info *info)
 {
+	if (info->hvc != NULL)
+		hvc_remove(info->hvc);
+	info->hvc = NULL;
 	if (info->irq > 0) {
-		notifier_del_irq(info->hvc, info->irq); 
 		evtchn_put(info->evtchn);
 		info->irq = 0;
 		info->evtchn = 0;
@@ -390,9 +392,6 @@ static void xencons_disconnect_backend(struct xencons_info *info)
 	if (info->gntref > 0)
 		gnttab_free_grant_references(info->gntref);
 	info->gntref = 0;
-	if (info->hvc != NULL)
-		hvc_remove(info->hvc);
-	info->hvc = NULL;
 }
 
 static void xencons_free(struct xencons_info *info)
@@ -558,6 +557,7 @@ static void xencons_backend_changed(struct xenbus_device *dev,
 			break;
 		fallthrough;	/* Missed the backend's CLOSING state */
 	case XenbusStateClosing:
+		xencons_disconnect_backend(dev_get_drvdata(&dev->dev));
 		xenbus_frontend_closed(dev);
 		break;
 	}