[RFC,v1,02/12] mm/usercopy.c: Prepare check_page_span() for PG_reserved changes

Commit Message

David Hildenbrand Oct. 22, 2019, 5:12 p.m. UTC

Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.

Let's make sure that the logic in the function won't change. Once we no
longer set these pages to reserved, we can rework this function to
perform separate checks for ZONE_DEVICE (split from PG_reserved checks).

Cc: Kees Cook <[e-mail]>
Cc: Andrew Morton <[e-mail]>
Cc: Kate Stewart <[e-mail]>
Cc: Allison Randal <[e-mail]>
Cc: "Isaac J. Manjarres" <[e-mail]>
Cc: Qian Cai <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Signed-off-by: David Hildenbrand <[e-mail]>
---
 mm/usercopy.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

David Hildenbrand Oct. 23, 2019, 8:20 a.m. UTC | #1

On 22.10.19 19:12, David Hildenbrand wrote:
> Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> change that.
> 
> Let's make sure that the logic in the function won't change. Once we no
> longer set these pages to reserved, we can rework this function to
> perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
> 
> Cc: Kees Cook <[e-mail]>
> Cc: Andrew Morton <[e-mail]>
> Cc: Kate Stewart <[e-mail]>
> Cc: Allison Randal <[e-mail]>
> Cc: "Isaac J. Manjarres" <[e-mail]>
> Cc: Qian Cai <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>
> ---
>   mm/usercopy.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 660717a1ea5c..a3ac4be35cde 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr, unsigned long n,
>   	 * device memory), or CMA. Otherwise, reject since the object spans
>   	 * several independently allocated pages.
>   	 */
> -	is_reserved = PageReserved(page);
> +	is_reserved = PageReserved(page) || is_zone_device_page(page);
>   	is_cma = is_migrate_cma_page(page);
>   	if (!is_reserved && !is_cma)
>   		usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
>   
>   	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
>   		page = virt_to_head_page(ptr);
> -		if (is_reserved && !PageReserved(page))
> +		if (is_reserved && !(PageReserved(page) ||
> +				     is_zone_device_page(page)))
>   			usercopy_abort("spans Reserved and non-Reserved pages",
>   				       NULL, to_user, 0, n);
>   		if (is_cma && !is_migrate_cma_page(page))
> 

@Kees, would it be okay to stop checking against ZONE_DEVICE pages here 
or is there a good rationale behind this?

