Re: kern/60760: lpt(4) concurrency issues

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Wed, 16 Sep 2026 08:28:17 +0200
> From: Johann Höpfner <hoepf%cit.tum.de@localhost>
> 
> A bug that I recently found in OpenBSD sys/dev/ic/lpt.c applies to
> NetBSD and FreeBSD too.
> 
> In short lptwrite reuses the same state struct under concurrent write
> syscalls. [...]

Thanks, I filed a problem report in gnats to track this:

PR kern/60760: lpt(4) concurrency issues
https://gnats.NetBSD.org/60760

(Followups to this message, to gnats-bugs%NetBSD.org@localhost with subject line
`Re: kern/60760: lpt(4) concurrency issues', will get appended to the
PR, and if you want, I can add you to the notify-list.)

> My prefered fix would be to only keep iterator context per-syscall
> execution and not use the per-fd struct lpt_softc *sc there, but the
> solution we settled on in OpenBSD was to simply initialize the buffer
> and make sure that we only read from inside it. The output may still
> skip over bytes or output zeros on concurrent writes to the same fd, but
> wont impact the kernel.

It's not clear to me that there is any value in allowing concurrent
lptwrite calls at all.

So rather than try to mitigate the damage of interleaving internal
data structure updates like that, I'm inclined to just set a flag
sc->sc_state |= LPT_WRITING while the first lptwrite is in progress,
to block subsequent attempts to lptwrite until the first one is done.

There are other issues with the driver too: in thread context, the
state (sc_state, sc_count, sc_cp, &c.) is not protected from changes
by the interrupt handler, so it likely needs various spltty/splx
sprinkled in too.  (Ideally, we would rewrite this driver with
mutex/condvar(9) and make it MP-safe, but that involves some
nontrivial testing on ancient platforms -- this approach of just
sprinkling some spltty/splx and a lock flag is much lower-risk.)

Draft attached, compile-tested only so far.

# HG changeset patch
# User Taylor R Campbell <riastradh%NetBSD.org@localhost>
# Date 1790029347 0
#      Mon Sep 21 22:22:27 2026 +0000
# Branch trunk
# Node ID de49d4446a053906cf48f54d3855d454b392873f
# Parent  c96fe1ab6a49c6b24bccc49f7ccb16b325ab0468
# EXP-Topic riastradh-pr60760-lptfixes
WIP: lpt(4): Fix concurrency and interrupt safety issues.

PR kern/60760: lpt(4) concurrency issues

diff -r c96fe1ab6a49 -r de49d4446a05 sys/dev/ic/lpt.c
--- a/sys/dev/ic/lpt.c	Sun Sep 20 03:58:19 2026 +0000
+++ b/sys/dev/ic/lpt.c	Mon Sep 21 22:22:27 2026 +0000
@@ -158,6 +158,7 @@ lptopen(dev_t dev, int flag, int mode, s
 	u_char control;
 	int error;
 	int spin;
+	int s;
 
 	sc = device_lookup_private(&lpt_cd, LPTUNIT(dev));
 	if (!sc || !sc->sc_dev_ok)
@@ -174,8 +175,12 @@ lptopen(dev_t dev, int flag, int mode, s
 		    sc->sc_state);
 #endif
 
-	if (sc->sc_state)
-		return EBUSY;
+	s = spltty();
+
+	if (sc->sc_state) {
+		error = EBUSY;
+		goto out;
+	}
 
 	sc->sc_state = LPT_INIT;
 	sc->sc_flags = flags;
@@ -197,14 +202,15 @@ lptopen(dev_t dev, int flag, int mode, s
 	for (spin = 0; NOT_READY_ERR(); spin += STEP) {
 		if (spin >= TIMEOUT) {
 			sc->sc_state = 0;
-			return EBUSY;
+			error = EBUSY;
+			goto out;
 		}
 
 		/* wait 1/4 second, give up if we get a signal */
 		error = tsleep((void *)sc, LPTPRI | PCATCH, "lptopen", STEP);
 		if (error != EWOULDBLOCK) {
 			sc->sc_state = 0;
-			return error;
+			goto out;
 		}
 	}
 
@@ -216,6 +222,7 @@ lptopen(dev_t dev, int flag, int mode, s
 	bus_space_write_1(iot, ioh, lpt_control, control);
 
 	sc->sc_inbuf = malloc(LPT_BSIZE, M_DEVBUF, M_WAITOK);
+	sc->sc_cp = NULL;
 	sc->sc_count = 0;
 	sc->sc_state = LPT_OPEN;
 
@@ -223,7 +230,10 @@ lptopen(dev_t dev, int flag, int mode, s
 		lptwakeup(sc);
 
 	LPRINTF(("%s: opened\n", device_xname(sc->sc_dev)));
-	return 0;
+	error = 0;
+
+out:	splx(s);
+	return error;
 }
 
 int
@@ -274,9 +284,12 @@ lptclose(dev_t dev, int flag, int mode,
 	    device_lookup_private(&lpt_cd, LPTUNIT(dev));
 	bus_space_tag_t iot = sc->sc_iot;
 	bus_space_handle_t ioh = sc->sc_ioh;
+	int s;
 
+	s = spltty();
 	if (sc->sc_count)
 		(void) lptpushbytes(sc);
+	sc->sc_count = 0;
 
 	if ((sc->sc_flags & LPT_NOINTR) == 0)
 		callout_stop(&sc->sc_wakeup_ch);
@@ -285,6 +298,8 @@ lptclose(dev_t dev, int flag, int mode,
 	sc->sc_state = 0;
 	bus_space_write_1(iot, ioh, lpt_control, LPC_NINIT);
 	free(sc->sc_inbuf, M_DEVBUF);
+	sc->sc_cp = sc->sc_inbuf = NULL;
+	splx(s);
 
 	LPRINTF(("%s: closed\n", device_xname(sc->sc_dev)));
 	return 0;
@@ -367,8 +382,19 @@ lptwrite(dev_t dev, struct uio *uio, int
 	struct lpt_softc *sc =
 	    device_lookup_private(&lpt_cd, LPTUNIT(dev));
 	size_t n;
+	int s;
+	bool locked = false;
 	int error = 0;
 
+	s = spltty();
+	while (sc->sc_state & LPT_WRITING) {
+		error = tsleep(&sc->sc_state, LPTPRI | PCATCH, "lptwrite", 0);
+		if (error)
+			goto out;
+	}
+	sc->sc_state |= LPT_WRITING;
+	locked = true;
+
 	while ((n = uimin(LPT_BSIZE, uio->uio_resid)) != 0) {
 		uiomove(sc->sc_cp = sc->sc_inbuf, n, uio);
 		sc->sc_count = n;
@@ -380,9 +406,16 @@ lptwrite(dev_t dev, struct uio *uio, int
 			 */
 			uio->uio_resid += sc->sc_count;
 			sc->sc_count = 0;
-			return error;
+			goto out;
 		}
 	}
+	error = 0;
+
+out:	if (locked) {
+		sc->sc_state &= LPT_WRITING;
+		wakeup(&sc->sc_state);
+	}
+	splx(s);
 	return 0;
 }
 
diff -r c96fe1ab6a49 -r de49d4446a05 sys/dev/ic/lptvar.h
--- a/sys/dev/ic/lptvar.h	Sun Sep 20 03:58:19 2026 +0000
+++ b/sys/dev/ic/lptvar.h	Mon Sep 21 22:22:27 2026 +0000
@@ -74,6 +74,7 @@ struct lpt_softc {
 #define	LPT_OPEN	0x01	/* device is open */
 #define	LPT_OBUSY	0x02	/* printer is busy doing output */
 #define	LPT_INIT	0x04	/* waiting to initialize for open */
+#define	LPT_WRITING	0x08	/* write in progress */
 	u_char sc_flags;
 #define	LPT_AUTOLF	0x20	/* automatic LF on CR */
 #define	LPT_NOPRIME	0x40	/* don't prime on open */