linux-next: manual merge of the akpm-current tree with the kvms390 tree

linux-next: manual merge of the akpm-current tree with the kvms390 tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

Hi all,

Today's linux-next merge of the akpm-current tree got a conflict in:

  mm/gup.c

between commit:

  732b80e677b8 ("mm/gup/writeback: add callbacks for inaccessible pages")

from the kvms390 tree and commit:

  9947ea2c1e60 ("mm/gup: track FOLL_PIN pages")

from the akpm-current tree.

I fixed it up (see below - maybe not optimally) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging.  You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc mm/gup.c
index 354bcfbd844b,f589299b0d4a..000000000000
--- a/mm/gup.c
+++ b/mm/gup.c
@@@ -269,18 -470,11 +468,19 @@@ retry
  		goto retry;
  	}
  
+ 	/* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */
+ 	if (unlikely(!try_grab_page(page, flags))) {
+ 		page = ERR_PTR(-ENOMEM);
+ 		goto out;
+ 	}
 +	if (flags & FOLL_GET) {
- 		if (unlikely(!try_get_page(page))) {
- 			page = ERR_PTR(-ENOMEM);
- 			goto out;
- 		}
 +		ret = arch_make_page_accessible(page);
 +		if (ret) {
 +			put_page(page);
 +			page = ERR_PTR(ret);
 +			goto out;
 +		}
 +	}
  	if (flags & FOLL_TOUCH) {
  		if ((flags & FOLL_WRITE) &&
  		    !pte_dirty(pte) && !PageDirty(page))

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: manual merge of the akpm-current tree with the kvms390 tree

[John Hubbard]

On 2/26/20 7:11 PM, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the akpm-current tree got a conflict in:
> 
>   mm/gup.c
> 
> between commit:
> 
>   732b80e677b8 ("mm/gup/writeback: add callbacks for inaccessible pages")
> 
> from the kvms390 tree and commit:
> 
>   9947ea2c1e60 ("mm/gup: track FOLL_PIN pages")
> 
> from the akpm-current tree.
> 
> I fixed it up (see below - maybe not optimally) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging.  You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
> 

Yes. Changes to mm/gup.c really should normally go through linux-mm and 
Andrew's tree, if at all possible. This would have been caught, and figured out
on linux-mm, had that been done--instead of leaving the linux-next maintainer
trying to guess at how to resolve the conflict.

+Cc David Hildenbrand, who I see looked at the kvms390 proposed patch a bit.
Maybe he has some opinions, especially about my questions below.

The fix-up below may (or may not) need some changes:


diff --cc mm/gup.c
index 354bcfbd844b,f589299b0d4a..000000000000
--- a/mm/gup.c
+++ b/mm/gup.c
@@@ -269,18 -470,11 +468,19 @@@ retry
  		goto retry;
  	}
  
