[PATCH V2] MIPS: change type of asid_cache to unsigned long

[PATCH V2] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

asid_cache must be unsigned long otherwise on 64bit system
it will become 0 if the value in get_new_mmu_context()
reaches 0xffffffff and in the end the assumption of
ASID_FIRST_VERSION is not true anymore thus leads to
more dangerous things.

Reported-by: libin <huawei.libin@huawei.com>
Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
---

V2<-V1: Add the reporter.

 arch/mips/include/asm/cpu-info.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
index f6299be..ebcc2ed 100644
--- a/arch/mips/include/asm/cpu-info.h
+++ b/arch/mips/include/asm/cpu-info.h
@@ -40,7 +40,7 @@ struct cache_desc {
 
 struct cpuinfo_mips {
 	unsigned int		udelay_val;
-	unsigned int		asid_cache;
+	unsigned long		asid_cache;
 
 	/*
 	 * Capability and feature descriptor structure for MIPS CPU
-- 
1.7.9.5

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Li Zefan]

I'm not quite happy about what happaned here. There's a story behind
this patch.

One of our Huawei product encountered a bug, and they're using WindRiver4,
so the kernel is 2.6.34.

Because they bought your licnece, they asked for your help, but
you were reluctant on this issue, and the problem remained there
for about one month.

At last they turned to us for help. We're the kernel department in
Huawei, but maintaining this product kernel isn't our job. Still
Li Bin devoted his time to analyzing this bug, and he did a great
job.

Li Bin told the product team what was wrong and was about to send
a fix for upstream kernel. They told you our analysis for further
confirmation, and you were so reluctant to help but so quick to
send the fix.

Li Bin never reported this bug, but he fixed it. It's a shame that
you took the credit from us.

On 2014/5/21 13:36, Yong Zhang wrote:
> asid_cache must be unsigned long otherwise on 64bit system
> it will become 0 if the value in get_new_mmu_context()
> reaches 0xffffffff and in the end the assumption of
> ASID_FIRST_VERSION is not true anymore thus leads to
> more dangerous things.
> 

We should describe what problem this bug can lead to, which
will help people who encounter the same problem and google it.

> Reported-by: libin <huawei.libin@huawei.com>
> Signed-off-by: Yong Zhang <yong.zhang@windriver.com>

Should mark the patch for stable trees. Though 2.6.34 is EOL,
the fix should be backported to other kernels.

> ---
> 
> V2<-V1: Add the reporter.
> 
>  arch/mips/include/asm/cpu-info.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index f6299be..ebcc2ed 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -40,7 +40,7 @@ struct cache_desc {
>  
>  struct cpuinfo_mips {
>  	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>  
>  	/*
>  	 * Capability and feature descriptor structure for MIPS CPU
> 

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
> I'm not quite happy about what happaned here. There's a story behind
> this patch.
> 
> One of our Huawei product encountered a bug, and they're using WindRiver4,
> so the kernel is 2.6.34.
> 
> Because they bought your licnece, they asked for your help, but
> you were reluctant on this issue, and the problem remained there
> for about one month.
> 
> At last they turned to us for help. We're the kernel department in
> Huawei, but maintaining this product kernel isn't our job. Still
> Li Bin devoted his time to analyzing this bug, and he did a great
> job.
> 
> Li Bin told the product team what was wrong and was about to send
> a fix for upstream kernel.

You have time to do that but you didn't.

> They told you our analysis for further
> confirmation,

So you realy didn't make the patch, right? Because you are not
sure the right fix.

> and you were so reluctant to help but so quick to
> send the fix.

We have responsed to you.

> 
> Li Bin never reported this bug, but he fixed it. It's a shame that
> you took the credit from us.

I just saw a bug report and ananysis. And I agreed and confirmed it's
a bug.

Thanks,
Yong





> 
> On 2014/5/21 13:36, Yong Zhang wrote:
> > asid_cache must be unsigned long otherwise on 64bit system
> > it will become 0 if the value in get_new_mmu_context()
> > reaches 0xffffffff and in the end the assumption of
> > ASID_FIRST_VERSION is not true anymore thus leads to
> > more dangerous things.
> > 
> 
> We should describe what problem this bug can lead to, which
> will help people who encounter the same problem and google it.
> 
> > Reported-by: libin <huawei.libin@huawei.com>
> > Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
> 
> Should mark the patch for stable trees. Though 2.6.34 is EOL,
> the fix should be backported to other kernels.
> 
> > ---
> > 
> > V2<-V1: Add the reporter.
> > 
> >  arch/mips/include/asm/cpu-info.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> > index f6299be..ebcc2ed 100644
> > --- a/arch/mips/include/asm/cpu-info.h
> > +++ b/arch/mips/include/asm/cpu-info.h
> > @@ -40,7 +40,7 @@ struct cache_desc {
> >  
> >  struct cpuinfo_mips {
> >  	unsigned int		udelay_val;
> > -	unsigned int		asid_cache;
> > +	unsigned long		asid_cache;
> >  
> >  	/*
> >  	 * Capability and feature descriptor structure for MIPS CPU
> > 

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Li Zefan]

On 2014/5/27 12:34, Yong Zhang wrote:
> On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
>> I'm not quite happy about what happaned here. There's a story behind
>> this patch.
>>
>> One of our Huawei product encountered a bug, and they're using WindRiver4,
>> so the kernel is 2.6.34.
>>
>> Because they bought your licnece, they asked for your help, but
>> you were reluctant on this issue, and the problem remained there
>> for about one month.
>>
>> At last they turned to us for help. We're the kernel department in
>> Huawei, but maintaining this product kernel isn't our job. Still
>> Li Bin devoted his time to analyzing this bug, and he did a great
>> job.
>>
>> Li Bin told the product team what was wrong and was about to send
>> a fix for upstream kernel.
> 
> You have time to do that but you didn't.
> 

Hah yeah, we do have time. we spent lots of time analyzing the bug,
and we were taking our time to write good changelog. As I've pointed
out that your changelog isn't informative.

>> They told you our analysis for further
>> confirmation,
> 
> So you realy didn't make the patch, right? Because you are not
> sure the right fix.
> 

We're confident about our analysis and we know how to fix it.

It's the product team wasn't sure about this, and they wasn't
able to contact with Li Bin for confirmation at that time, so they
asked you.

>> and you were so reluctant to help but so quick to
>> send the fix.
> 
> We have responsed to you.
> 

You responded to us but you did nothing to help, that's why the
product team found us.

>>
>> Li Bin never reported this bug, but he fixed it. It's a shame that
>> you took the credit from us.
> 
> I just saw a bug report and ananysis. And I agreed and confirmed it's
> a bug.
> 

And that's our work and our credit, and I don't think you're gonna
to deny it.

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

BTW, I realy don't care who credits the patch and Ralf said that
he will applied the one which moves the place of udelay_val.

Anyway, if your company pays you more money if you contribute to
the community, just take it and talk about it with Ralf ;-)

Thanks,
Yong

On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
> I'm not quite happy about what happaned here. There's a story behind
> this patch.
> 
> One of our Huawei product encountered a bug, and they're using WindRiver4,
> so the kernel is 2.6.34.
> 
> Because they bought your licnece, they asked for your help, but
> you were reluctant on this issue, and the problem remained there
> for about one month.
> 
> At last they turned to us for help. We're the kernel department in
> Huawei, but maintaining this product kernel isn't our job. Still
> Li Bin devoted his time to analyzing this bug, and he did a great
> job.
> 
> Li Bin told the product team what was wrong and was about to send
> a fix for upstream kernel. They told you our analysis for further
> confirmation, and you were so reluctant to help but so quick to
> send the fix.
> 
> Li Bin never reported this bug, but he fixed it. It's a shame that
> you took the credit from us.
> 
> On 2014/5/21 13:36, Yong Zhang wrote:
> > asid_cache must be unsigned long otherwise on 64bit system
> > it will become 0 if the value in get_new_mmu_context()
> > reaches 0xffffffff and in the end the assumption of
> > ASID_FIRST_VERSION is not true anymore thus leads to
> > more dangerous things.
> > 
> 
> We should describe what problem this bug can lead to, which
> will help people who encounter the same problem and google it.
> 
> > Reported-by: libin <huawei.libin@huawei.com>
> > Signed-off-by: Yong Zhang <yong.zhang@windriver.com>
> 
> Should mark the patch for stable trees. Though 2.6.34 is EOL,
> the fix should be backported to other kernels.
> 
> > ---
> > 
> > V2<-V1: Add the reporter.
> > 
> >  arch/mips/include/asm/cpu-info.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> > index f6299be..ebcc2ed 100644
> > --- a/arch/mips/include/asm/cpu-info.h
> > +++ b/arch/mips/include/asm/cpu-info.h
> > @@ -40,7 +40,7 @@ struct cache_desc {
> >  
> >  struct cpuinfo_mips {
> >  	unsigned int		udelay_val;
> > -	unsigned int		asid_cache;
> > +	unsigned long		asid_cache;
> >  
> >  	/*
> >  	 * Capability and feature descriptor structure for MIPS CPU
> > 

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Li Zefan]

On 2014/5/27 12:50, Yong Zhang wrote:
> BTW, I realy don't care who credits the patch and Ralf said that
> he will applied the one which moves the place of udelay_val.
> 
> Anyway, if your company pays you more money if you contribute to
> the community, just take it and talk about it with Ralf ;-)
> 

We don't do contribution for money, and I don't think you do,
but crediting properly is one of the reason that our kernel
community keeps prosperous for so many years, and that's one
of the reason we introduced Reported-by and Tested-by tags.

> Thanks,
> Yong

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Yong Zhang]

On Tue, May 27, 2014 at 01:07:20PM +0800, Li Zefan wrote:
> On 2014/5/27 12:50, Yong Zhang wrote:
> > BTW, I realy don't care who credits the patch and Ralf said that
> > he will applied the one which moves the place of udelay_val.
> > 
> > Anyway, if your company pays you more money if you contribute to
> > the community, just take it and talk about it with Ralf ;-)
> > 
> 
> We don't do contribution for money, and I don't think you do,
> but crediting properly is one of the reason that our kernel
> community keeps prosperous for so many years, and that's one
> of the reason we introduced Reported-by and Tested-by tags.

I'll reply this email for the last time.

To me your action is just like Reported-by, but I admit that
you also do analysis. If you don't the way change it to whatever
you want.

Thanks,
Yong

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Li Zefan]

On 2014/5/27 13:23, Yong Zhang wrote:
> On Tue, May 27, 2014 at 01:07:20PM +0800, Li Zefan wrote:
>> On 2014/5/27 12:50, Yong Zhang wrote:
>>> BTW, I realy don't care who credits the patch and Ralf said that
>>> he will applied the one which moves the place of udelay_val.
>>>
>>> Anyway, if your company pays you more money if you contribute to
>>> the community, just take it and talk about it with Ralf ;-)
>>>
>>
>> We don't do contribution for money, and I don't think you do,
>> but crediting properly is one of the reason that our kernel
>> community keeps prosperous for so many years, and that's one
>> of the reason we introduced Reported-by and Tested-by tags.
> 
> I'll reply this email for the last time.
> 
> To me your action is just like Reported-by, but I admit that
> you also do analysis. If you don't the way change it to whatever
> you want.
> 

Sorry if I sounded offensive. I want Li Bin to get the credit,
because he's supposed to, and I want him to be encouraged in
contributing to the mainline kernel.

The decision is on Ralf, whether to accept your patch or let
us send our fix with detailed changelog.

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Aaro Koskinen]

Hi,

On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
> On 2014/5/21 13:36, Yong Zhang wrote:
> > asid_cache must be unsigned long otherwise on 64bit system
> > it will become 0 if the value in get_new_mmu_context()
> > reaches 0xffffffff and in the end the assumption of
> > ASID_FIRST_VERSION is not true anymore thus leads to
> > more dangerous things.
> 
> We should describe what problem this bug can lead to, which
> will help people who encounter the same problem and google it.

Please describe it, then. Even if the patch is already committed,
googling would probably still find this e-mail thread.

Thanks,

A.

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Li Zefan]

On 2014/5/29 4:09, Aaro Koskinen wrote:
> Hi,
> 
> On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
>> On 2014/5/21 13:36, Yong Zhang wrote:
>>> asid_cache must be unsigned long otherwise on 64bit system
>>> it will become 0 if the value in get_new_mmu_context()
>>> reaches 0xffffffff and in the end the assumption of
>>> ASID_FIRST_VERSION is not true anymore thus leads to
>>> more dangerous things.
>>
>> We should describe what problem this bug can lead to, which
>> will help people who encounter the same problem and google it.
> 
> Please describe it, then. Even if the patch is already committed,
> googling would probably still find this e-mail thread.
> 

I don't think Ralf has committed it, so we'll send out a fix
with detailed changelog.

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Ralf Baechle]

On Thu, May 29, 2014 at 04:57:07PM +0800, Li Zefan wrote:

> I don't think Ralf has committed it, so we'll send out a fix
> with detailed changelog.

It's queued to go upstream, commit e5eb925a1804c4a52994ba57f4f68ee7a9132905.

  Ralf

Re: [PATCH V2] MIPS: change type of asid_cache to unsigned long

[Libin]

On 2014/5/29 4:09, Aaro Koskinen wrote:
> Hi,
> 
> On Tue, May 27, 2014 at 12:16:30PM +0800, Li Zefan wrote:
>> On 2014/5/21 13:36, Yong Zhang wrote:
>>> asid_cache must be unsigned long otherwise on 64bit system
>>> it will become 0 if the value in get_new_mmu_context()
>>> reaches 0xffffffff and in the end the assumption of
>>> ASID_FIRST_VERSION is not true anymore thus leads to
>>> more dangerous things.
>>
>> We should describe what problem this bug can lead to, which
>> will help people who encounter the same problem and google it.
> 
> Please describe it, then. Even if the patch is already committed,
> googling would probably still find this e-mail thread.
> 
> Thanks,
> 
> A.
> 
> 

Problem description:
On our MIPS architecture product, after a long time running our business
service, a random cpu trigger the problem, that if running test cases
include the following code on this cpu will trigger bus error or
segment fault:
    ...
    pid = fork();
    if (pid < 0)
        return 1;
    if (0 == pid)
        exit(0);
    else
            exit(0);
    ...

Root cause:
After doing a lot of fork/mmap/munmap operations, it will make the asid value
exceeds 0xffffffff in get_new_mmu_context function, which is truncated to 0:
|-get_new_mmu_context(struct mm_struct *mm, unsigned long cpu)
    unsigned long asid = asid_cache(cpu); //if asid_cache(cpu) is 0xffffffff now
    if (! ((asid += ASID_INC) & ASID_MASK) ) {  //asid reaches 0x1 0000 0000
        ...
        local_flush_tlb_all();         /* start new asid cycle */
        if (!asid)             /* fix version if needed */  //but here condition does not meet...
            asid = ASID_FIRST_VERSION;
         }
         cpu_context(cpu, mm) = asid_cache(cpu) = asid; //and here cpu_context and asid_cache is truncated to 0

In do_fork()->dup_mmap(), adding write-protect flag for writable page but the
following tlb flush does not take effect, and breaks the normal COW:
do_fork()
|-copy_process()
    |-copy_mm()
        ...
        |-dup_mmap()
            |-copy_page_range()
                ...
                |-copy_one_pte()
                ...
                    if (is_cow_mapping(vm_flags)) {
                        ptep_set_wrprotect(src_mm, addr, src_pte);
                        pte = pte_wrprotect(pte);
                    }
                ...
        |-flush_tlb_mm(oldmm)
            |-local_flush_tlb_mm（）
                if (cpu_context(cpu, mm) != 0) {//cpu_context is 0, no tlb flush
                drop_mmu_context(mm, cpu);
            }

In addition, the condition ((cpu_context(cpu, next) ^ asid_cache(cpu))
& ASID_VERSION_MASK) can not be met in switch_mm(), and the tlb flush operation
can not be completed during the process switch.
|-switch_mm()
    ...
    /* Check if our ASID is of an older version and thus invalid */
    if ((cpu_context(cpu, next) ^ asid_cache(cpu)) & ASID_VERSION_MASK)
        get_new_mmu_context(next, cpu);
        write_c0_entryhi(cpu_asid(cpu, next));
    ...

In short, due to the truncation operation caused by inappropriate type conversion,
making tlb flush failure, causing problems of COW, triggering bus error or segment fault.

Thanks,
Libin