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.
