[PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
        https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
---
 security/tomoyo/domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index dc4ecc0..bd748be 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -914,7 +914,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
 	 * (represented by bprm).  'current' is the process doing
 	 * the execve().
 	 */
-	if (get_user_pages_remote(bprm->mm, pos, 1,
+	if (pin_user_pages_remote(bprm->mm, pos, 1,
 				FOLL_FORCE, &page, NULL, NULL) <= 0)
 		return false;
 #else
@@ -936,7 +936,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
 	}
 	/* Same with put_arg_page(page) in fs/exec.c */
 #ifdef CONFIG_MMU
-	put_page(page);
+	unpin_user_page(page);
 #endif
 	return true;
 }
-- 
1.9.1

[PATCH 2/2] tomoyo: Fixed typo in documentation

[Souptick Joarder]

Fixed typo s/Poiner/Pointer

Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as conditions.")
Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
---
 security/tomoyo/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index bd748be..7b2babe 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
  *
  * @bprm: Pointer to "struct linux_binprm".
  * @pos:  Location to dump.
- * @dump: Poiner to "struct tomoyo_page_dump".
+ * @dump: Pointer to "struct tomoyo_page_dump".
  *
  * Returns true on success, false otherwise.
  */
-- 
1.9.1

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[John Hubbard]

On 11/7/20 12:24 AM, Souptick Joarder wrote:
> Fixed typo s/Poiner/Pointer
> 
> Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as conditions.")
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
>   security/tomoyo/domain.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> index bd748be..7b2babe 100644
> --- a/security/tomoyo/domain.c
> +++ b/security/tomoyo/domain.c
> @@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
>    *
>    * @bprm: Pointer to "struct linux_binprm".
>    * @pos:  Location to dump.
> - * @dump: Poiner to "struct tomoyo_page_dump".
> + * @dump: Pointer to "struct tomoyo_page_dump".

Not worth a separate patch, especially since the original comment is merely
copying the C sources, and as such, does not add any value.

I'd either a) craft a new documentation line that adds some value, or b) just
merge this patch into the previous one, and make a note in the commit
description to the effect that you've included a trivial typo fix as long
as you're there.


thanks,
-- 
John Hubbard
NVIDIA

>    *
>    * Returns true on success, false otherwise.
>    */
> 

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[Souptick Joarder]

On Sat, Nov 7, 2020 at 2:27 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 11/7/20 12:24 AM, Souptick Joarder wrote:
> > Fixed typo s/Poiner/Pointer
> >
> > Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as conditions.")
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > ---
> >   security/tomoyo/domain.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> > index bd748be..7b2babe 100644
> > --- a/security/tomoyo/domain.c
> > +++ b/security/tomoyo/domain.c
> > @@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
> >    *
> >    * @bprm: Pointer to "struct linux_binprm".
> >    * @pos:  Location to dump.
> > - * @dump: Poiner to "struct tomoyo_page_dump".
> > + * @dump: Pointer to "struct tomoyo_page_dump".
>
> Not worth a separate patch, especially since the original comment is merely
> copying the C sources, and as such, does not add any value.
>
> I'd either a) craft a new documentation line that adds some value, or b) just
> merge this patch into the previous one, and make a note in the commit
> description to the effect that you've included a trivial typo fix as long
> as you're there.
>

John, as patch[1/2] is dropped, can we take this patch forward with some more
updates in documentations ?

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[John Hubbard]

On 11/8/20 7:41 PM, Souptick Joarder wrote:
> On Sat, Nov 7, 2020 at 2:27 PM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
>>> Fixed typo s/Poiner/Pointer
>>>
>>> Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as conditions.")
>>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> ---
>>>    security/tomoyo/domain.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
>>> index bd748be..7b2babe 100644
>>> --- a/security/tomoyo/domain.c
>>> +++ b/security/tomoyo/domain.c
>>> @@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
>>>     *
>>>     * @bprm: Pointer to "struct linux_binprm".
>>>     * @pos:  Location to dump.
>>> - * @dump: Poiner to "struct tomoyo_page_dump".
>>> + * @dump: Pointer to "struct tomoyo_page_dump".
>>
>> Not worth a separate patch, especially since the original comment is merely
>> copying the C sources, and as such, does not add any value.
>>
>> I'd either a) craft a new documentation line that adds some value, or b) just
>> merge this patch into the previous one, and make a note in the commit
>> description to the effect that you've included a trivial typo fix as long
>> as you're there.
>>
> 
> John, as patch[1/2] is dropped, can we take this patch forward with some more
> updates in documentations ?
> 

That's really up to the folks who work on this code. Personally I would rarely
post a patch *just* for this, but on the other hand it is a correction. Either
way is fine with me of course.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[Tetsuo Handa]

On 2020/11/09 13:17, John Hubbard wrote:
> On 11/8/20 7:41 PM, Souptick Joarder wrote:
>> John, as patch[1/2] is dropped, can we take this patch forward with some more
>> updates in documentations ?
>>
> 
> That's really up to the folks who work on this code. Personally I would rarely
> post a patch *just* for this, but on the other hand it is a correction. Either
> way is fine with me of course.

Xiaoming Ni has posted a patch for fixing spelling errors in security/ directory
at https://lkml.kernel.org/r/20200822030534.2577-1-nixiaoming@huawei.com .

James, can we apply that patch? Or, should we split that patch into each subsystem?

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[James Morris]

On Mon, 9 Nov 2020, Tetsuo Handa wrote:

> On 2020/11/09 13:17, John Hubbard wrote:
> > On 11/8/20 7:41 PM, Souptick Joarder wrote:
> >> John, as patch[1/2] is dropped, can we take this patch forward with some more
> >> updates in documentations ?
> >>
> > 
> > That's really up to the folks who work on this code. Personally I would rarely
> > post a patch *just* for this, but on the other hand it is a correction. Either
> > way is fine with me of course.
> 
> Xiaoming Ni has posted a patch for fixing spelling errors in security/ directory
> at https://lkml.kernel.org/r/20200822030534.2577-1-nixiaoming@huawei.com .
> 
> James, can we apply that patch? Or, should we split that patch into each subsystem?
> 

Probably easiest to go via my tree, the author should resend and include 
me on the To: line.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 11/7/20 12:24 AM, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information. This is case 5 as per document [1].

It turns out that Case 5 can be implemented via a better pattern, as long
as we're just dealing with a page at a time, briefly:

lock_page()
write to page's data
unlock_page()

...which neatly synchronizes with writeback and other fs activities.

I was going to track down the Case 5's and do that [1].

+CC Jan and Matthew, to keep us on the straight and narrow, just in case
I'm misunderstanding something.

[1] https://lore.kernel.org/r/e78fb7af-627b-ce80-275e-51f97f1f3168@nvidia.com

thanks,
-- 
John Hubbard
NVIDIA

> 
> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
>          https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
>   security/tomoyo/domain.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> index dc4ecc0..bd748be 100644
> --- a/security/tomoyo/domain.c
> +++ b/security/tomoyo/domain.c
> @@ -914,7 +914,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
>   	 * (represented by bprm).  'current' is the process doing
>   	 * the execve().
>   	 */
> -	if (get_user_pages_remote(bprm->mm, pos, 1,
> +	if (pin_user_pages_remote(bprm->mm, pos, 1,
>   				FOLL_FORCE, &page, NULL, NULL) <= 0)
>   		return false;
>   #else
> @@ -936,7 +936,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned long pos,
>   	}
>   	/* Same with put_arg_page(page) in fs/exec.c */
>   #ifdef CONFIG_MMU
> -	put_page(page);
> +	unpin_user_page(page);
>   #endif
>   	return true;
>   }
> 

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 11/7/20 1:04 AM, John Hubbard wrote:
> On 11/7/20 12:24 AM, Souptick Joarder wrote:
>> In 2019, we introduced pin_user_pages*() and now we are converting
>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>> be referred for more information. This is case 5 as per document [1].
> 
> It turns out that Case 5 can be implemented via a better pattern, as long
> as we're just dealing with a page at a time, briefly:
> 
> lock_page()
> write to page's data
> unlock_page()
> 
> ...which neatly synchronizes with writeback and other fs activities.

Ahem, I left out a key step: set_page_dirty()!

lock_page()
write to page's data
set_page_dirty()
unlock_page()


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Tetsuo Handa]

