NetBSD Problem Report #60760

From www@netbsd.org  Mon Sep 21 22:11:54 2026
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.netbsd.org", Issuer "YR1" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 6D6381A923B
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 21 Sep 2026 22:11:54 +0000 (UTC)
Message-Id: <20260921221153.1EE9B1A923C@mollari.NetBSD.org>
Date: Mon, 21 Sep 2026 22:11:53 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: lpt(4) concurrency issues
X-Send-Pr-Version: www-1.0
X-From4GNATS: "campbell+netbsd@mumble.net via gnats" <gnats-admin@NetBSD.org>

>Number:         60760
>Notify-List:    hoepf@cit.tum.de
>Category:       kern
>Synopsis:       lpt(4) concurrency issues
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          needs-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Sep 21 22:15:00 +0000 2026
>Closed-Date:    
>Last-Modified:  Thu Oct 08 15:56:36 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current, 11, 10, ...
>Organization:
Acme BSD Line Printers, Inc.
>Environment:
>Description:

	lpt(4) is a small but grody legacy driver that is not MP-safe.
	Not being MP-safe would be fine, but it's also buggy even in
	the giant-locked model of kernel computation:

	1. Access to variables like sc->sc_state and sc->sc_count is
	   done from both thread context, e.g. in
	   lptopen/lptclose/lptwrite, and in interrupt context, in
	   lptintr, which runs in various platforms at IPL_TTY.

	   But this access is not protected with spltty.  So the
	   interrupt handler might screw up the thread logic.

	2. Nothing prevents concurrent writes from being issued,
	   interleaving their results, and likely overrunning kernel
	   buffers.  Reported by Johann Höpfner on tech-kern:
	   https://mail-index.NetBSD.org/tech-kern/2026/09/16/msg031252.html

>How-To-Repeat:

	code inspection

>Fix:

	Ideally we would rewrite this driver to be MP-safe, but I have
	no idea how to test it seriously, so I'm inclined to apply
	minimal changes to (a) sprinkle spltty and (b) serialize
	lptwrite calls with tsleep and a lock flag.

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: Johann =?utf-8?Q?H=C3=B6pfner?= <hoepf@cit.tum.de>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
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>
 >=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 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>
 # 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--

From: Johann =?utf-8?Q?H=C3=B6pfner?= <hoepf@cit.tum.de>
To: Taylor R Campbell <riastradh@netbsd.org>
Cc: gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/60760: lpt(4) concurrency issues
Date: Thu, 24 Sep 2026 15:18:21 +0200

 > 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 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.)

 Feel free to do that.

 > 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.

 I agree. If someone relied on writing to the same port concurrently they
 would have probably noticed the mangled output by now. Especially since,
 now that I think of it, the bug should get worse with slower printers.

 Two things, without further looking at the patch:

 > +out:	if (locked) {
 > +		sc->sc_state &= LPT_WRITING;
 > +		wakeup(&sc->sc_state);
 > +	}
 > +	splx(s);

 I believe you want to clear LPT_WRITING here. You are clearing all other
 bits though.

 >  	return 0;

 Maybe return error here instead of 0.

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Johann =?utf-8?Q?H=C3=B6pfner?= <hoepf@cit.tum.de>
Cc: gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/60760: lpt(4) concurrency issues
Date: Thu, 8 Oct 2026 15:54:06 +0000

 > Date: Thu, 24 Sep 2026 15:18:21 +0200
 > From: Johann H=C3=B6pfner <hoepf@cit.tum.de>
 >=20
 > > +out:	if (locked) {
 > > +		sc->sc_state &=3D LPT_WRITING;
 > > +		wakeup(&sc->sc_state);
 > > +	}
 > > +	splx(s);
 >=20
 > I believe you want to clear LPT_WRITING here. You are clearing all other
 > bits though.

 You would believe correctly.  Oops.

 > >  	return 0;
 >=20
 > Maybe return error here instead of 0.

 Yes, that sounds like a good idea!

 Thanks for the review!

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/60760 CVS commit: src/sys/dev/ic
Date: Thu, 8 Oct 2026 15:54:35 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Thu Oct  8 15:54:35 UTC 2026

 Modified Files:
 	src/sys/dev/ic: lpt.c lptvar.h

 Log Message:
 lpt(4): Fix concurrency and interrupt safety issues.

 PR kern/60760: lpt(4) concurrency issues


 To generate a diff of this commit:
 cvs rdiff -u -r1.82 -r1.83 src/sys/dev/ic/lpt.c
 cvs rdiff -u -r1.56 -r1.57 src/sys/dev/ic/lptvar.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 08 Oct 2026 15:56:36 +0000
State-Changed-Why:
fixed in HEAD, needs pullup-11, pullup-10


>Unformatted:

Home	PR Database Search