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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <55b87d75e13f1b888d90b03ea4bebe044b305e6a.camel@infradead.org>
Date:   Fri, 20 Oct 2023 16:20:20 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Juergen Gross <jgross@...e.com>,
        xen-devel <xen-devel@...ts.xenproject.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Roger Pau Monne <roger.pau@...rix.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Dawei Li <set_pte_at@...look.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org, Paul Durrant <paul@....org>
Subject: Re: [PATCH] hvc/xen: fix event channel handling for secondary
 consoles

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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)