On 2020/11/08 4:17, John Hubbard wrote:
> On 11/7/20 1:04 AM, John Hubbard wrote:
>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>> be referred for more information. This is case 5 as per document [1].
>>
>> It turns out that Case 5 can be implemented via a better pattern, as long
>> as we're just dealing with a page at a time, briefly:
>>
>> lock_page()
>> write to page's data
>> unlock_page()
>>
>> ...which neatly synchronizes with writeback and other fs activities.
> 
> Ahem, I left out a key step: set_page_dirty()!
> 
> lock_page()
> write to page's data
> set_page_dirty()
> unlock_page()
> 

Excuse me, but Documentation/core-api/pin_user_pages.rst says 
"CASE 5: Pinning in order to _write_ to the data within the page"
while tomoyo_dump_page() is for "_read_ the data within the page".
Do we want to convert to pin_user_pages_remote() or lock_page() ?

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 11/7/20 5:13 PM, Tetsuo Handa wrote:
> On 2020/11/08 4:17, John Hubbard wrote:
>> On 11/7/20 1:04 AM, John Hubbard wrote:
>>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
>>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>>> be referred for more information. This is case 5 as per document [1].
>>>
>>> It turns out that Case 5 can be implemented via a better pattern, as long
>>> as we're just dealing with a page at a time, briefly:
>>>
>>> lock_page()
>>> write to page's data
>>> unlock_page()
>>>
>>> ...which neatly synchronizes with writeback and other fs activities.
>>
>> Ahem, I left out a key step: set_page_dirty()!
>>
>> lock_page()
>> write to page's data
>> set_page_dirty()
>> unlock_page()
>>
> 
> Excuse me, but Documentation/core-api/pin_user_pages.rst says
> "CASE 5: Pinning in order to _write_ to the data within the page"
> while tomoyo_dump_page() is for "_read_ the data within the page".
> Do we want to convert to pin_user_pages_remote() or lock_page() ?
> 

