Dear Maintainer,

* What is the problem:
Issuing the commands stop followed by go, at the input of the uml_mconsole
client, results in the client becoming blocked on read socket. This is
because
of logic in arch/um/drivers/mconsole_kern.c, where mconsole_stop() doesn't
reactivate the MCONSOLE_IRQ before the function has exited.

I've managed to find a fix which seems to be working, but I don't know if
it's
a proper fix. Please see the attached file.

I failed to mention that the block on read occurs by sending anything
through uml_mconsole, after the go command has been processed.

Hello Mihai,

In this case, it is good to have the User Mode Linux upstream in the
loop.

Thanks,
Ritesh

--- linux-source-5.16/arch/um/drivers/mconsole_kern.c	2022-02-05 20:22:06.000000000 +0200
+++ linux-source-5.16.fix/arch/um/drivers/mconsole_kern.c	2022-02-16 23:35:39.562668086 +0200
@@ -224,6 +224,7 @@

 void mconsole_stop(struct mc_request *req)
 {
+	int err;
 	deactivate_fd(req->originating_fd, MCONSOLE_IRQ);
 	os_set_fd_block(req->originating_fd, 1);
 	mconsole_reply(req, "stopped", 0, 0);
@@ -247,6 +248,11 @@
 	}
 	os_set_fd_block(req->originating_fd, 0);
 	mconsole_reply(req, "", 0, 0);
+	err=activate_fd(MCONSOLE_IRQ, req->originating_fd, IRQ_READ,
+		(void*)(req->originating_fd), NULL);
+	if (err)
+	  mconsole_reply(req, "Failed to reactivate MCONSOLE_IRQ, \
+			this will block the read for uml_mconsole", 1, 0);
 }

 static DEFINE_SPINLOCK(mc_devices_lock);
--- linux-source-5.16/arch/um/kernel/irq.c	2022-02-05 20:22:06.000000000 +0200
+++ linux-source-5.16.fix/arch/um/kernel/irq.c	2022-02-16 23:39:15.650279367 +0200
@@ -249,7 +249,7 @@
 		free_irq_entry(entry, false);
 }

Hi Ritesh, hi Mihai,

Apologies for the delay in the answer, I was traveling last week.

1. Your patch will not achieve the desired aim. The IRQS in UML nowadays are per fd and looping inside the IRQ handler for mconsole will not stop UML as it used to when it was using the old poll() based IRQ subsystem. It will still handle other IRQs. That is a bug, we need to see what can be done here.

2. Otherwise, just a - on the deactivate_fd() would do the trick. There is a reentrancy check on the IRQ handler and while you are looping inside it, the same IRQ will not be triggered again. No need to deactivate_fd(). However, as per "1", this is insufficient - all other IRQS will still be handled.

Brgds,

A.

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

Moving to an EPOLL based IRQ controller broke uml_mconsole stop/go
commands. This fixes it and restores stop/go functionality.

Fixes: ff6a17989c08b0bb0fd490cc500b084581b3a9b9 Epoll based IRQ controller
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/drivers/mconsole_kern.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 6ead1e240457..8ca67a692683 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -224,7 +224,7 @@ void mconsole_go(struct mc_request *req)

 void mconsole_stop(struct mc_request *req)
 {
-	deactivate_fd(req->originating_fd, MCONSOLE_IRQ);
+	block_signals();
 	os_set_fd_block(req->originating_fd, 1);
 	mconsole_reply(req, "stopped", 0, 0);
 	for (;;) {
@@ -247,6 +247,7 @@ void mconsole_stop(struct mc_request *req)
 	}
 	os_set_fd_block(req->originating_fd, 0);
 	mconsole_reply(req, "", 0, 0);
+	unblock_signals();
 }

 static DEFINE_SPINLOCK(mc_devices_lock);

Ack, will resubmit shortly.

The old poll controller had all IO IRQs shared and disabled IRQ processing while in the IRQ loop. Thus a while(;;) in the IRQ loop combined with a blocking read was an effective way to stop processing.

That is no longer the case.

1. While individual IRQs are not reentrant (there is a check for that in the IRQ handler), other IRQs will be processed and each FD is allocated a separate one. So looping inside one will not stop the kernel. It will still handle timer IRQs and other IO.

2. In the old controller disable_fd() was the reentrance guard. It removed the fd from the poll set so that it is not triggered again until the IRQ is handled. It was used everywhere in the beginning of each handler (followed by re-enable at IRQ exit). It has different semantics, cost and should not be used without need in the epoll case. In fact, I removed it throughout, but somehow missed the mconsole. Still having it was a bug.

As we do not have any means to shut-off the IRQs in the IRQ controller itself, the easiest way to stop them is to kill signals - as per the patch.
-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

Fixes: ff6a17989c08 ("Epoll based IRQ controller")

Don't think I can comment on the patch itself, sorry.

johannes

From: Anton Ivanov <anton.ivanov@cambridgegreys.com>

Moving to an EPOLL based IRQ controller broke uml_mconsole stop/go
commands. This fixes it and restores stop/go functionality.

Fixes: ff6a17989c08 ("Epoll based IRQ controller")
Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
---
 arch/um/drivers/mconsole_kern.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index 6ead1e240457..8ca67a692683 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -224,7 +224,7 @@ void mconsole_go(struct mc_request *req)

 void mconsole_stop(struct mc_request *req)
 {
-	deactivate_fd(req->originating_fd, MCONSOLE_IRQ);
+	block_signals();
 	os_set_fd_block(req->originating_fd, 1);
 	mconsole_reply(req, "stopped", 0, 0);
 	for (;;) {
@@ -247,6 +247,7 @@ void mconsole_stop(struct mc_request *req)
 	}
 	os_set_fd_block(req->originating_fd, 0);
 	mconsole_reply(req, "", 0, 0);
+	unblock_signals();
 }

 static DEFINE_SPINLOCK(mc_devices_lock);