[PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

Unfortunately, p?d_alloc() and p?d_free() are not pair!! If p?d_alloc()
succeed, they may be used, so in the next failure, we have to skip them
to let exit_mmap() or do_munmap() to process it.

According to "Documentation/vm/locking", 'mm->page_table_lock' is for
using vma list, so not need it when its related vmas are detached or
unmapped from using vma list.

The related work flow:

  exit_mmap() ->
    unmap_vmas(); /* so not need mm->page_table_lock */
    free_pgtables();

  do_munmap()->
    detach_vmas_to_be_unmapped(); /* so not need mm->page_table_lock */
    unmap_region() ->
      free_pgtables();

  free_pgtables() ->
    free_pgd_range() ->
      free_pud_range() ->
        free_pmd_range() ->
          free_pte_range() ->
            pmd_clear();
            pte_free_tlb();
          pud_clear();
          pmd_free_tlb();
        pgd_clear(); 
        pud_free_tlb();


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 arch/um/kernel/skas/mmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
index 007d550..3fd1951 100644
--- a/arch/um/kernel/skas/mmu.c
+++ b/arch/um/kernel/skas/mmu.c
@@ -40,9 +40,9 @@ static int init_stub_pte(struct mm_struct *mm, unsigned long proc,
 	return 0;
 
  out_pte:
-	pmd_free(mm, pmd);
+	/* used by mm->pgd->pud, will free in do_munmap() or exit_mmap() */
  out_pmd:
-	pud_free(mm, pud);
+	/* used by mm->pgd, will free in do_munmap() or exit_mmap() */
  out:
 	return -ENOMEM;
 }
-- 
1.7.7.6

Re: [uml-devel] [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Richard Weinberger]

Am 13.11.2013 06:06, schrieb Chen Gang:
> Unfortunately, p?d_alloc() and p?d_free() are not pair!! If p?d_alloc()
> succeed, they may be used, so in the next failure, we have to skip them
> to let exit_mmap() or do_munmap() to process it.
> 
> According to "Documentation/vm/locking", 'mm->page_table_lock' is for
> using vma list, so not need it when its related vmas are detached or
> unmapped from using vma list.
> 
> The related work flow:
> 
>   exit_mmap() ->
>     unmap_vmas(); /* so not need mm->page_table_lock */
>     free_pgtables();
> 
>   do_munmap()->
>     detach_vmas_to_be_unmapped(); /* so not need mm->page_table_lock */
>     unmap_region() ->
>       free_pgtables();
> 
>   free_pgtables() ->
>     free_pgd_range() ->
>       free_pud_range() ->
>         free_pmd_range() ->
>           free_pte_range() ->
>             pmd_clear();
>             pte_free_tlb();
>           pud_clear();
>           pmd_free_tlb();
>         pgd_clear(); 
>         pud_free_tlb();
> 
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>

Sounds reasonable to me.
*But* there are patches you from out there that tried to fix similar issues and got reverted later.
Now I'm a bit nervous and want a ACK from mm folks to have this verified.
It's not that I don't trust you, but I really don't trust you. ;-)

Thanks,
//richard

------------------------------------------------------------------------------
DreamFactory - Open Source REST & JSON Services for HTML5 & Native Apps
OAuth, Users, Roles, SQL, NoSQL, BLOB Storage and External API Access
Free app hosting. Or install the open source package on any LAMP server.
Sign up and see examples for AngularJS, jQuery, Sencha Touch and Native!
http://pubads.g.doubleclick.net/gampad/clk?id=63469471&iu=/4140/ostg.clktrk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

On 11/13/2013 05:07 PM, Richard Weinberger wrote:
> Am 13.11.2013 06:06, schrieb Chen Gang:
>> Unfortunately, p?d_alloc() and p?d_free() are not pair!! If p?d_alloc()
>> succeed, they may be used, so in the next failure, we have to skip them
>> to let exit_mmap() or do_munmap() to process it.
>>
>> According to "Documentation/vm/locking", 'mm->page_table_lock' is for
>> using vma list, so not need it when its related vmas are detached or
>> unmapped from using vma list.
>>
>> The related work flow:
>>
>>   exit_mmap() ->
>>     unmap_vmas(); /* so not need mm->page_table_lock */
>>     free_pgtables();
>>
>>   do_munmap()->
>>     detach_vmas_to_be_unmapped(); /* so not need mm->page_table_lock */
>>     unmap_region() ->
>>       free_pgtables();
>>
>>   free_pgtables() ->
>>     free_pgd_range() ->
>>       free_pud_range() ->
>>         free_pmd_range() ->
>>           free_pte_range() ->
>>             pmd_clear();
>>             pte_free_tlb();
>>           pud_clear();
>>           pmd_free_tlb();
>>         pgd_clear(); 
>>         pud_free_tlb();
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> 
> Sounds reasonable to me.
> *But* there are patches you from out there that tried to fix similar issues and got reverted later.
> Now I'm a bit nervous and want a ACK from mm folks to have this verified.
> It's not that I don't trust you, but I really don't trust you. ;-)
> 

OK, thanks.

> Thanks,
> //richard
> 
> 

-- 
Chen Gang

------------------------------------------------------------------------------
DreamFactory - Open Source REST & JSON Services for HTML5 & Native Apps
OAuth, Users, Roles, SQL, NoSQL, BLOB Storage and External API Access
Free app hosting. Or install the open source package on any LAMP server.
Sign up and see examples for AngularJS, jQuery, Sencha Touch and Native!
http://pubads.g.doubleclick.net/gampad/clk?id=63469471&iu=/4140/ostg.clktrk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Hugh Dickins]

On Wed, 13 Nov 2013, Chen Gang wrote:

> Unfortunately, p?d_alloc() and p?d_free() are not pair!! If p?d_alloc()
> succeed, they may be used, so in the next failure, we have to skip them
> to let exit_mmap() or do_munmap() to process it.
> 
> According to "Documentation/vm/locking", 'mm->page_table_lock' is for
> using vma list, so not need it when its related vmas are detached or
> unmapped from using vma list.

Hah, don't believe a word of Documentation/vm/locking.  From time to
time someone or other has updated some part of it, but on the whole
it represents the state of the art in 1999.  Look at its git history:
not a lot of activity there.

And please don't ask me to update it, and please don't try to update
it yourself.  Delete it?  Maybe.

Study the code itself for how mm locking is actually done
(can you see anywhere we use page_table_lock on the vma list?)

> 
> The related work flow:
> 
>   exit_mmap() ->
>     unmap_vmas(); /* so not need mm->page_table_lock */
>     free_pgtables();
> 
>   do_munmap()->
>     detach_vmas_to_be_unmapped(); /* so not need mm->page_table_lock */
>     unmap_region() ->
>       free_pgtables();
> 
>   free_pgtables() ->
>     free_pgd_range() ->
>       free_pud_range() ->
>         free_pmd_range() ->
>           free_pte_range() ->
>             pmd_clear();
>             pte_free_tlb();
>           pud_clear();
>           pmd_free_tlb();
>         pgd_clear(); 
>         pud_free_tlb();

I don't think those notes would belong in this patch...

> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  arch/um/kernel/skas/mmu.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
> index 007d550..3fd1951 100644
> --- a/arch/um/kernel/skas/mmu.c
> +++ b/arch/um/kernel/skas/mmu.c
> @@ -40,9 +40,9 @@ static int init_stub_pte(struct mm_struct *mm, unsigned long proc,
>  	return 0;
>  
>   out_pte:
> -	pmd_free(mm, pmd);
> +	/* used by mm->pgd->pud, will free in do_munmap() or exit_mmap() */
>   out_pmd:
> -	pud_free(mm, pud);
> +	/* used by mm->pgd, will free in do_munmap() or exit_mmap() */
>   out:
>  	return -ENOMEM;
>  }
> -- 
> 1.7.7.6

... but I'm not going to ack this: I just don't share your zest
for mucking around with what I don't understand, and don't have
the time to spare to understand it well enough.

From the look of it, if an error did occur in init_stub_pte(),
then the special mapping of STUB_CODE and STUB_DATA would not
be installed, so this area would be invisible to munmap and exit,
and with your patch then the pages allocated likely to be leaked.

Which is not to say that the existing code is actually correct:
you're probably right that it's technically wrong.  But it would
be very hard to get init_stub_pte() to fail, and has anyone
reported a problem with it?  My guess is not, and my own
inclination to dabble here is zero.

Hugh

Re: [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

Firstly, thank you for spending your time resources to reply so many
details.


On 11/14/2013 01:20 PM, Hugh Dickins wrote:
> On Wed, 13 Nov 2013, Chen Gang wrote:
> 
>> Unfortunately, p?d_alloc() and p?d_free() are not pair!! If p?d_alloc()
>> succeed, they may be used, so in the next failure, we have to skip them
>> to let exit_mmap() or do_munmap() to process it.
>>
>> According to "Documentation/vm/locking", 'mm->page_table_lock' is for
>> using vma list, so not need it when its related vmas are detached or
>> unmapped from using vma list.
> 
> Hah, don't believe a word of Documentation/vm/locking.  From time to
> time someone or other has updated some part of it, but on the whole
> it represents the state of the art in 1999.  Look at its git history:
> not a lot of activity there.
> 
> And please don't ask me to update it, and please don't try to update
> it yourself.  Delete it?  Maybe.
> 

I agree with you.

> Study the code itself for how mm locking is actually done
> (can you see anywhere we use page_table_lock on the vma list?)
> 

in p?d_alloc() will use page_table_lock, that the reason why I noticed
about it.


>>
>> The related work flow:
>>
>>   exit_mmap() ->
>>     unmap_vmas(); /* so not need mm->page_table_lock */
>>     free_pgtables();
>>
>>   do_munmap()->
>>     detach_vmas_to_be_unmapped(); /* so not need mm->page_table_lock */
>>     unmap_region() ->
>>       free_pgtables();
>>
>>   free_pgtables() ->
>>     free_pgd_range() ->
>>       free_pud_range() ->
>>         free_pmd_range() ->
>>           free_pte_range() ->
>>             pmd_clear();
>>             pte_free_tlb();
>>           pud_clear();
>>           pmd_free_tlb();
>>         pgd_clear(); 
>>         pud_free_tlb();
> 
> I don't think those notes would belong in this patch...
> 

Hmm... maybe what you said above is correct, although it is also the
sample to show how to remove them if pgd is related with real 'mm',
which is not like p?d_free().


>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  arch/um/kernel/skas/mmu.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
>> index 007d550..3fd1951 100644
>> --- a/arch/um/kernel/skas/mmu.c
>> +++ b/arch/um/kernel/skas/mmu.c
>> @@ -40,9 +40,9 @@ static int init_stub_pte(struct mm_struct *mm, unsigned long proc,
>>  	return 0;
>>  
>>   out_pte:
>> -	pmd_free(mm, pmd);
>> +	/* used by mm->pgd->pud, will free in do_munmap() or exit_mmap() */
>>   out_pmd:
>> -	pud_free(mm, pud);
>> +	/* used by mm->pgd, will free in do_munmap() or exit_mmap() */
>>   out:
>>  	return -ENOMEM;
>>  }
>> -- 
>> 1.7.7.6
> 
> .. but I'm not going to ack this: I just don't share your zest
> for mucking around with what I don't understand, and don't have
> the time to spare to understand it well enough.
> 

OK, I can understand. It belongs both mm area and um, it seems no one
familiar both of them (for me, I am familiar with neither of them).

But, for me, need continue analyzing and discussing.

>>>From the look of it, if an error did occur in init_stub_pte(),
> then the special mapping of STUB_CODE and STUB_DATA would not
> be installed, so this area would be invisible to munmap and exit,
> and with your patch then the pages allocated likely to be leaked.
> 

It sounds reasonable to me: "although 'pgd' related with 'mm', but they
are not installed". But just like you said originally: "better get ACK
from some mm guys".


Hmm... is it another issue: "after STUB_CODE succeeds, but STUB_DATA
fails, the STUB_CODE will be leaked".


> Which is not to say that the existing code is actually correct:
> you're probably right that it's technically wrong.  But it would
> be very hard to get init_stub_pte() to fail, and has anyone
> reported a problem with it?  My guess is not, and my own
> inclination to dabble here is zero.
> 

Yeah.

> Hugh
> 
> 


-- 
Chen Gang

Re: [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

On 11/14/2013 02:48 PM, Chen Gang wrote:
>> >From the look of it, if an error did occur in init_stub_pte(),
>> > then the special mapping of STUB_CODE and STUB_DATA would not
>> > be installed, so this area would be invisible to munmap and exit,
>> > and with your patch then the pages allocated likely to be leaked.
>> > 
> It sounds reasonable to me: "although 'pgd' related with 'mm', but they
> are not installed". But just like you said originally: "better get ACK
> from some mm guys".
> 
> 
> Hmm... is it another issue: "after STUB_CODE succeeds, but STUB_DATA
> fails, the STUB_CODE will be leaked".
> 
> 
>> > Which is not to say that the existing code is actually correct:
>> > you're probably right that it's technically wrong.  But it would
>> > be very hard to get init_stub_pte() to fail, and has anyone
>> > reported a problem with it?  My guess is not, and my own
>> > inclination to dabble here is zero.
>> > 
> Yeah.
> 

If we can not get ACK from any mm guys, and we have no enough time
resource to read related source code, for me, I still recommend to
remove p?d_free() in failure processing.

In the worst cases, we will leak a little memory, and no any other
negative effect, it is an executable way which is no any risks.

For current mm implementation, it seems we can not assume any thing,
although they sounds (or should be) reasonable (include what you said
about mm).


Thanks.
-- 
Chen Gang

Re: [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Richard Weinberger]

Am 14.11.2013 08:33, schrieb Chen Gang:
> On 11/14/2013 02:48 PM, Chen Gang wrote:
>>> >From the look of it, if an error did occur in init_stub_pte(),
>>>> then the special mapping of STUB_CODE and STUB_DATA would not
>>>> be installed, so this area would be invisible to munmap and exit,
>>>> and with your patch then the pages allocated likely to be leaked.
>>>>
>> It sounds reasonable to me: "although 'pgd' related with 'mm', but they
>> are not installed". But just like you said originally: "better get ACK
>> from some mm guys".
>>
>>
>> Hmm... is it another issue: "after STUB_CODE succeeds, but STUB_DATA
>> fails, the STUB_CODE will be leaked".
>>
>>
>>>> Which is not to say that the existing code is actually correct:
>>>> you're probably right that it's technically wrong.  But it would
>>>> be very hard to get init_stub_pte() to fail, and has anyone
>>>> reported a problem with it?  My guess is not, and my own
>>>> inclination to dabble here is zero.
>>>>
>> Yeah.
>>
> 
> If we can not get ACK from any mm guys, and we have no enough time
> resource to read related source code, for me, I still recommend to
> remove p?d_free() in failure processing.

It's rather easy, does your commit fix a real problem you are facing?
If the answer is "yes" we can talk.

Chen, If you really want to help us, please investigate into existing/real problems.
Toralf does a very good job in finding strange issues using trinity.
You could help him resolving the issue described in that thread:
"[uml-devel] fuzz tested 32 bit user mode linux image hangs in radix_tree_next_chunk()"

Thanks,
//richard

Re: [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

On 11/14/2013 03:55 PM, Richard Weinberger wrote:
> Am 14.11.2013 08:33, schrieb Chen Gang:
>> > On 11/14/2013 02:48 PM, Chen Gang wrote:
>>>> >>> >From the look of it, if an error did occur in init_stub_pte(),
>>>>> >>>> then the special mapping of STUB_CODE and STUB_DATA would not
>>>>> >>>> be installed, so this area would be invisible to munmap and exit,
>>>>> >>>> and with your patch then the pages allocated likely to be leaked.
>>>>> >>>>
>>> >> It sounds reasonable to me: "although 'pgd' related with 'mm', but they
>>> >> are not installed". But just like you said originally: "better get ACK
>>> >> from some mm guys".
>>> >>
>>> >>
>>> >> Hmm... is it another issue: "after STUB_CODE succeeds, but STUB_DATA
>>> >> fails, the STUB_CODE will be leaked".
>>> >>
>>> >>
>>>>> >>>> Which is not to say that the existing code is actually correct:
>>>>> >>>> you're probably right that it's technically wrong.  But it would
>>>>> >>>> be very hard to get init_stub_pte() to fail, and has anyone
>>>>> >>>> reported a problem with it?  My guess is not, and my own
>>>>> >>>> inclination to dabble here is zero.
>>>>> >>>>
>>> >> Yeah.
>>> >>
>> > 
>> > If we can not get ACK from any mm guys, and we have no enough time
>> > resource to read related source code, for me, I still recommend to
>> > remove p?d_free() in failure processing.
> It's rather easy, does your commit fix a real problem you are facing?
> If the answer is "yes" we can talk.
> 

We have met many code which *should* be correct, but in really world it
is incorrect (most of reasons come from related interface definition).
I want to let these code less and less.

Now I want to make clear all p?d_free() related things, and except our
um, also 3 areas use it.

  arch/arm/mm/pgd.c:100:  pud_free(mm, new_pud);
  arch/arm/mm/pgd.c:137:  pud_free(mm, pud);
  arch/arm/mm/pgd.c:155:          pud_free(mm, pud);
  drivers/iommu/arm-smmu.c:947:   pud_free(NULL, pud_base);
  mm/memory.c:3835:               pud_free(mm, new);

I am checking all of them (include um), and now for me, the related
code under um can be improved, so I communicate with you (send the
related patch).


> Chen, If you really want to help us, please investigate into existing/real problems.
> Toralf does a very good job in finding strange issues using trinity.
> You could help him resolving the issue described in that thread:
> "[uml-devel] fuzz tested 32 bit user mode linux image hangs in radix_tree_next_chunk()"

Excuse me, at least now, I have no related plan to analyze issues which
not find by myself. The reasons are:

 - I am not quite familiar with kernel, I need preparing (what I am doing is just familiar kernel step by step).

 - my current/original plan about public kernel is delayed.

 - originally I plan (not declare to outside) to solve the issues from Bugzilla of public kernel in next year (2014).
   it will (of cause) also be delayed (I even can not dare to declare it, now).


Thanks.
-- 
Chen Gang

Re: [uml-devel] [PATCH] arch: um: kernel: skas: mmu: remove pmd_free() and pud_free() for failure processing in init_stub_pte()

[Chen Gang]

On 11/14/2013 03:33 PM, Chen Gang wrote:
> On 11/14/2013 02:48 PM, Chen Gang wrote:
>>> >From the look of it, if an error did occur in init_stub_pte(),
>>>> then the special mapping of STUB_CODE and STUB_DATA would not
>>>> be installed, so this area would be invisible to munmap and exit,
>>>> and with your patch then the pages allocated likely to be leaked.
>>>>
>> It sounds reasonable to me: "although 'pgd' related with 'mm', but they
>> are not installed". But just like you said originally: "better get ACK
>> from some mm guys".
>>
>>
>> Hmm... is it another issue: "after STUB_CODE succeeds, but STUB_DATA
>> fails, the STUB_CODE will be leaked".
>>
>>
>>>> Which is not to say that the existing code is actually correct:
>>>> you're probably right that it's technically wrong.  But it would
>>>> be very hard to get init_stub_pte() to fail, and has anyone
>>>> reported a problem with it?  My guess is not, and my own
>>>> inclination to dabble here is zero.
>>>>
>> Yeah.
>>
> 
> If we can not get ACK from any mm guys, and we have no enough time
> resource to read related source code, for me, I still recommend to
> remove p?d_free() in failure processing.
> 

Oh, I am very sorry to Hugh and Richard, I make a mistake in common
sense: I recognized incorrect members (I treated Hugh as Richard), Hugh
is "mm guys".

Next time, I should see the mail carefully, not only for contents, but
also for senders.

Sorry again to both of you.

Thanks.

> In the worst cases, we will leak a little memory, and no any other
> negative effect, it is an executable way which is no any risks.
> 
> For current mm implementation, it seems we can not assume any thing,
> although they sounds (or should be) reasonable (include what you said
> about mm).
> 
> 
> Thanks.
> 


-- 
Chen Gang

------------------------------------------------------------------------------
DreamFactory - Open Source REST & JSON Services for HTML5 & Native Apps
OAuth, Users, Roles, SQL, NoSQL, BLOB Storage and External API Access
Free app hosting. Or install the open source package on any LAMP server.
Sign up and see examples for AngularJS, jQuery, Sencha Touch and Native!
http://pubads.g.doubleclick.net/gampad/clk?id=63469471&iu=/4140/ostg.clktrk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel