[PATCH 0/3] s390/vmem: minor cleanup

[PATCH 0/3] s390/vmem: minor cleanup

[Alexander Gordeev]

Hi David,

Nothing really fancy here, just couple of cleanups

Alexander Gordeev (3):
  s390/vmem: remove redundant check
  s390/vmem: fix possible memory overwrite
  s390/vmem: make variable and function names consistent

 arch/s390/mm/vmem.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

-- 
2.26.0

[PATCH 1/3] s390/vmem: remove redundant check

[Alexander Gordeev]

Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/mm/vmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index b239f2ba93b09..56ab9bb770f3a 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -223,7 +223,7 @@ static int __ref modify_pmd_table(pud_t *pud, unsigned long addr,
 		if (!add) {
 			if (pmd_none(*pmd))
 				continue;
-			if (pmd_large(*pmd) && !add) {
+			if (pmd_large(*pmd)) {
 				if (IS_ALIGNED(addr, PMD_SIZE) &&
 				    IS_ALIGNED(next, PMD_SIZE)) {
 					if (!direct)
-- 
2.26.0

[PATCH 2/3] s390/vmem: fix possible memory overwrite

[Alexander Gordeev]

If for whatever reason the sub-PMD region to be used is less
than struct page size (e.g in the future), then it is possible
to overwrite beyond the region size.

Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/mm/vmem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index 56ab9bb770f3a..d7f25884061f4 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -91,13 +91,15 @@ static void vmemmap_flush_unused_pmd(void)
 
 static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
 {
+	unsigned long size = min(end - start, sizeof(struct page));
+
 	/*
 	 * As we expect to add in the same granularity as we remove, it's
 	 * sufficient to mark only some piece used to block the memmap page from
 	 * getting removed (just in case the memmap never gets initialized,
 	 * e.g., because the memory block never gets onlined).
 	 */
-	memset(__va(start), 0, sizeof(struct page));
+	memset(__va(start), 0, size);
 }
 
 static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
-- 
2.26.0

[PATCH 3/3] s390/vmem: make variable and function names consistent

[Alexander Gordeev]

Rename some variable and functions to better clarify
what they are and what they do.

Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 arch/s390/mm/vmem.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index d7f25884061f4..4bb198db6aa01 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -76,20 +76,20 @@ static void vmem_pte_free(unsigned long *table)
 
 /*
  * The unused vmemmap range, which was not yet memset(PAGE_UNUSED) ranges
- * from unused_pmd_start to next PMD_SIZE boundary.
+ * from unused_sub_pmd_start to next PMD_SIZE boundary.
  */
-static unsigned long unused_pmd_start;
+static unsigned long unused_sub_pmd_start;
 
-static void vmemmap_flush_unused_pmd(void)
+static void vmemmap_flush_unused_sub_pmd(void)
 {
-	if (!unused_pmd_start)
+	if (!unused_sub_pmd_start)
 		return;
-	memset(__va(unused_pmd_start), PAGE_UNUSED,
-	       ALIGN(unused_pmd_start, PMD_SIZE) - unused_pmd_start);
-	unused_pmd_start = 0;
+	memset(__va(unused_sub_pmd_start), PAGE_UNUSED,
+	       ALIGN(unused_sub_pmd_start, PMD_SIZE) - unused_sub_pmd_start);
+	unused_sub_pmd_start = 0;
 }
 
-static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
+static void vmemmap_mark_sub_pmd_used(unsigned long start, unsigned long end)
 {
 	unsigned long size = min(end - start, sizeof(struct page));
 
@@ -108,24 +108,24 @@ static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
 	 * We only optimize if the new used range directly follows the
 	 * previously unused range (esp., when populating consecutive sections).
 	 */
-	if (unused_pmd_start == start) {
-		unused_pmd_start = end;
-		if (likely(IS_ALIGNED(unused_pmd_start, PMD_SIZE)))
-			unused_pmd_start = 0;
+	if (unused_sub_pmd_start == start) {
+		unused_sub_pmd_start = end;
+		if (likely(IS_ALIGNED(unused_sub_pmd_start, PMD_SIZE)))
+			unused_sub_pmd_start = 0;
 		return;
 	}
-	vmemmap_flush_unused_pmd();
-	__vmemmap_use_sub_pmd(start, end);
+	vmemmap_flush_unused_sub_pmd();
+	vmemmap_mark_sub_pmd_used(start, end);
 }
 
 static void vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end)
 {
 	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
 
-	vmemmap_flush_unused_pmd();
+	vmemmap_flush_unused_sub_pmd();
 
 	/* Could be our memmap page is filled with PAGE_UNUSED already ... */
-	__vmemmap_use_sub_pmd(start, end);
+	vmemmap_mark_sub_pmd_used(start, end);
 
 	/* Mark the unused parts of the new memmap page PAGE_UNUSED. */
 	if (!IS_ALIGNED(start, PMD_SIZE))
@@ -136,7 +136,7 @@ static void vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end)
 	 * unused range in the populated PMD.
 	 */
 	if (!IS_ALIGNED(end, PMD_SIZE))
-		unused_pmd_start = end;
+		unused_sub_pmd_start = end;
 }
 
 /* Returns true if the PMD is completely unused and can be freed. */
@@ -144,7 +144,7 @@ static bool vmemmap_unuse_sub_pmd(unsigned long start, unsigned long end)
 {
 	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
 
-	vmemmap_flush_unused_pmd();
+	vmemmap_flush_unused_sub_pmd();
 	memset(__va(start), PAGE_UNUSED, end - start);
 	return !memchr_inv(page, PAGE_UNUSED, PMD_SIZE);
 }
-- 
2.26.0

Re: [PATCH 2/3] s390/vmem: fix possible memory overwrite

[David Hildenbrand]

On 10.11.20 10:36, Alexander Gordeev wrote:
> If for whatever reason the sub-PMD region to be used is less
> than struct page size (e.g in the future), then it is possible
> to overwrite beyond the region size.
> 
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>   arch/s390/mm/vmem.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index 56ab9bb770f3a..d7f25884061f4 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -91,13 +91,15 @@ static void vmemmap_flush_unused_pmd(void)
>   
>   static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
>   {
> +	unsigned long size = min(end - start, sizeof(struct page));
> +
>   	/*
>   	 * As we expect to add in the same granularity as we remove, it's
>   	 * sufficient to mark only some piece used to block the memmap page from
>   	 * getting removed (just in case the memmap never gets initialized,
>   	 * e.g., because the memory block never gets onlined).
>   	 */
> -	memset(__va(start), 0, sizeof(struct page));
> +	memset(__va(start), 0, size);
>   }
>   
>   static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
> 

I don't really see a need for that. Can you spell out one possible 
configuration that would trigger that in the future? It's sounds very 
unlikely and I have the feeling there might be more to change at other 
points.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/3] s390/vmem: remove redundant check

[David Hildenbrand]

On 10.11.20 10:36, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>   arch/s390/mm/vmem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index b239f2ba93b09..56ab9bb770f3a 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -223,7 +223,7 @@ static int __ref modify_pmd_table(pud_t *pud, unsigned long addr,
>   		if (!add) {
>   			if (pmd_none(*pmd))
>   				continue;
> -			if (pmd_large(*pmd) && !add) {
> +			if (pmd_large(*pmd)) {
>   				if (IS_ALIGNED(addr, PMD_SIZE) &&
>   				    IS_ALIGNED(next, PMD_SIZE)) {
>   					if (!direct)
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/3] s390/vmem: fix possible memory overwrite

[Alexander Gordeev]

On Tue, Nov 10, 2020 at 10:41:14AM +0100, David Hildenbrand wrote:
> On 10.11.20 10:36, Alexander Gordeev wrote:
> >If for whatever reason the sub-PMD region to be used is less
> >than struct page size (e.g in the future), then it is possible
> >to overwrite beyond the region size.
> >
> >Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> >---
> >  arch/s390/mm/vmem.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> >diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> >index 56ab9bb770f3a..d7f25884061f4 100644
> >--- a/arch/s390/mm/vmem.c
> >+++ b/arch/s390/mm/vmem.c
> >@@ -91,13 +91,15 @@ static void vmemmap_flush_unused_pmd(void)
> >  static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
> >  {
> >+	unsigned long size = min(end - start, sizeof(struct page));
> >+
> >  	/*
> >  	 * As we expect to add in the same granularity as we remove, it's
> >  	 * sufficient to mark only some piece used to block the memmap page from
> >  	 * getting removed (just in case the memmap never gets initialized,
> >  	 * e.g., because the memory block never gets onlined).
> >  	 */
> >-	memset(__va(start), 0, sizeof(struct page));
> >+	memset(__va(start), 0, size);
> >  }
> >  static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
> >
> 
> I don't really see a need for that. Can you spell out one possible
> configuration that would trigger that in the future? It's sounds
> very unlikely and I have the feeling there might be more to change
> at other points.

No configuration in mind. But dependency on struct page is the only
obstacle that prevents the whole thing to become generic (unless I
am missing something). Moreover, the memset() would not be needed
also - just a single non-PAGE_UNUSED word within [start..end) should
be enough.

> -- 
> Thanks,
> 
> David / dhildenb
> 

Re: [PATCH 1/3] s390/vmem: remove redundant check

[Heiko Carstens]

On Tue, Nov 10, 2020 at 10:36:21AM +0100, Alexander Gordeev wrote:
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/s390/mm/vmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index b239f2ba93b09..56ab9bb770f3a 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -223,7 +223,7 @@ static int __ref modify_pmd_table(pud_t *pud, unsigned long addr,
>  		if (!add) {
>  			if (pmd_none(*pmd))
>  				continue;
> -			if (pmd_large(*pmd) && !add) {
> +			if (pmd_large(*pmd)) {

Applied, thanks!

Re: [PATCH 2/3] s390/vmem: fix possible memory overwrite

[Heiko Carstens]

On Tue, Nov 10, 2020 at 11:50:52AM +0100, Alexander Gordeev wrote:
> > >  static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
> > >  {
> > >+	unsigned long size = min(end - start, sizeof(struct page));
> > >+
> > >  	/*
> > >  	 * As we expect to add in the same granularity as we remove, it's
> > >  	 * sufficient to mark only some piece used to block the memmap page from
> > >  	 * getting removed (just in case the memmap never gets initialized,
> > >  	 * e.g., because the memory block never gets onlined).
> > >  	 */
> > >-	memset(__va(start), 0, sizeof(struct page));
> > >+	memset(__va(start), 0, size);
> > >  }
> > >  static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
> > >
> > 
> > I don't really see a need for that. Can you spell out one possible
> > configuration that would trigger that in the future? It's sounds
> > very unlikely and I have the feeling there might be more to change
> > at other points.
> 
> No configuration in mind. But dependency on struct page is the only
> obstacle that prevents the whole thing to become generic (unless I
> am missing something). Moreover, the memset() would not be needed
> also - just a single non-PAGE_UNUSED word within [start..end) should
> be enough.

Well, I agree with David here - so not applying.

Re: [PATCH 3/3] s390/vmem: make variable and function names consistent

[Heiko Carstens]

On Tue, Nov 10, 2020 at 10:36:23AM +0100, Alexander Gordeev wrote:
> Rename some variable and functions to better clarify
> what they are and what they do.
> 
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/s390/mm/vmem.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Applied, thanks.