[PATCH 1/2] sparc: Fix handling of page table constructor failure

[PATCH 1/2] sparc: Fix handling of page table constructor failure

[Matthew Wilcox (Oracle)]

The page has just been allocated, so its refcount is 1.  free_unref_page()
is for use on pages which have a zero refcount.  Use __free_page()
like the other implementations of pte_alloc_one().

Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 arch/sparc/mm/init_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 96edf64d4fb3..182bb7bdaa0a 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
 	if (!page)
 		return NULL;
 	if (!pgtable_pte_page_ctor(page)) {
-		free_unref_page(page);
+		__free_page(page);
 		return NULL;
 	}
 	return (pte_t *) page_address(page);
-- 
2.29.2

[PATCH 2/2] mm: Move free_unref_page to mm/internal.h

[Matthew Wilcox (Oracle)]

Code outside mm/ should not be calling free_unref_page().  Also
move free_unref_page_list().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/gfp.h | 2 --
 mm/internal.h       | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..6e479e9c48ce 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -580,8 +580,6 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
 
 extern void __free_pages(struct page *page, unsigned int order);
 extern void free_pages(unsigned long addr, unsigned int order);
-extern void free_unref_page(struct page *page);
-extern void free_unref_page_list(struct list_head *list);
 
 struct page_frag_cache;
 extern void __page_frag_cache_drain(struct page *page, unsigned int count);
diff --git a/mm/internal.h b/mm/internal.h
index 75ae680d0a2c..5864815947fe 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -201,6 +201,9 @@ extern void post_alloc_hook(struct page *page, unsigned int order,
 					gfp_t gfp_flags);
 extern int user_min_free_kbytes;
 
+extern void free_unref_page(struct page *page);
+extern void free_unref_page_list(struct list_head *list);
+
 extern void zone_pcp_update(struct zone *zone);
 extern void zone_pcp_reset(struct zone *zone);
 extern void zone_pcp_disable(struct zone *zone);
-- 
2.29.2

Re: [PATCH 1/2] sparc: Fix handling of page table constructor failure

[David Hildenbrand]

On 25.11.20 04:46, Matthew Wilcox (Oracle) wrote:
> The page has just been allocated, so its refcount is 1.  free_unref_page()
> is for use on pages which have a zero refcount.  Use __free_page()
> like the other implementations of pte_alloc_one().
> 
> Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  arch/sparc/mm/init_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 96edf64d4fb3..182bb7bdaa0a 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
>  	if (!page)
>  		return NULL;
>  	if (!pgtable_pte_page_ctor(page)) {
> -		free_unref_page(page);
> +		__free_page(page);
>  		return NULL;
>  	}
>  	return (pte_t *) page_address(page);
> 

I wonder if reusing __pte_alloc_one() - e.g., internally - would be even
cleaner.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] mm: Move free_unref_page to mm/internal.h

[David Hildenbrand]

On 25.11.20 04:46, Matthew Wilcox (Oracle) wrote:
> Code outside mm/ should not be calling free_unref_page().  Also
> move free_unref_page_list().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/gfp.h | 2 --
>  mm/internal.h       | 3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index c603237e006c..6e479e9c48ce 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -580,8 +580,6 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
>  
>  extern void __free_pages(struct page *page, unsigned int order);
>  extern void free_pages(unsigned long addr, unsigned int order);
> -extern void free_unref_page(struct page *page);
> -extern void free_unref_page_list(struct list_head *list);
>  
>  struct page_frag_cache;
>  extern void __page_frag_cache_drain(struct page *page, unsigned int count);
> diff --git a/mm/internal.h b/mm/internal.h
> index 75ae680d0a2c..5864815947fe 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -201,6 +201,9 @@ extern void post_alloc_hook(struct page *page, unsigned int order,
>  					gfp_t gfp_flags);
>  extern int user_min_free_kbytes;
>  
> +extern void free_unref_page(struct page *page);
> +extern void free_unref_page_list(struct list_head *list);
> +
>  extern void zone_pcp_update(struct zone *zone);
>  extern void zone_pcp_reset(struct zone *zone);
>  extern void zone_pcp_disable(struct zone *zone);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] sparc: Fix handling of page table constructor failure

[Mike Rapoport]

On Wed, Nov 25, 2020 at 03:46:54AM +0000, Matthew Wilcox (Oracle) wrote:
> The page has just been allocated, so its refcount is 1.  free_unref_page()
> is for use on pages which have a zero refcount.  Use __free_page()
> like the other implementations of pte_alloc_one().
> 
> Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  arch/sparc/mm/init_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 96edf64d4fb3..182bb7bdaa0a 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
>  	if (!page)
>  		return NULL;
>  	if (!pgtable_pte_page_ctor(page)) {
> -		free_unref_page(page);
> +		__free_page(page);
>  		return NULL;
>  	}
>  	return (pte_t *) page_address(page);
> -- 
> 2.29.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 2/2] mm: Move free_unref_page to mm/internal.h

[Mike Rapoport]

On Wed, Nov 25, 2020 at 03:46:55AM +0000, Matthew Wilcox (Oracle) wrote:
> Code outside mm/ should not be calling free_unref_page().  Also
> move free_unref_page_list().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
>  include/linux/gfp.h | 2 --
>  mm/internal.h       | 3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index c603237e006c..6e479e9c48ce 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -580,8 +580,6 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
>  
>  extern void __free_pages(struct page *page, unsigned int order);
>  extern void free_pages(unsigned long addr, unsigned int order);
> -extern void free_unref_page(struct page *page);
> -extern void free_unref_page_list(struct list_head *list);
>  
>  struct page_frag_cache;
>  extern void __page_frag_cache_drain(struct page *page, unsigned int count);
> diff --git a/mm/internal.h b/mm/internal.h
> index 75ae680d0a2c..5864815947fe 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -201,6 +201,9 @@ extern void post_alloc_hook(struct page *page, unsigned int order,
>  					gfp_t gfp_flags);
>  extern int user_min_free_kbytes;
>  
> +extern void free_unref_page(struct page *page);
> +extern void free_unref_page_list(struct list_head *list);
> +
>  extern void zone_pcp_update(struct zone *zone);
>  extern void zone_pcp_reset(struct zone *zone);
>  extern void zone_pcp_disable(struct zone *zone);
> -- 
> 2.29.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 1/2] sparc: Fix handling of page table constructor failure

[Matthew Wilcox]

On Wed, Nov 25, 2020 at 09:43:15AM +0100, David Hildenbrand wrote:
> On 25.11.20 04:46, Matthew Wilcox (Oracle) wrote:
> > The page has just been allocated, so its refcount is 1.  free_unref_page()
> > is for use on pages which have a zero refcount.  Use __free_page()
> > like the other implementations of pte_alloc_one().
> > 
> > Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  arch/sparc/mm/init_64.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> > index 96edf64d4fb3..182bb7bdaa0a 100644
> > --- a/arch/sparc/mm/init_64.c
> > +++ b/arch/sparc/mm/init_64.c
> > @@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
> >  	if (!page)
> >  		return NULL;
> >  	if (!pgtable_pte_page_ctor(page)) {
> > -		free_unref_page(page);
> > +		__free_page(page);
> >  		return NULL;
> >  	}
> >  	return (pte_t *) page_address(page);
> > 
> 
> I wonder if reusing __pte_alloc_one() - e.g., internally - would be even
> cleaner.

It's really awkward to do because pgtable_t is defined differently.
The clean thing to do would be:

--- arch/sparc/include/asm/page_64.h
-typedef pte_t *pgtable_t;
+typedef struct page *pgtable_t;

and then do all the other changes that would require.

But that feels like a lot more work than appropriate to fix this
unlikely bug.

Re: [PATCH 1/2] sparc: Fix handling of page table constructor failure

[Vlastimil Babka]

On 11/25/20 4:46 AM, Matthew Wilcox (Oracle) wrote:
> The page has just been allocated, so its refcount is 1.  free_unref_page()
> is for use on pages which have a zero refcount.  Use __free_page()
> like the other implementations of pte_alloc_one().
> 
> Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>   arch/sparc/mm/init_64.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 96edf64d4fb3..182bb7bdaa0a 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
>   	if (!page)
>   		return NULL;
>   	if (!pgtable_pte_page_ctor(page)) {
> -		free_unref_page(page);
> +		__free_page(page);
>   		return NULL;
>   	}
>   	return (pte_t *) page_address(page);
> 

Re: [PATCH 2/2] mm: Move free_unref_page to mm/internal.h

