Re: [PATCH] MIPS: Close races in TLB modify handlers.

From: David Daney <david.s.daney@gmail.com>
To: Ralf Baechle <ralf@linux-mips.org>
Cc: David Daney <david.daney@cavium.com>,
	"Maciej W. Rozycki" <macro@linux-mips.org>,
	linux-mips@linux-mips.org
Subject: Re: [PATCH] MIPS: Close races in TLB modify handlers.
Date: Fri, 01 Jul 2011 09:38:20 -0700	[thread overview]
Message-ID: <4E0DF7FC.7010007@gmail.com> (raw)
In-Reply-To: <20110701082706.GA8308@linux-mips.org>

On 07/01/2011 01:27 AM, Ralf Baechle wrote:
> On Thu, Jun 30, 2011 at 04:58:04PM -0700, David Daney wrote:
>
>> On 06/30/2011 04:34 PM, Maciej W. Rozycki wrote:
>>> Hi David,
>>>
>>>> Page table entries are made invalid by writing a zero into the the PTE
>>>> slot in a page table.  This creates a race condition with the TLB
>>>> modify handlers when they are updating the PTE.
>>>>
>>>> CPU0                              CPU1
>>>>
>>>> Test for _PAGE_PRESENT
>>>> .                                 set to not _PAGE_PRESENT (zero)
>>>> Set to _PAGE_VALID
>>>>
>>>> So now the page not present value (zero) is suddenly valid and user
>>>> space programs have access to physical page zero.
>>>>
>>>> We close the race by putting the test for _PAGE_PRESENT and setting of
>>>> _PAGE_VALID into an atomic LL/SC section.  This requires more
>>>> registers than just K0 and K1 in the handlers, so we need to save some
>>>> registers to a save area and then restore them when we are done.
>>>   Hmm, good catch, but doesn't your change pessimise the UP case?
>> It may, It is really just a first version of the patch.  I am
>> looking for feedback and testing.
>>
>>> It looks
>>> to me like you save&   restore the scratch registers even though the race
>>> does not apply to UP (you can't interrupt a TLB handler, not at this
>>> stage).
>> That's right.  I will look at trying to generate the old code
>> sequences for non-SMP.
> We can replace all the CONFIG_SMPs in tlbex.c (existing and those added
> by your patch) with num_possible_cpus>  1 which will improve readability
> and give SMP kernels running on a single processor the uniprocessor TLB
> exception handler.
>
> But that's something for a followup patch; your patch is big enough as it
> is, it's not as straight forward as it may sound and the 3.0 clock is
> ticking ...

I am testing a slight revision (using Context/XContext to get the 
logical CPU number instead of physical CPU number from EBase).  I will 
not send this revision until Tuesday because we are on holiday until then.

David Daney