svn commit: r285639 - head/sys/dev/e1000

[Bruce Evans]

On Thu, 16 Jul 2015, Sean Bruno wrote:

> Log:
>  Add an adapter CORE lock in the DDB hook em_dump_queue to avoid WITNESS
>  panic in em_init_locked() while debugging.

It is a bug to lock anything from within ddb.

Witness or something should panic when such a lock is attempted.

> Modified: head/sys/dev/e1000/if_em.c
> ==============================================================================
> --- head/sys/dev/e1000/if_em.c	Thu Jul 16 16:08:40 2015	(r285638)
> +++ head/sys/dev/e1000/if_em.c	Thu Jul 16 16:32:57 2015	(r285639)
> @@ -5998,7 +5998,9 @@ DB_COMMAND(em_reset_dev, em_ddb_reset_de

The primary bug is probably existence of this command.  You just can't
call arbitary code from within ddb.  It wanders into locks galore.  It
should detect this and panic, but is usually not that smart.  It is
more likely to deadlock.

The existence of the dump command is an even larger error.

The existence of the panic command is a smaller error.  Panic enters
a separate state, similar to ddb's but not as strong.  Panic is a last
resort, so deadlocks and recursive panics in it are acceptable.  It
has some defense against endless recursion.

> 		dev = devclass_get_device(dc, index);
> 		if (device_get_driver(dev) == &em_driver) {

New-bus calls should be locked by Giant or something like that.  This
is probably not done, and the bug is apparently not detected.

> 			struct adapter *adapter = device_get_softc(dev);
> +			EM_CORE_LOCK(adapter);
> 			em_init_locked(adapter);

A bug was previously detected here by an assertion that the lock is held.
Acquiring the lock breaks the the detection.

em's mutex is not recursive, so attempting to acquire it from within ddb
like this gives deadlock if it is already held.

In the previous version, suppose you turn off witness so that the bug is
detected.  Then em's mutext locking just doesn't work.

> +			EM_CORE_UNLOCK(adapter);
> 		}
> 	}
> }

I used checks like the following in a very old version to detect broken
locking calls from within ddb.  This had to be turned off because there
are too many broken callers:

X diff -u2 kern_mutex.c~ kern_mutex.c
X --- kern_mutex.c~	Wed Apr  7 20:39:12 2004
X +++ kern_mutex.c	Sat Feb  3 04:25:00 2007
X @@ -244,4 +246,9 @@
X  {
X 
X +#ifdef DDB1
X +	if (db_active)
X +		db_printf("_mtx_lock_flags: called with db_active @ %s:%d\n",
X +		    file, line);
X +#endif
X  	MPASS(curthread != NULL);
X  	KASSERT(m->mtx_object.lo_class == &lock_class_mtx_sleep,
X @@ -348,4 +355,10 @@
X  {
X 
X +#ifdef DDB1
X +	if (db_active)
X +		db_printf(
X +		    "_mtx_lock_spin_flags: called with db_active @ %s:%d\n",
X +		    file, line);
X +#endif
X  	MPASS(curthread != NULL);
X  	KASSERT(m->mtx_object.lo_class == &lock_class_mtx_spin,
X @@ -432,4 +445,9 @@
X  #endif
X 
X +#ifdef DDB1
X +	if (db_active)
X +		db_printf("_mtx_lock_sleep: called with db_active @ %s:%d\n",
X +		    file, line);
X +#endif
X  	if (mtx_owned(m)) {
X  		KASSERT((m->mtx_object.lo_flags & LO_RECURSABLE) != 0,
X @@ -568,4 +585,9 @@
X  	int i = 0;
X 
X +#ifdef DDB1
X +	if (db_active)
X +		db_printf("_mtx_lock_spin: called with db_active @ %s:%d\n",
X +		    file, line);
X +#endif
X  	if (LOCK_LOG_TEST(&m->mtx_object, opts))
X  		CTR1(KTR_LOCK, "_mtx_lock_spin: %p spinning", m);

The correct way to implement many ddb commands and all instances of call
commands from within ddb is to use a trampoline like gdb does in userland
(or just like signals are handled in userland).  ddb must be exited from
to run the command, and must regain control after running the command.

The dangers of calling arbitrary code are then more obvious.  The code
is then run without ddb's stopping of other CPUs and masking of
interrupts on the current CPU.  Sometimes that allows it to work, but
sometimes it just races more with other CPUs.  The code acts at it is
run by a trap handler that may be entered by any CPU at any address.

An intermediate mode where ddb exits but keeps other CPUs stopped and
interrupts masked might be useful but is hard to implement.  ddb is
too stupid to implement this even for trace traps.  Single stepping is
implemented by exiting ddb for every step (thus starting other CPUs,
etc., for every step) after setting the trace flag to arrange for a
trap after the next instruction.  It is the normal path of execution
that is exited to, so this is relatively safe, but ddb still loses
all control and other threads and interrupts may change the state that
you are trying to see changed 1 step at a time.

Bruce