NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/60760: lpt(4) concurrency issues
The following reply was made to PR kern/60760; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Johann =?utf-8?Q?H=C3=B6pfner?= <hoepf%cit.tum.de@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/60760: lpt(4) concurrency issues
Date: Mon, 21 Sep 2026 22:30:50 +0000
This is a multi-part message in MIME format.
--=_C6YHulfOHmGeJviPQ9de6kWxQAj6ZXS1
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
> Date: Wed, 16 Sep 2026 08:28:17 +0200
> From: Johann H=C3=B6pfner <hoepf%cit.tum.de@localhost>
>=20
> A bug that I recently found in OpenBSD sys/dev/ic/lpt.c applies to
> NetBSD and FreeBSD too.
>=20
> 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 |=3D 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.
--=_C6YHulfOHmGeJviPQ9de6kWxQAj6ZXS1
Content-Type: text/plain; charset="ISO-8859-1"; name="pr60760-lptfixes"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr60760-lptfixes.patch"
# 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;
=20
sc =3D 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
=20
- if (sc->sc_state)
- return EBUSY;
+ s =3D spltty();
+
+ if (sc->sc_state) {
+ error =3D EBUSY;
+ goto out;
+ }
=20
sc->sc_state =3D LPT_INIT;
sc->sc_flags =3D flags;
@@ -197,14 +202,15 @@ lptopen(dev_t dev, int flag, int mode, s
for (spin =3D 0; NOT_READY_ERR(); spin +=3D STEP) {
if (spin >=3D TIMEOUT) {
sc->sc_state =3D 0;
- return EBUSY;
+ error =3D EBUSY;
+ goto out;
}
=20
/* wait 1/4 second, give up if we get a signal */
error =3D tsleep((void *)sc, LPTPRI | PCATCH, "lptopen", STEP);
if (error !=3D EWOULDBLOCK) {
sc->sc_state =3D 0;
- return error;
+ goto out;
}
}
=20
@@ -216,6 +222,7 @@ lptopen(dev_t dev, int flag, int mode, s
bus_space_write_1(iot, ioh, lpt_control, control);
=20
sc->sc_inbuf =3D malloc(LPT_BSIZE, M_DEVBUF, M_WAITOK);
+ sc->sc_cp =3D NULL;
sc->sc_count =3D 0;
sc->sc_state =3D LPT_OPEN;
=20
@@ -223,7 +230,10 @@ lptopen(dev_t dev, int flag, int mode, s
lptwakeup(sc);
=20
LPRINTF(("%s: opened\n", device_xname(sc->sc_dev)));
- return 0;
+ error =3D 0;
+
+out: splx(s);
+ return error;
}
=20
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 =3D sc->sc_iot;
bus_space_handle_t ioh =3D sc->sc_ioh;
+ int s;
=20
+ s =3D spltty();
if (sc->sc_count)
(void) lptpushbytes(sc);
+ sc->sc_count =3D 0;
=20
if ((sc->sc_flags & LPT_NOINTR) =3D=3D 0)
callout_stop(&sc->sc_wakeup_ch);
@@ -285,6 +298,8 @@ lptclose(dev_t dev, int flag, int mode,
sc->sc_state =3D 0;
bus_space_write_1(iot, ioh, lpt_control, LPC_NINIT);
free(sc->sc_inbuf, M_DEVBUF);
+ sc->sc_cp =3D sc->sc_inbuf =3D NULL;
+ splx(s);
=20
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 =3D
device_lookup_private(&lpt_cd, LPTUNIT(dev));
size_t n;
+ int s;
+ bool locked =3D false;
int error =3D 0;
=20
+ s =3D spltty();
+ while (sc->sc_state & LPT_WRITING) {
+ error =3D tsleep(&sc->sc_state, LPTPRI | PCATCH, "lptwrite", 0);
+ if (error)
+ goto out;
+ }
+ sc->sc_state |=3D LPT_WRITING;
+ locked =3D true;
+
while ((n =3D uimin(LPT_BSIZE, uio->uio_resid)) !=3D 0) {
uiomove(sc->sc_cp =3D sc->sc_inbuf, n, uio);
sc->sc_count =3D n;
@@ -380,9 +406,16 @@ lptwrite(dev_t dev, struct uio *uio, int
*/
uio->uio_resid +=3D sc->sc_count;
sc->sc_count =3D 0;
- return error;
+ goto out;
}
}
+ error =3D 0;
+
+out: if (locked) {
+ sc->sc_state &=3D LPT_WRITING;
+ wakeup(&sc->sc_state);
+ }
+ splx(s);
return 0;
}
=20
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 */
--=_C6YHulfOHmGeJviPQ9de6kWxQAj6ZXS1--
Home |
Main Index |
Thread Index |
Old Index