Re: Instant panic while trying run ports-mgmt/poudriere

On Thu, Aug 27, 2015 at 10:21:47AM +0300, Andriy Gapon wrote:
> On 27/08/2015 02:36, John-Mark Gurney wrote:
> > We should/cannot get here w/ an empty list.  If we do, then there is
> > something seriously wrong...  The current kn (which we must have as we
> > are here) MUST be on the list, but as you just showed, there are no
> > knotes on the list.
> > 
> > Can you get me a print of the knote?  That way I can see what flags
> > are on it?
> 
> Apologies if the following might sound a little bit patronizing, but it
> seems that you have got all the facts correctly, but somehow the
> connection between them did not become clear.
> 
> So:
> 1. The list originally is NOT empty.  I guess that it has one entry, but
> that's an unimportant detail.
> 2. This is why the loop is entered. It's a fact that it is entered.
> 3. The list becomes empty precisely because the entry is removed during
> the iteration in the loop (as kib has explained).  It's a fact that the
> list became empty at least in the panic that I reported.
The only detail I can add to this explanation, which is probably third (?)
time, is that the removal is done in the filt_proc() event method, by
the call to knlist_remove_inevent().

> 4. The element is not only unlinked from the list, but its memory is
> also freed.
> 5. That's why we have the use after free: SLIST_FOREACH is trying to get
> a pointer to a next element from the freed memory.
> 6. This is why the commit for trashing the freed memory made all the
> difference: previously the freed memory was unlikely to be re-used /
> modified, so the use-after-free had a high chance of succeeding.  It's a
> fact that in my panic there was an attempt to dereference a trashed pointer.
> 7. Finally, this is why SLIST_FOREACH_SAFE helps here: we stash the
> pointer to the next element beforehand and, thus, we do not access the
> freed memory.
The additional, eighth item, should explain why the change to _SAFE() is
the correct fix, and not just a papering over the problem. Nobody except
the current thread can modify the knlist, because knlist is locked. As
a consequence, only the current element can be unlinked and removed. So
the _SAFE() iterator actually work.

> 
> Please let me know if you see any fault in above reasoning or if
> something is still no clear.