Sorry, I missed the direction here, was too focused on the Case 5
aspect. Yes. Case 5 (which, again, I think we're about to re-document)
is only about *writing* to data within the page.

So in this case, where it is just reading from the page, I think it's
already from a gup vs pup point of view.

btw, it's not clear to me whether the current code is susceptible to any
sort of problem involving something writing to the page while it
is being dumped (I am curious). But changing from gup to pup wouldn't
fix that, if it were a problem. It a separate question from this patch.

(Souptick, if you're interested, the Case 5 documentation change and
callsite retrofit is all yours if you want it. Otherwise it's on
my list.)

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Tetsuo Handa]

On 2020/11/08 11:17, John Hubbard wrote:
>> Excuse me, but Documentation/core-api/pin_user_pages.rst says
>> "CASE 5: Pinning in order to _write_ to the data within the page"
>> while tomoyo_dump_page() is for "_read_ the data within the page".
>> Do we want to convert to pin_user_pages_remote() or lock_page() ?
>>
> 
> Sorry, I missed the direction here, was too focused on the Case 5
> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
> is only about *writing* to data within the page.
> 
> So in this case, where it is just reading from the page, I think it's
> already from a gup vs pup point of view.
> 
> btw, it's not clear to me whether the current code is susceptible to any
> sort of problem involving something writing to the page while it
> is being dumped (I am curious). But changing from gup to pup wouldn't
> fix that, if it were a problem. It a separate question from this patch.

The "struct page" tomoyo_dump_page() accesses is argv/envp arguments passed
to execve() syscall. Therefore, these pages are not visible from threads
except current thread, and thus there is no possibility that these pages
are modified by other threads while current thread is reading.

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 11/7/20 8:12 PM, Tetsuo Handa wrote:
> On 2020/11/08 11:17, John Hubbard wrote:
>>> Excuse me, but Documentation/core-api/pin_user_pages.rst says
>>> "CASE 5: Pinning in order to _write_ to the data within the page"
>>> while tomoyo_dump_page() is for "_read_ the data within the page".
>>> Do we want to convert to pin_user_pages_remote() or lock_page() ?
>>>
>>
>> Sorry, I missed the direction here, was too focused on the Case 5
>> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
>> is only about *writing* to data within the page.
>>
>> So in this case, where it is just reading from the page, I think it's
>> already from a gup vs pup point of view.
>>
>> btw, it's not clear to me whether the current code is susceptible to any
>> sort of problem involving something writing to the page while it
>> is being dumped (I am curious). But changing from gup to pup wouldn't
>> fix that, if it were a problem. It a separate question from this patch.
> 
> The "struct page" tomoyo_dump_page() accesses is argv/envp arguments passed
> to execve() syscall. Therefore, these pages are not visible from threads
> except current thread, and thus there is no possibility that these pages
> are modified by other threads while current thread is reading.
> 