[Vlastimil Babka]

On 11/25/20 4:46 AM, Matthew Wilcox (Oracle) wrote:
> Code outside mm/ should not be calling free_unref_page().  Also
> move free_unref_page_list().

Good idea.

> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

There seems to be some effort to remove "extern" from function 
declarations from headers. Do we want to do that, at once, or piecemeal? 
If the latter, this is a chance for these functions at least :)

> ---
>   include/linux/gfp.h | 2 --
>   mm/internal.h       | 3 +++
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index c603237e006c..6e479e9c48ce 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -580,8 +580,6 @@ void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask);
>   
>   extern void __free_pages(struct page *page, unsigned int order);
>   extern void free_pages(unsigned long addr, unsigned int order);
> -extern void free_unref_page(struct page *page);
> -extern void free_unref_page_list(struct list_head *list);
>   
>   struct page_frag_cache;
>   extern void __page_frag_cache_drain(struct page *page, unsigned int count);
> diff --git a/mm/internal.h b/mm/internal.h
> index 75ae680d0a2c..5864815947fe 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -201,6 +201,9 @@ extern void post_alloc_hook(struct page *page, unsigned int order,
>   					gfp_t gfp_flags);
>   extern int user_min_free_kbytes;
>   
> +extern void free_unref_page(struct page *page);
> +extern void free_unref_page_list(struct list_head *list);
> +
>   extern void zone_pcp_update(struct zone *zone);
>   extern void zone_pcp_reset(struct zone *zone);
>   extern void zone_pcp_disable(struct zone *zone);
> 

Re: [PATCH 2/2] mm: Move free_unref_page to mm/internal.h

[Matthew Wilcox]

On Wed, Nov 25, 2020 at 01:34:04PM +0100, Vlastimil Babka wrote:
> On 11/25/20 4:46 AM, Matthew Wilcox (Oracle) wrote:
> > Code outside mm/ should not be calling free_unref_page().  Also
> > move free_unref_page_list().
> 
> Good idea.
> 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
> There seems to be some effort to remove "extern" from function declarations
> from headers. Do we want to do that, at once, or piecemeal? If the latter,
> this is a chance for these functions at least :)

I'm generally in favour of those efforts, but since I was just moving
the function declarations, and all the code near the destination was
using 'extern', I decided not to remove it so as to avoid getting caught
up in bikeshedding ...

Re: [PATCH 1/2] sparc: Fix handling of page table constructor failure

[David Hildenbrand]

On 25.11.20 13:10, Matthew Wilcox wrote:
> On Wed, Nov 25, 2020 at 09:43:15AM +0100, David Hildenbrand wrote:
>> On 25.11.20 04:46, Matthew Wilcox (Oracle) wrote:
>>> The page has just been allocated, so its refcount is 1.  free_unref_page()
>>> is for use on pages which have a zero refcount.  Use __free_page()
>>> like the other implementations of pte_alloc_one().
>>>
>>> Fixes: 1ae9ae5f7df7 ("sparc: handle pgtable_page_ctor() fail")
>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>> ---
>>>  arch/sparc/mm/init_64.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>>> index 96edf64d4fb3..182bb7bdaa0a 100644
>>> --- a/arch/sparc/mm/init_64.c
>>> +++ b/arch/sparc/mm/init_64.c
>>> @@ -2894,7 +2894,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm)
>>>  	if (!page)
>>>  		return NULL;
>>>  	if (!pgtable_pte_page_ctor(page)) {
>>> -		free_unref_page(page);
>>> +		__free_page(page);
>>>  		return NULL;
>>>  	}
>>>  	return (pte_t *) page_address(page);
>>>
>>
>> I wonder if reusing __pte_alloc_one() - e.g., internally - would be even
>> cleaner.
> 
> It's really awkward to do because pgtable_t is defined differently.
> The clean thing to do would be:
> 
> --- arch/sparc/include/asm/page_64.h
> -typedef pte_t *pgtable_t;
> +typedef struct page *pgtable_t;
> 
> and then do all the other changes that would require.
> 
> But that feels like a lot more work than appropriate to fix this
> unlikely bug.

Yeah, cleanups would have to come on top of the fix of course. But I can
understand that you have plenty of better things to do :) ... maybe
sparc people want to work on that at one point.

-- 
Thanks,

David / dhildenb