Re: CVS commit: src/sys/kern

[Michael van Elst <mlelstv%serpens.de@localhost>]

On Tue, Aug 17, 2021 at 11:39:26PM +0000, Taylor R Campbell wrote:
> > 
> > Log Message:
> > skip symbol tables that were unloaded again to avoid EFAULT when reading
> > ksyms.
> > 
> > also restore TAILQ_FOREACH idiom.
> 
> This change isn't quite right: Reading past st = ksyms_last_snapshot
> in ksymsread, or in any context where we rely on the snapshot without
> holding ksyms_lock, is not allowed -- it will lead to a corrupted view
> of the snapshot, and possibly end up reading uninitialized memory.
> 
> That's why this code didn't use TAILQ_FOREACH -- it must stop at
> ksyms_last_snapshot; if it gets to the end of the list, and witnesses
> st = NULL, then that's a bug.

TAILQ_FOREACH just adds another exit condition that prevents entering
the loop with st = NULL.

A problem might be to continue the loop in case ksyms_last_snapshot itself
is gone.

Something like:

	if (st->sd_gone)
		goto skip;
	...
skip:
	if (st == ksyms_last_snapshot)
		break;

avoids that case.


> The code also didn't skip entries with st->sd_gone because we arrange
> for st->sd_nmap not to be freed until the last ksymsclose, at which
> point no more ksymsread can be active.

Keeping sd_nmap isn't sufficient, ksymsread failed with EFAULT as
you still derefence pointers to the unloaded module.
Skipping the unloaded module when reading the symbols avoids this.