Perfect. So since I accidentally left out the word "correct" above (I meant
to write, "it's already correct"), let me be extra clear: Souptick, we
should just drop this patch.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Sun, Nov 8, 2020 at 10:30 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 11/7/20 8:12 PM, Tetsuo Handa wrote:
> > On 2020/11/08 11:17, John Hubbard wrote:
> >>> Excuse me, but Documentation/core-api/pin_user_pages.rst says
> >>> "CASE 5: Pinning in order to _write_ to the data within the page"
> >>> while tomoyo_dump_page() is for "_read_ the data within the page".
> >>> Do we want to convert to pin_user_pages_remote() or lock_page() ?
> >>>
> >>
> >> Sorry, I missed the direction here, was too focused on the Case 5
> >> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
> >> is only about *writing* to data within the page.
> >>
> >> So in this case, where it is just reading from the page, I think it's
> >> already from a gup vs pup point of view.
> >>
> >> btw, it's not clear to me whether the current code is susceptible to any
> >> sort of problem involving something writing to the page while it
> >> is being dumped (I am curious). But changing from gup to pup wouldn't
> >> fix that, if it were a problem. It a separate question from this patch.
> >
> > The "struct page" tomoyo_dump_page() accesses is argv/envp arguments passed
> > to execve() syscall. Therefore, these pages are not visible from threads
> > except current thread, and thus there is no possibility that these pages
> > are modified by other threads while current thread is reading.
> >
>
> Perfect. So since I accidentally left out the word "correct" above (I meant
> to write, "it's already correct"), let me be extra clear: Souptick, we
> should just drop this patch.
>

Agreed. I will drop this patch.

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Sun, Nov 8, 2020 at 7:47 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 11/7/20 5:13 PM, Tetsuo Handa wrote:
> > On 2020/11/08 4:17, John Hubbard wrote:
> >> On 11/7/20 1:04 AM, John Hubbard wrote:
> >>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
> >>>> In 2019, we introduced pin_user_pages*() and now we are converting
> >>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
> >>>> be referred for more information. This is case 5 as per document [1].
> >>>
> >>> It turns out that Case 5 can be implemented via a better pattern, as long
> >>> as we're just dealing with a page at a time, briefly:
> >>>
> >>> lock_page()
> >>> write to page's data
> >>> unlock_page()
> >>>
> >>> ...which neatly synchronizes with writeback and other fs activities.
> >>
> >> Ahem, I left out a key step: set_page_dirty()!
> >>
> >> lock_page()
> >> write to page's data
> >> set_page_dirty()
> >> unlock_page()
> >>
> >
> > Excuse me, but Documentation/core-api/pin_user_pages.rst says
> > "CASE 5: Pinning in order to _write_ to the data within the page"
> > while tomoyo_dump_page() is for "_read_ the data within the page".
> > Do we want to convert to pin_user_pages_remote() or lock_page() ?
> >
>
> Sorry, I missed the direction here, was too focused on the Case 5
> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
> is only about *writing* to data within the page.
>
> So in this case, where it is just reading from the page, I think it's
> already from a gup vs pup point of view.
>
> btw, it's not clear to me whether the current code is susceptible to any
> sort of problem involving something writing to the page while it
> is being dumped (I am curious). But changing from gup to pup wouldn't
> fix that, if it were a problem. It a separate question from this patch.
>
> (Souptick, if you're interested, the Case 5 documentation change and
> callsite retrofit is all yours if you want it. Otherwise it's on
> my list.)

Sure John, I will take it.