Re: [PATCH] hvc/xen: fix event channel handling for secondary consoles

[David Woodhouse]

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;
 	}

Attachment: smime.p7s
Description: S/MIME cryptographic signature