+ 	/* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */
+ 	if (unlikely(!try_grab_page(page, flags))) {
+ 		page = ERR_PTR(-ENOMEM);
+ 		goto out;
+ 	}
 +	if (flags & FOLL_GET) {


If I'm reading the diff correctly, I believe that line should *maybe* be changed to:

	if (flags & (FOLL_GET | FOLL_PIN)) {

...because each of those flags has a similar effect: pinned pages for DMA or RDMA
use. So either flag will require a call to arch_make_page_accessible()...except that
I'm not sure that's what you want. Would the absence of a call to 
arch_make_page_accessible() cause things like pin_user_pages() to not work correctly?
Seems like it would, to me.

(I'm pretty unhappy that we have to ask this at the linux-next level.)

Also below...


- 		if (unlikely(!try_get_page(page))) {
- 			page = ERR_PTR(-ENOMEM);
- 			goto out;
- 		}
 +		ret = arch_make_page_accessible(page);
 +		if (ret) {
 +			put_page(page);


put_page() only works with FOLL_GET. So if we do allow to get here via either FOLL_GET or
FOLL_PIN, the we need to do an unpin_user_page(), like this:

		if (flags & FOLL_PIN)
			unpin_user_page(page);
		else
			put_page(page);



 +			page = ERR_PTR(ret);
 +			goto out;
 +		}
 +	}
  	if (flags & FOLL_TOUCH) {
  		if ((flags & FOLL_WRITE) &&
  		    !pte_dirty(pte) && !PageDirty(page))

thanks,
-- 
John Hubbard
NVIDIA

Re: linux-next: manual merge of the akpm-current tree with the kvms390 tree

[Christian Borntraeger]

On 27.02.20 06:58, John Hubbard wrote:
> On 2/26/20 7:11 PM, Stephen Rothwell wrote:
>> Hi all,
>>
>> Today's linux-next merge of the akpm-current tree got a conflict in:
>>
>>   mm/gup.c
>>
>> between commit:
>>
>>   732b80e677b8 ("mm/gup/writeback: add callbacks for inaccessible pages")
>>
>> from the kvms390 tree and commit:
>>
>>   9947ea2c1e60 ("mm/gup: track FOLL_PIN pages")
>>
>> from the akpm-current tree.
>>
>> I fixed it up (see below - maybe not optimally) and can carry the fix as
>> necessary. This is now fixed as far as linux-next is concerned, but any
>> non trivial conflicts should be mentioned to your upstream maintainer
>> when your tree is submitted for merging.  You may also want to consider
>> cooperating with the maintainer of the conflicting tree to minimise any
>> particularly complex conflicts.
>>
> 
> Yes. Changes to mm/gup.c really should normally go through linux-mm and 
> Andrew's tree, if at all possible. This would have been caught, and figured out
> on linux-mm, had that been done--instead of leaving the linux-next maintainer
> trying to guess at how to resolve the conflict.

Yes. This patch should go via Andrew. Claudio is going to provide a fixed up
version that takes care of the new semantics.

This patch was posted several times on linux-mm (also before rc1) and I will 
drop it from my tree due to the conflict.



> 
> +Cc David Hildenbrand, who I see looked at the kvms390 proposed patch a bit.
> Maybe he has some opinions, especially about my questions below.
> 
> The fix-up below may (or may not) need some changes:



> 
> 
> diff --cc mm/gup.c
> index 354bcfbd844b,f589299b0d4a..000000000000
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@@ -269,18 -470,11 +468,19 @@@ retry
>   		goto retry;
>   	}
>   
> + 	/* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */
> + 	if (unlikely(!try_grab_page(page, flags))) {
> + 		page = ERR_PTR(-ENOMEM);
> + 		goto out;
> + 	}
>  +	if (flags & FOLL_GET) {
> 
> 
> If I'm reading the diff correctly, I believe that line should *maybe* be changed to:
> 
> 	if (flags & (FOLL_GET | FOLL_PIN)) {
> 
> ...because each of those flags has a similar effect: pinned pages for DMA or RDMA
> use. So either flag will require a call to arch_make_page_accessible()...except that
> I'm not sure that's what you want. Would the absence of a call to 
> arch_make_page_accessible() cause things like pin_user_pages() to not work correctly?
> Seems like it would, to me.
> 
> (I'm pretty unhappy that we have to ask this at the linux-next level.)
> 
> Also below...
> 
> 
> - 		if (unlikely(!try_get_page(page))) {
> - 			page = ERR_PTR(-ENOMEM);
> - 			goto out;
> - 		}
>  +		ret = arch_make_page_accessible(page);
>  +		if (ret) {
>  +			put_page(page);
> 
> 
> put_page() only works with FOLL_GET. So if we do allow to get here via either FOLL_GET or
> FOLL_PIN, the we need to do an unpin_user_page(), like this:
> 
> 		if (flags & FOLL_PIN)
> 			unpin_user_page(page);
> 		else
> 			put_page(page);
> 
> 
> 
>  +			page = ERR_PTR(ret);
>  +			goto out;
>  +		}
>  +	}
>   	if (flags & FOLL_TOUCH) {
>   		if ((flags & FOLL_WRITE) &&
>   		    !pte_dirty(pte) && !PageDirty(page))
> 
> thanks,
> 

Re: linux-next: manual merge of the akpm-current tree with the kvms390 tree

[David Hildenbrand]

> Yes. Changes to mm/gup.c really should normally go through linux-mm and 
> Andrew's tree, if at all possible. This would have been caught, and figured out
> on linux-mm, had that been done--instead of leaving the linux-next maintainer
> trying to guess at how to resolve the conflict.
> 
> +Cc David Hildenbrand, who I see looked at the kvms390 proposed patch a bit.
> Maybe he has some opinions, especially about my questions below.

I'll leave figuring out the details to Christian/Claudio (-EBUSY) :)

> 
> The fix-up below may (or may not) need some changes:
> 
> 
> diff --cc mm/gup.c
> index 354bcfbd844b,f589299b0d4a..000000000000
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@@ -269,18 -470,11 +468,19 @@@ retry
>   		goto retry;
>   	}
>   
> + 	/* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */
> + 	if (unlikely(!try_grab_page(page, flags))) {
> + 		page = ERR_PTR(-ENOMEM);
> + 		goto out;
> + 	}
>  +	if (flags & FOLL_GET) {
> 
> 
> If I'm reading the diff correctly, I believe that line should *maybe* be changed to:
> 
> 	if (flags & (FOLL_GET | FOLL_PIN)) {
> 
> ...because each of those flags has a similar effect: pinned pages for DMA or RDMA
> use. So either flag will require a call to arch_make_page_accessible()...except that
> I'm not sure that's what you want. Would the absence of a call to 
> arch_make_page_accessible() cause things like pin_user_pages() to not work correctly?
> Seems like it would, to me.

Yes, it's required. From the commit message "enable paging, file backing
etc, it is also necessary to protect the host against a malicious user
space. For example a bad QEMU could simply start direct I/O on such
protected memory.". So we really want to convert the page from
unencrypted/inaccessible to encrypted/accessible at this point (iow,
make it definitely accessible, and make sure it stays accessible).

> 
> (I'm pretty unhappy that we have to ask this at the linux-next level.)

Yeah, I *think* this fell through the cracks (on linux-mm, but also in
Andrew's inbox) because the series has a big fat "KVM: s390:" as prefix.
Christian decided to pull it in to give it some churn yesterday (I think
he originally wanted to have this patch and the other KVM protvirt
patches in 5.7 [2] ... but not sure what will happen due to this conflict).

At least now this patch has attention ... although it would have been
better if linux-next admins wouldn't have to mess with this :)

[1] https://lkml.kernel.org/r/20200224114107.4646-2-borntraeger@de.ibm.com
[2] https://lkml.kernel.org/r/20200224114107.4646-1-borntraeger@de.ibm.com

-- 
Thanks,

David / dhildenb

Re: linux-next: manual merge of the akpm-current tree with the kvms390 tree

[Christian Borntraeger]

On 27.02.20 10:20, David Hildenbrand wrote:
>> Yes. Changes to mm/gup.c really should normally go through linux-mm and 
>> Andrew's tree, if at all possible. This would have been caught, and figured out
>> on linux-mm, had that been done--instead of leaving the linux-next maintainer
>> trying to guess at how to resolve the conflict.
>>
>> +Cc David Hildenbrand, who I see looked at the kvms390 proposed patch a bit.
>> Maybe he has some opinions, especially about my questions below.
> 
> I'll leave figuring out the details to Christian/Claudio (-EBUSY) :)
> 
>>
>> The fix-up below may (or may not) need some changes:
>>
>>
>> diff --cc mm/gup.c
>> index 354bcfbd844b,f589299b0d4a..000000000000
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@@ -269,18 -470,11 +468,19 @@@ retry
>>   		goto retry;
>>   	}
>>   
>> + 	/* try_grab_page() does nothing unless FOLL_GET or FOLL_PIN is set. */
>> + 	if (unlikely(!try_grab_page(page, flags))) {
>> + 		page = ERR_PTR(-ENOMEM);
>> + 		goto out;
>> + 	}
>>  +	if (flags & FOLL_GET) {
>>
>>
>> If I'm reading the diff correctly, I believe that line should *maybe* be changed to:
>>
>> 	if (flags & (FOLL_GET | FOLL_PIN)) {
>>
>> ...because each of those flags has a similar effect: pinned pages for DMA or RDMA
>> use. So either flag will require a call to arch_make_page_accessible()...except that
>> I'm not sure that's what you want. Would the absence of a call to 
>> arch_make_page_accessible() cause things like pin_user_pages() to not work correctly?
>> Seems like it would, to me.
> 
> Yes, it's required. From the commit message "enable paging, file backing
> etc, it is also necessary to protect the host against a malicious user
> space. For example a bad QEMU could simply start direct I/O on such
> protected memory.". So we really want to convert the page from
> unencrypted/inaccessible to encrypted/accessible at this point (iow,
> make it definitely accessible, and make sure it stays accessible).
> 
>>
>> (I'm pretty unhappy that we have to ask this at the linux-next level.)
> 
> Yeah, I *think* this fell through the cracks (on linux-mm, but also in
> Andrew's inbox) because the series has a big fat "KVM: s390:" as prefix.
> Christian decided to pull it in to give it some churn yesterday (I think
> he originally wanted to have this patch and the other KVM protvirt
> patches in 5.7 [2] ... but not sure what will happen due to this conflict).

Yes, I would like to have this patch in 5.7. Depending on the schedule of the
FOLL_PIN patches that means:
1. Claudios callback patch _before_ the FOLL_PIN patches + Claudio will provide a fixup.
2. Claudios callback patch on top of the FOLL_PIN patches (Claudio will provide a
version that combines the first patch + fixup)


> 
> At least now this patch has attention ... although it would have been
> better if linux-next admins wouldn't have to mess with this :)
> 
> [1] https://lkml.kernel.org/r/20200224114107.4646-2-borntraeger@de.ibm.com
> [2] https://lkml.kernel.org/r/20200224114107.4646-1-borntraeger@de.ibm.com
>