Re: CVS commit: src/sys/dev/dkwedge

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Date: Tue, 11 Apr 2023 17:21:17 +0200
> From: Michael van Elst <mlelstv%serpens.de@localhost>
> 
> On Tue, Apr 11, 2023 at 09:07:49AM +0000, Taylor R Campbell wrote:
> > 
> > (a) how we can legitimately enter a state where the assertions are
> >     violated, and
> 
> dklastclose is called when the close operation ends in no more openers.
> There is nothing that guarantees that any open was successful before
> with the effect that dk_rawopens is > 0 and dk_rawvp is not NULL.
> 
> In that state then decrementing dk_rawopens beyond zero will make
> dklastclose do the right thing: nothing.

How can that be?  If dkfirstopen fails, then dkopen won't set any bits
in sc->sc_dk.dk_openmask, so dkclose won't call dklastclose.


/* dkopen */
	if (sc->sc_dk.dk_openmask == 0) {
		error = dkfirstopen(sc, flags);
		if (error)
			goto popen_fail;
	}
...
	if (fmt == S_IFCHR)
		sc->sc_dk.dk_copenmask |= 1;
	else
		sc->sc_dk.dk_bopenmask |= 1;
	sc->sc_dk.dk_openmask =
	    sc->sc_dk.dk_copenmask | sc->sc_dk.dk_bopenmask;

 popen_fail:
	mutex_exit(&sc->sc_parent->dk_rawlock);
	mutex_exit(&sc->sc_dk.dk_openlock);
	return (error);


/* dkclose */
	KASSERT(sc->sc_dk.dk_openmask != 0);

	if (fmt == S_IFCHR)
		sc->sc_dk.dk_copenmask &= ~1;
	else
		sc->sc_dk.dk_bopenmask &= ~1;
	sc->sc_dk.dk_openmask =
	    sc->sc_dk.dk_copenmask | sc->sc_dk.dk_bopenmask;

	if (sc->sc_dk.dk_openmask == 0) {
		dklastclose(sc);
	}


> When you want to check for overflows of dk_rawopens (which is difficult
> to overflow as you had to create 2^32 wedges) you need to watch it being
> incremented (also temporarily). Crashing after the fact with an assertion
> in dklastclose doesn't help.

I meant the overflow of --dk_rawopens if dk_rawopens=0 (in which case
dk_rawvp=NULL), which would cause dk_rawopens=UINT_MAX (and then crash
soon after because dk_close_parent(NULL, ...) will trip on a null
pointer dereference).

That said: you're right, we should make dkopen fail gracefully if
dk_rawopens is already UINT_MAX.