Date:   Thu, 6 Jul 2023 11:28:23 +0100
From:   "Richard W.M. Jones" <rjones@...hat.com>
To:     Richard Henderson <richard.henderson@...aro.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        Anders Roxell <anders.roxell@...aro.org>,
        Daniel Díaz <daniel.diaz@...aro.org>,
        Benjamin Copeland <ben.copeland@...aro.org>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: qemu-x86_64 booting with 8.0.0 stil see int3: when running LTP
 tracing testing.

On Thu, Jul 06, 2023 at 07:30:50AM +0100, Richard Henderson wrote:
> On 7/5/23 22:50, Richard W.M. Jones wrote:
> >tb_invalidate_phys_range_fast() *is* called, and we end up calling
> >   tb_invalidate_phys_page_range__locked ->
> >     tb_phys_invalidate__locked ->
> >       do_tb_phys_invalidate
> >
> >Nevertheless the old TB (containing the call to the int3 helper) is
> >still called after the code has been replaced with a NOP.
> >
> >Of course there are 4 MTTCG threads so maybe another thread is in the
> >middle of executing the same TB when it gets invalidated.
> 
> Yes.
> 
> >tb_invalidate_phys_page_range__locked goes to some effort to check if
> >the current TB is being invalidated and restart the TB, but as far as
> >I can see the test can only work for the current core, and won't
> >restart the TB on other cores.
> 
> Yes.
> 
> The assumption with any of these sorts of races is that it is "as
> if" the other thread has already passed the location of the write
> within that block.  But by the time this thread has finished
> do_tb_phys_invalidate, any other thread cannot execute the same
> block *again*.
> 
> There's a race here, and now that I think about it, there's been mail about it in the past:
> 
> https://lore.kernel.org/qemu-devel/cebad06c-48f2-6dbd-6d7f-3a3cf5aebbe3@linaro.org/
> 
> We take care of the same race for user-only in translator_access, by
> ensuring that each translated page is read-only *before* performing
> the read for translation.  But for system mode we grab the page
> locks *after* the reads.  Which means there's a race.
> 
> The email above describes the race pretty clearly, with a new TB
> being generated before the write is even complete.
> 
> It'll be non-trivial fixing this, because not only do we need to
> grab the lock earlier, there are ordering issues for a TB that spans
> two pages, in that one must grab the two locks in the correct order
> lest we deadlock.

Yes I can see how this is hard to fix.  Even if we just lock the page
containing the first instruction (which we know) before doing
translation, we still have a problem when entering tb_link_page()
where we would need to only lock the second page, which might cause
ordering issues.

How about a new per-page lock, which would be grabbed by
do_tb_phys_invalidate() and tb_gen_code(), just on the first
instruction?  It would mean, I think, that no page can be having TBs
invalidated and generated at the same time.

Or something like scanning the bytes as they are being translated,
generate a secure-ish checksum, then recheck it after translation and
discard the TB if the code changed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Powered by blists - more mailing lists