(I would turn this patch into a simple update of the comment if we agree 
that we don't care)

Kees Cook Oct. 23, 2019, 4:25 p.m. UTC | #2

On Wed, Oct 23, 2019 at 10:20:14AM +0200, David Hildenbrand wrote:
> On 22.10.19 19:12, David Hildenbrand wrote:
> > Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> > change that.
> > 
> > Let's make sure that the logic in the function won't change. Once we no
> > longer set these pages to reserved, we can rework this function to
> > perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
> > 
> > Cc: Kees Cook <[e-mail]>
> > Cc: Andrew Morton <[e-mail]>
> > Cc: Kate Stewart <[e-mail]>
> > Cc: Allison Randal <[e-mail]>
> > Cc: "Isaac J. Manjarres" <[e-mail]>
> > Cc: Qian Cai <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> > Signed-off-by: David Hildenbrand <[e-mail]>
> > ---
> >   mm/usercopy.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 660717a1ea5c..a3ac4be35cde 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr, unsigned long n,
> >   	 * device memory), or CMA. Otherwise, reject since the object spans
> >   	 * several independently allocated pages.
> >   	 */
> > -	is_reserved = PageReserved(page);
> > +	is_reserved = PageReserved(page) || is_zone_device_page(page);
> >   	is_cma = is_migrate_cma_page(page);
> >   	if (!is_reserved && !is_cma)
> >   		usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
> >   	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
> >   		page = virt_to_head_page(ptr);
> > -		if (is_reserved && !PageReserved(page))
> > +		if (is_reserved && !(PageReserved(page) ||
> > +				     is_zone_device_page(page)))
> >   			usercopy_abort("spans Reserved and non-Reserved pages",
> >   				       NULL, to_user, 0, n);
> >   		if (is_cma && !is_migrate_cma_page(page))
> > 
> 
> @Kees, would it be okay to stop checking against ZONE_DEVICE pages here or
> is there a good rationale behind this?
> 
> (I would turn this patch into a simple update of the comment if we agree
> that we don't care)

There has been work to actually remove the page span checks entirely,
but there wasn't consensus on what the right way forward was. I continue
to leaning toward just dropping it entirely, but Matthew Wilcox has some
alternative ideas that could use some further thought/testing.

David Hildenbrand Oct. 23, 2019, 4:32 p.m. UTC | #3

On 23.10.19 18:25, Kees Cook wrote:
> On Wed, Oct 23, 2019 at 10:20:14AM +0200, David Hildenbrand wrote:
>> On 22.10.19 19:12, David Hildenbrand wrote:
>>> Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
>>> change that.
>>>
>>> Let's make sure that the logic in the function won't change. Once we no
>>> longer set these pages to reserved, we can rework this function to
>>> perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
>>>
>>> Cc: Kees Cook <[e-mail]>
>>> Cc: Andrew Morton <[e-mail]>
>>> Cc: Kate Stewart <[e-mail]>
>>> Cc: Allison Randal <[e-mail]>
>>> Cc: "Isaac J. Manjarres" <[e-mail]>
>>> Cc: Qian Cai <[e-mail]>
>>> Cc: Thomas Gleixner <[e-mail]>
>>> Signed-off-by: David Hildenbrand <[e-mail]>
>>> ---
>>>   mm/usercopy.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>>> index 660717a1ea5c..a3ac4be35cde 100644
>>> --- a/mm/usercopy.c
>>> +++ b/mm/usercopy.c
>>> @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr, unsigned long n,
>>>   	 * device memory), or CMA. Otherwise, reject since the object spans
>>>   	 * several independently allocated pages.
>>>   	 */
>>> -	is_reserved = PageReserved(page);
>>> +	is_reserved = PageReserved(page) || is_zone_device_page(page);
>>>   	is_cma = is_migrate_cma_page(page);
>>>   	if (!is_reserved && !is_cma)
>>>   		usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
>>>   	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
>>>   		page = virt_to_head_page(ptr);
>>> -		if (is_reserved && !PageReserved(page))
>>> +		if (is_reserved && !(PageReserved(page) ||
>>> +				     is_zone_device_page(page)))
>>>   			usercopy_abort("spans Reserved and non-Reserved pages",
>>>   				       NULL, to_user, 0, n);
>>>   		if (is_cma && !is_migrate_cma_page(page))
>>>
>>
>> @Kees, would it be okay to stop checking against ZONE_DEVICE pages here or
>> is there a good rationale behind this?
>>
>> (I would turn this patch into a simple update of the comment if we agree
>> that we don't care)
> 
> There has been work to actually remove the page span checks entirely,
> but there wasn't consensus on what the right way forward was. I continue
> to leaning toward just dropping it entirely, but Matthew Wilcox has some
> alternative ideas that could use some further thought/testing.

Thanks for your reply!

So, the worst thing that could happen right now, when dropping this
patch, is that we would reject some ranges when hardening is on,
correct? (sounds like that can easily be found by testing if it is
actually relevant)

Do you remember if there were real ZONE_DEVICE usecases that required
this filter to be in place for PG_reserved pages?

Patch

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 660717a1ea5c..a3ac4be35cde 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -203,14 +203,15 @@  static inline void check_page_span(const void *ptr, unsigned long n,
 	 * device memory), or CMA. Otherwise, reject since the object spans
 	 * several independently allocated pages.
 	 */
-	is_reserved = PageReserved(page);
+	is_reserved = PageReserved(page) || is_zone_device_page(page);
 	is_cma = is_migrate_cma_page(page);
 	if (!is_reserved && !is_cma)
 		usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
 
 	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
 		page = virt_to_head_page(ptr);
-		if (is_reserved && !PageReserved(page))
+		if (is_reserved && !(PageReserved(page) ||
+				     is_zone_device_page(page)))
 			usercopy_abort("spans Reserved and non-Reserved pages",
 				       NULL, to_user, 0, n);
 		if (is_cma && !is_migrate_cma_page(page))